From 336a98feb0d56b9ac54e12736b18785c27f75090 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 17 Apr 2020 16:53:51 -0700 Subject: [PATCH 1/2] Implement max settings option --- doc/CMakeLists.txt | 1 + doc/Makefile.am | 1 + lib/includes/nghttp2/nghttp2.h | 23 +++++++++++++ lib/nghttp2_helper.c | 2 ++ lib/nghttp2_option.c | 5 +++ lib/nghttp2_option.h | 5 +++ lib/nghttp2_session.c | 21 ++++++++++++ lib/nghttp2_session.h | 2 ++ tests/main.c | 2 ++ tests/nghttp2_session_test.c | 61 ++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 11 files changed, 124 insertions(+) diff --git a/doc/CMakeLists.txt b/doc/CMakeLists.txt index 34c02792..f3aec84d 100644 --- a/doc/CMakeLists.txt +++ b/doc/CMakeLists.txt @@ -42,6 +42,7 @@ set(APIDOCS nghttp2_option_set_no_recv_client_magic.rst nghttp2_option_set_peer_max_concurrent_streams.rst nghttp2_option_set_user_recv_extension_type.rst + nghttp2_option_set_max_settings.rst nghttp2_pack_settings_payload.rst nghttp2_priority_spec_check_default.rst nghttp2_priority_spec_default_init.rst diff --git a/doc/Makefile.am b/doc/Makefile.am index 4d73cef5..f073bfa4 100644 --- a/doc/Makefile.am +++ b/doc/Makefile.am @@ -69,6 +69,7 @@ APIDOCS= \ nghttp2_option_set_peer_max_concurrent_streams.rst \ nghttp2_option_set_user_recv_extension_type.rst \ nghttp2_option_set_max_outbound_ack.rst \ + nghttp2_option_set_max_settings.rst \ nghttp2_pack_settings_payload.rst \ nghttp2_priority_spec_check_default.rst \ nghttp2_priority_spec_default_init.rst \ diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index e3aeb9fe..9be6eea5 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -228,6 +228,13 @@ typedef struct { */ #define NGHTTP2_CLIENT_MAGIC_LEN 24 +/** + * @macro + * + * The default max number of settings per SETTINGS frame + */ +#define NGHTTP2_DEFAULT_MAX_SETTINGS 32 + /** * @enum * @@ -398,6 +405,11 @@ typedef enum { * receives an other type of frame. */ NGHTTP2_ERR_SETTINGS_EXPECTED = -536, + /** + * When a local endpoint receives too many settings entries + * in a single SETTINGS frame. + */ + NGHTTP2_ERR_TOO_MANY_SETTINGS = -537, /** * The errors < :enum:`NGHTTP2_ERR_FATAL` mean that the library is * under unexpected condition and processing was terminated (e.g., @@ -2659,6 +2671,17 @@ NGHTTP2_EXTERN void nghttp2_option_set_no_closed_streams(nghttp2_option *option, NGHTTP2_EXTERN void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, size_t val); +/** + * @function + * + * This function sets the maximum number of SETTINGS entries per + * SETTINGS frame that will be accepted. If more than those entries + * are received, the peer is considered to be misbehaving and session + * will be closed. The default value is 32. + */ +NGHTTP2_EXTERN void nghttp2_option_set_max_settings(nghttp2_option *option, + size_t val); + /** * @function * diff --git a/lib/nghttp2_helper.c b/lib/nghttp2_helper.c index 91136a61..0bd54147 100644 --- a/lib/nghttp2_helper.c +++ b/lib/nghttp2_helper.c @@ -334,6 +334,8 @@ const char *nghttp2_strerror(int error_code) { case NGHTTP2_ERR_FLOODED: return "Flooding was detected in this HTTP/2 session, and it must be " "closed"; + case NGHTTP2_ERR_TOO_MANY_SETTINGS: + return "SETTINGS frame contained more than the maximum allowed entries"; default: return "Unknown error code"; } diff --git a/lib/nghttp2_option.c b/lib/nghttp2_option.c index e53f22d3..34348e66 100644 --- a/lib/nghttp2_option.c +++ b/lib/nghttp2_option.c @@ -121,3 +121,8 @@ void nghttp2_option_set_max_outbound_ack(nghttp2_option *option, size_t val) { option->opt_set_mask |= NGHTTP2_OPT_MAX_OUTBOUND_ACK; option->max_outbound_ack = val; } + +void nghttp2_option_set_max_settings(nghttp2_option *option, size_t val) { + option->opt_set_mask |= NGHTTP2_OPT_MAX_SETTINGS; + option->max_settings = val; +} diff --git a/lib/nghttp2_option.h b/lib/nghttp2_option.h index 1f740aaa..939729fd 100644 --- a/lib/nghttp2_option.h +++ b/lib/nghttp2_option.h @@ -67,6 +67,7 @@ typedef enum { NGHTTP2_OPT_MAX_DEFLATE_DYNAMIC_TABLE_SIZE = 1 << 9, NGHTTP2_OPT_NO_CLOSED_STREAMS = 1 << 10, NGHTTP2_OPT_MAX_OUTBOUND_ACK = 1 << 11, + NGHTTP2_OPT_MAX_SETTINGS = 1 << 12, } nghttp2_option_flag; /** @@ -85,6 +86,10 @@ struct nghttp2_option { * NGHTTP2_OPT_MAX_OUTBOUND_ACK */ size_t max_outbound_ack; + /** + * NGHTTP2_OPT_MAX_SETTINGS + */ + size_t max_settings; /** * Bitwise OR of nghttp2_option_flag to determine that which fields * are specified. diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 563ccd7d..415e3477 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -458,6 +458,7 @@ static int session_new(nghttp2_session **session_ptr, (*session_ptr)->max_send_header_block_length = NGHTTP2_MAX_HEADERSLEN; (*session_ptr)->max_outbound_ack = NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM; + (*session_ptr)->max_settings = NGHTTP2_DEFAULT_MAX_SETTINGS; if (option) { if ((option->opt_set_mask & NGHTTP2_OPT_NO_AUTO_WINDOW_UPDATE) && @@ -521,6 +522,11 @@ static int session_new(nghttp2_session **session_ptr, if (option->opt_set_mask & NGHTTP2_OPT_MAX_OUTBOUND_ACK) { (*session_ptr)->max_outbound_ack = option->max_outbound_ack; } + + if ((option->opt_set_mask & NGHTTP2_OPT_MAX_SETTINGS) && + option->max_settings) { + (*session_ptr)->max_settings = option->max_settings; + } } rv = nghttp2_hd_deflate_init2(&(*session_ptr)->hd_deflater, @@ -5657,6 +5663,16 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, iframe->max_niv = iframe->frame.hd.length / NGHTTP2_FRAME_SETTINGS_ENTRY_LENGTH + 1; + if (iframe->max_niv - 1 > session->max_settings) { + rv = nghttp2_session_terminate_session_with_reason( + session, NGHTTP2_ENHANCE_YOUR_CALM, + "SETTINGS: too many setting entries"); + if (nghttp2_is_fatal(rv)) { + return rv; + } + return (ssize_t)inlen; + } + iframe->iv = nghttp2_mem_malloc(mem, sizeof(nghttp2_settings_entry) * iframe->max_niv); @@ -7425,6 +7441,11 @@ static int nghttp2_session_upgrade_internal(nghttp2_session *session, if (settings_payloadlen % NGHTTP2_FRAME_SETTINGS_ENTRY_LENGTH) { return NGHTTP2_ERR_INVALID_ARGUMENT; } + /* SETTINGS frame contains too many settings */ + if (settings_payloadlen / NGHTTP2_FRAME_SETTINGS_ENTRY_LENGTH + > session->max_settings) { + return NGHTTP2_ERR_TOO_MANY_SETTINGS; + } rv = nghttp2_frame_unpack_settings_payload2(&iv, &niv, settings_payload, settings_payloadlen, mem); if (rv != 0) { diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index d2082731..07bfbb6c 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -267,6 +267,8 @@ struct nghttp2_session { /* The maximum length of header block to send. Calculated by the same way as nghttp2_hd_deflate_bound() does. */ size_t max_send_header_block_length; + /* The maximum number of settings accepted per SETTINGS frame. */ + size_t max_settings; /* Next Stream ID. Made unsigned int to detect >= (1 << 31). */ uint32_t next_stream_id; /* The last stream ID this session initiated. For client session, diff --git a/tests/main.c b/tests/main.c index 41e0b03e..67eb4a1c 100644 --- a/tests/main.c +++ b/tests/main.c @@ -317,6 +317,8 @@ int main() { test_nghttp2_session_set_local_window_size) || !CU_add_test(pSuite, "session_cancel_from_before_frame_send", test_nghttp2_session_cancel_from_before_frame_send) || + !CU_add_test(pSuite, "session_too_many_settings", + test_nghttp2_session_too_many_settings) || !CU_add_test(pSuite, "session_removed_closed_stream", test_nghttp2_session_removed_closed_stream) || !CU_add_test(pSuite, "session_pause_data", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 6eb8e244..33ee3ad8 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -10614,6 +10614,67 @@ void test_nghttp2_session_cancel_from_before_frame_send(void) { nghttp2_session_del(session); } +void test_nghttp2_session_too_many_settings(void) { + nghttp2_session *session; + nghttp2_option *option; + nghttp2_session_callbacks callbacks; + nghttp2_frame frame; + nghttp2_bufs bufs; + nghttp2_buf *buf; + ssize_t rv; + my_user_data ud; + nghttp2_settings_entry iv[3]; + nghttp2_mem *mem; + nghttp2_outbound_item *item; + + mem = nghttp2_mem_default(); + frame_pack_bufs_init(&bufs); + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.on_frame_recv_callback = on_frame_recv_callback; + callbacks.send_callback = null_send_callback; + + nghttp2_option_new(&option); + nghttp2_option_set_max_settings(option, 1); + + nghttp2_session_client_new2(&session, &callbacks, &ud, option); + + CU_ASSERT(1 == session->max_settings); + + nghttp2_option_del(option); + + iv[0].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; + iv[0].value = 3000; + + iv[1].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; + iv[1].value = 16384; + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, dup_iv(iv, 2), + 2); + + rv = nghttp2_frame_pack_settings(&bufs, &frame.settings); + + CU_ASSERT(0 == rv); + CU_ASSERT(nghttp2_bufs_len(&bufs) > 0); + + nghttp2_frame_settings_free(&frame.settings, mem); + + buf = &bufs.head->buf; + assert(nghttp2_bufs_len(&bufs) == nghttp2_buf_len(buf)); + + ud.frame_recv_cb_called = 0; + + rv = nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf)); + CU_ASSERT((ssize_t)nghttp2_buf_len(buf) == rv); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_GOAWAY == item->frame.hd.type); + + nghttp2_bufs_reset(&bufs); + nghttp2_bufs_free(&bufs); + nghttp2_session_del(session); +} + static void prepare_session_removed_closed_stream(nghttp2_session *session, nghttp2_hd_deflater *deflater) { diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index e872c5d0..818c808d 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -156,6 +156,7 @@ void test_nghttp2_session_repeated_priority_change(void); void test_nghttp2_session_repeated_priority_submission(void); void test_nghttp2_session_set_local_window_size(void); void test_nghttp2_session_cancel_from_before_frame_send(void); +void test_nghttp2_session_too_many_settings(void); void test_nghttp2_session_removed_closed_stream(void); void test_nghttp2_session_pause_data(void); void test_nghttp2_session_no_closed_streams(void); From f8da73bd042f810f34d19f9eae02b46d870af394 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 19 Apr 2020 09:12:24 -0700 Subject: [PATCH 2/2] Earlier check for settings flood --- lib/nghttp2_session.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 415e3477..39f81f49 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -5653,6 +5653,12 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, break; } + /* Check the settings flood counter early to be safe */ + if (session->obq_flood_counter_ >= session->max_outbound_ack && + !(iframe->frame.hd.flags & NGHTTP2_FLAG_ACK)) { + return NGHTTP2_ERR_FLOODED; + } + iframe->state = NGHTTP2_IB_READ_SETTINGS; if (iframe->payloadleft) {