From a1c3f89c72e512ebc015154bb9b83a21dce5562f Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sun, 8 Sep 2013 23:21:06 +0200 Subject: [PATCH] nghttp2_pack_settings_payload: added a buffer size argument To make it less likely that a user gets a buffer overflow if a too small buffer is used. --- lib/includes/nghttp2/nghttp2.h | 22 +++++++++++++++------- lib/nghttp2_submit.c | 7 ++++++- tests/nghttp2_session_test.c | 19 ++++++++++++++----- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index c2682f59..309359eb 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -254,6 +254,10 @@ typedef enum { * Flow control error */ NGHTTP2_ERR_FLOW_CONTROL = -524, + /** + * Insufficient buffer size given to function. + */ + NGHTTP2_ERR_INSUFF_BUFSIZE = -525, /** * The errors < :enum:`NGHTTP2_ERR_FATAL` mean that the library is * under unexpected condition and cannot process any further data @@ -1502,13 +1506,13 @@ int nghttp2_session_upgrade(nghttp2_session *session, /** * @function * - * Serializes the SETTINGS values |iv| in the |buf|. The number of - * entry pointed by |iv| array is given by the |niv|. This function - * may reorder the pointers in |iv|. The |buf| must have enough region - * to hold serialized data. The required space for the |niv| entries - * are ``8*niv`` bytes. This function is used mainly for creating - * SETTINGS payload to be sent with ``HTTP2-Settings`` header field in - * HTTP Upgrade request. The data written in |buf| is NOT + * Serializes the SETTINGS values |iv| in the |buf|. The size of the |buf| is + * specified by |buflen|. The number of entries in the |iv| array is given by + * |niv|. This function may reorder the pointers in |iv|. The required space + * in |buf| for the |niv| entries is ``8*niv`` bytes and if the given buffer + * is too small, an error is returned. This function is used mainly for + * creating a SETTINGS payload to be sent with the ``HTTP2-Settings`` header + * field in an HTTP Upgrade request. The data written in |buf| is NOT * base64url encoded and the application is responsible for encoding. * * This function returns the number of bytes written in |buf|, or one @@ -1516,8 +1520,12 @@ int nghttp2_session_upgrade(nghttp2_session *session, * * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` * The |iv| contains duplicate settings ID or invalid value. + * + * :enum:`NGHTTP2_ERR_INSUFF_BUFSIZE` + * The provided |buflen| size is too small to hold the output. */ ssize_t nghttp2_pack_settings_payload(uint8_t *buf, + size_t buflen, nghttp2_settings_entry *iv, size_t niv); /** diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 181f433f..6bb952a2 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -1,7 +1,7 @@ /* * nghttp2 - HTTP/2.0 C Library * - * Copyright (c) 2012 Tatsuhiro Tsujikawa + * Copyright (c) 2012, 2013 Tatsuhiro Tsujikawa * * Permission is hereby granted, free of charge, to any person obtaining * a copy of this software and associated documentation files (the @@ -348,6 +348,7 @@ int nghttp2_submit_data(nghttp2_session *session, uint8_t flags, } ssize_t nghttp2_pack_settings_payload(uint8_t *buf, + size_t buflen, nghttp2_settings_entry *iv, size_t niv) { /* Assume that current flow_control_option is 0 (which means that @@ -355,5 +356,9 @@ ssize_t nghttp2_pack_settings_payload(uint8_t *buf, if(!nghttp2_iv_check(iv, niv, 0)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } + + if(buflen < (niv * 8)) + return NGHTTP2_ERR_INSUFF_BUFSIZE; + return nghttp2_frame_pack_settings_payload(buf, iv, niv); } diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 738fa0dd..269e3154 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1544,7 +1544,9 @@ void test_nghttp2_session_upgrade(void) iv[0].value = 1; iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; iv[1].value = 4095; - settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, iv, 2); + settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, + sizeof(settings_payload), + iv, 2); /* Check client side */ nghttp2_session_client_new(&session, &callbacks, NULL); @@ -1594,7 +1596,9 @@ void test_nghttp2_session_upgrade(void) /* Check required settings */ iv[0].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; iv[0].value = 1; - settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, iv, 1); + settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, + sizeof(settings_payload), + iv, 1); nghttp2_session_client_new(&session, &callbacks, NULL); CU_ASSERT(NGHTTP2_ERR_PROTO == @@ -1604,7 +1608,9 @@ void test_nghttp2_session_upgrade(void) iv[0].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; iv[0].value = 4095; - settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, iv, 1); + settings_payloadlen = nghttp2_pack_settings_payload(settings_payload, + sizeof(settings_payload), + iv, 1); nghttp2_session_client_new(&session, &callbacks, NULL); CU_ASSERT(NGHTTP2_ERR_PROTO == @@ -3278,7 +3284,7 @@ void test_nghttp2_pack_settings_payload(void) { nghttp2_settings_entry iv[2]; uint8_t buf[64]; - size_t len; + ssize_t len; nghttp2_settings_entry *resiv; size_t resniv; @@ -3287,7 +3293,7 @@ void test_nghttp2_pack_settings_payload(void) iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; iv[1].value = 4095; - len = nghttp2_pack_settings_payload(buf, iv, 2); + len = nghttp2_pack_settings_payload(buf, sizeof(buf), iv, 2); CU_ASSERT(16 == len); CU_ASSERT(0 == nghttp2_frame_unpack_settings_payload(&resiv, &resniv, buf, len)); @@ -3298,4 +3304,7 @@ void test_nghttp2_pack_settings_payload(void) CU_ASSERT(4095 == resiv[1].value); free(resiv); + + len = nghttp2_pack_settings_payload(buf, 15 /* too small */, iv, 2); + CU_ASSERT(NGHTTP2_ERR_INSUFF_BUFSIZE == len); }