From: Matus Fabian Date: Wed, 9 Apr 2025 18:34:53 +0000 (-0400) Subject: http: http/2 improvements X-Git-Tag: v25.10-rc0~106 X-Git-Url: https://gerrit.fd.io/r/gitweb?a=commitdiff_plain;h=497448ce93237975c9b234fe0554a1b2f029a5fa;p=vpp.git http: http/2 improvements - improved handling of malformed frames and protocol violations - if frame payload is not in fifo stop rx processing Type: improvement Change-Id: I4f5bd3f2fda23f2170b63af47d11deca666be27b Signed-off-by: Matus Fabian --- diff --git a/src/plugins/hs_apps/test_builtins.c b/src/plugins/hs_apps/test_builtins.c index c314e71b5df..4c324d5b953 100644 --- a/src/plugins/hs_apps/test_builtins.c +++ b/src/plugins/hs_apps/test_builtins.c @@ -161,6 +161,7 @@ test_builtins_init (vlib_main_t *vm) 0, "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); (*fp) (handle_get_test1, "test1", HTTP_REQ_GET); + (*fp) (handle_get_test1, "test1", HTTP_REQ_POST); (*fp) (handle_get_test2, "test2", HTTP_REQ_GET); (*fp) (handle_get_test_delayed, "test_delayed", HTTP_REQ_GET); (*fp) (handle_post_test3, "test3", HTTP_REQ_POST); diff --git a/src/plugins/http/http.c b/src/plugins/http/http.c index f1590799a05..ff32a1f2f8f 100644 --- a/src/plugins/http/http.c +++ b/src/plugins/http/http.c @@ -273,6 +273,20 @@ http_disconnect_transport (http_conn_t *hc) clib_warning ("disconnect returned"); } +void +http_shutdown_transport (http_conn_t *hc) +{ + vnet_shutdown_args_t a = { + .handle = hc->hc_tc_session_handle, + .app_index = http_main.app_index, + }; + + hc->state = HTTP_CONN_STATE_CLOSED; + + if (vnet_shutdown_session (&a)) + clib_warning ("shutdown returned"); +} + http_status_code_t http_sc_by_u16 (u16 status_code) { diff --git a/src/plugins/http/http2/hpack.c b/src/plugins/http/http2/hpack.c index 1a1ecd43cf3..76021ae14a6 100644 --- a/src/plugins/http/http2/hpack.c +++ b/src/plugins/http/http2/hpack.c @@ -192,6 +192,8 @@ hpack_decode_huffman (u8 **src, u8 *end, u8 **buf, uword *buf_len) /* trim code to correct length */ u32 code = (accumulator >> (accumulator_len - hg->code_len)) & ((1 << hg->code_len) - 1); + if (!code) + return HTTP2_ERROR_COMPRESSION_ERROR; /* find symbol in the list */ **buf = hg->symbols[code - hg->first_code]; (*buf)++; @@ -240,7 +242,8 @@ hpack_decode_string (u8 **src, u8 *end, u8 **buf, uword *buf_len) u8 *p, is_huffman; uword len; - ASSERT (*src < end); + if (*src == end) + return HTTP2_ERROR_COMPRESSION_ERROR; p = *src; /* H flag in first bit */ @@ -507,11 +510,6 @@ hpack_get_table_entry (uword index, http_token_t *name, http_token_t *value, name->len = e->name_len; if (value_is_indexed) { - if (PREDICT_FALSE (e->value_len == 0)) - { - HTTP_DBG (1, "static table entry [%llu] without value", index); - return HTTP2_ERROR_COMPRESSION_ERROR; - } value->base = e->value; value->len = e->value_len; } @@ -705,6 +703,9 @@ hpack_header_value_is_valid (u8 *value, u32 value_len) 0xffffffffffffffff, }; + if (value_len == 0) + return 1; + /* must not start or end with SP or HTAB */ if ((value[0] == 0x20 || value[0] == 0x09 || value[value_len - 1] == 0x20 || value[value_len - 1] == 0x09)) @@ -774,7 +775,8 @@ hpack_parse_req_pseudo_header (u8 *name, u32 name_len, u8 *value, case 5: if (!memcmp (name + 1, "path", 4)) { - if (control_data->parsed_bitmap & HPACK_PSEUDO_HEADER_PATH_PARSED) + if (control_data->parsed_bitmap & HPACK_PSEUDO_HEADER_PATH_PARSED || + value_len == 0) return HTTP2_ERROR_PROTOCOL_ERROR; control_data->parsed_bitmap |= HPACK_PSEUDO_HEADER_PATH_PARSED; control_data->path = value; @@ -830,6 +832,66 @@ hpack_parse_req_pseudo_header (u8 *name, u32 name_len, u8 *value, return HTTP2_ERROR_NO_ERROR; } +/* Special treatment for headers like: + * + * RFC9113 8.2.2: any message containing connection-specific header + * fields MUST be treated as malformed (connection, upgrade, keep-alive, + * proxy-connection, transfer-encoding), TE header MUST NOT contain any value + * other than "trailers" + * + * find headers that will be used later in preprocessing (content-length) + */ +always_inline http2_error_t +hpack_preprocess_header (u8 *name, u32 name_len, u8 *value, u32 value_len, + uword index, + hpack_request_control_data_t *control_data) +{ + switch (name_len) + { + case 2: + if (name[0] == 't' && name[1] == 'e' && + !http_token_is_case ((const char *) value, value_len, + http_token_lit ("trailers"))) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + case 7: + if (!memcmp (name, "upgrade", 7)) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + case 10: + switch (name[0]) + { + case 'c': + if (!memcmp (name + 1, "onnection", 9)) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + case 'k': + if (!memcmp (name + 1, "eep-alive", 9)) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + default: + break; + } + break; + case 14: + if (!memcmp (name, "content-length", 7) && + control_data->content_len_header_index == ~0) + control_data->content_len_header_index = index; + break; + case 16: + if (!memcmp (name, "proxy-connection", 16)) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + case 17: + if (!memcmp (name, "transfer-encoding", 17)) + return HTTP2_ERROR_PROTOCOL_ERROR; + break; + default: + break; + } + return HTTP2_ERROR_NO_ERROR; +} + __clib_export http2_error_t hpack_parse_request (u8 *src, u32 src_len, u8 *dst, u32 dst_len, hpack_request_control_data_t *control_data, @@ -903,14 +965,15 @@ hpack_parse_request (u8 *src, u32 src_len, u8 *dst, u32 dst_len, header->value_len = value_len; control_data->headers_len += name_len; control_data->headers_len += value_len; - /* find headers that will be used later in preprocessing */ if (regular_header_parsed) { - if (control_data->content_len_header_index == ~0 && - http_token_is ((const char *) name, name_len, - hpack_headers[HTTP_HEADER_CONTENT_LENGTH].base, - hpack_headers[HTTP_HEADER_CONTENT_LENGTH].len)) - control_data->content_len_header_index = header - *headers; + rv = hpack_preprocess_header (name, name_len, value, value_len, + header - *headers, control_data); + if (rv != HTTP2_ERROR_NO_ERROR) + { + HTTP_DBG (1, "connection-specific header present"); + return rv; + } } } control_data->control_data_len = dst_len - b_left; diff --git a/src/plugins/http/http2/http2.c b/src/plugins/http/http2/http2.c index 9e0bb06f477..9dbca646955 100644 --- a/src/plugins/http/http2/http2.c +++ b/src/plugins/http/http2/http2.c @@ -223,30 +223,37 @@ http2_connection_error (http_conn_t *hc, http2_error_t error, response = http_get_tx_buf (hc); http2_frame_write_goaway (error, h2c->last_processed_stream_id, &response); - http_io_ts_after_write (hc, sp, 0, 1); + http_io_ts_write (hc, response, vec_len (response), sp); + http_io_ts_after_write (hc, sp, 1, 1); hash_foreach (stream_id, req_index, h2c->req_by_stream_id, ({ req = http2_req_get (req_index, hc->c_thread_index); if (req->stream_state != HTTP2_STREAM_STATE_CLOSED) - session_transport_closing_notify (&req->base.connection); + session_transport_reset_notify (&req->base.connection); })); - http_disconnect_transport (hc); + http_shutdown_transport (hc); } -/* send RST_STREAM frame and notify app */ always_inline void -http2_stream_error (http_conn_t *hc, http2_req_t *req, http2_error_t error, - transport_send_params_t *sp) +http2_send_stream_error (http_conn_t *hc, u32 stream_id, http2_error_t error, + transport_send_params_t *sp) { u8 *response; - ASSERT (req->stream_state > HTTP2_STREAM_STATE_IDLE); - response = http_get_tx_buf (hc); - http2_frame_write_rst_stream (error, req->stream_id, &response); + http2_frame_write_rst_stream (error, stream_id, &response); http_io_ts_write (hc, response, vec_len (response), sp); - http_io_ts_after_write (hc, sp, 0, 1); + http_io_ts_after_write (hc, sp, 1, 1); +} + +/* send RST_STREAM frame and notify app */ +always_inline void +http2_stream_error (http_conn_t *hc, http2_req_t *req, http2_error_t error, + transport_send_params_t *sp) +{ + ASSERT (req->stream_state > HTTP2_STREAM_STATE_IDLE); + http2_send_stream_error (hc, req->stream_id, error, sp); req->stream_state = HTTP2_STREAM_STATE_CLOSED; if (req->flags & HTTP2_REQ_F_APP_CLOSED) session_transport_closed_notify (&req->base.connection); @@ -382,8 +389,13 @@ http2_req_state_wait_transport_method (http_conn_t *hc, http2_req_t *req, } new_state = HTTP_REQ_STATE_TRANSPORT_IO_MORE_DATA; } - /* TODO: handle following case (for now we just discard data frames) - * req->base.body_len == 0 && req->stream_state == HTTP2_STREAM_STATE_OPEN */ + /* TODO: message framing without content length using END_STREAM flag */ + if (req->base.body_len == 0 && req->stream_state == HTTP2_STREAM_STATE_OPEN) + { + HTTP_DBG (1, "no content-length and DATA frame expected"); + *error = HTTP2_ERROR_INTERNAL_ERROR; + return HTTP_SM_ERROR; + } req->base.to_recv = req->base.body_len; req->base.target_path_len = control_data.path_len; @@ -430,10 +442,17 @@ http2_req_state_transport_io_more_data (http_conn_t *hc, http2_req_t *req, transport_send_params_t *sp, http2_error_t *error) { - http_io_as_write (&req->base, req->payload, req->payload_len); req->base.to_recv -= req->payload_len; - if (req->base.to_recv) - http_req_state_change (&req->base, HTTP_REQ_STATE_WAIT_APP_REPLY); + if (req->base.to_recv == 0) + { + if (req->stream_state != HTTP2_STREAM_STATE_HALF_CLOSED) + { + http2_stream_error (hc, req, HTTP2_ERROR_PROTOCOL_ERROR, sp); + return HTTP_SM_STOP; + } + http_req_state_change (&req->base, HTTP_REQ_STATE_WAIT_APP_REPLY); + } + http_io_as_write (&req->base, req->payload, req->payload_len); http_app_worker_rx_notify (&req->base); return HTTP_SM_STOP; @@ -636,6 +655,7 @@ http2_req_run_state_machine (http_conn_t *hc, http2_req_t *req, static http2_error_t http2_handle_headers_frame (http_conn_t *hc, http2_frame_header_t *fh) { + http2_main_t *h2m = &http2_main; http2_req_t *req; u8 *rx_buf; http2_error_t rv; @@ -663,6 +683,15 @@ http2_handle_headers_frame (http_conn_t *hc, http2_frame_header_t *fh) return HTTP2_ERROR_STREAM_CLOSED; } h2c->last_opened_stream_id = fh->stream_id; + if (hash_elts (h2c->req_by_stream_id) == + h2m->settings.max_concurrent_streams) + { + HTTP_DBG (1, "SETTINGS_MAX_CONCURRENT_STREAMS exceeded"); + http_io_ts_drain (hc, fh->length); + http2_send_stream_error (hc, fh->stream_id, + HTTP2_ERROR_REFUSED_STREAM, 0); + return HTTP2_ERROR_NO_ERROR; + } req = http2_conn_alloc_req (hc, fh->stream_id); http_conn_accept_request (hc, &req->base); http_req_state_change (&req->base, HTTP_REQ_STATE_WAIT_TRANSPORT_METHOD); @@ -708,18 +737,26 @@ http2_handle_data_frame (http_conn_t *hc, http2_frame_header_t *fh) req = http2_conn_get_req (hc, fh->stream_id); if (!req) { + if (fh->stream_id == 0) + { + HTTP_DBG (1, "DATA frame with stream id 0"); + return HTTP2_ERROR_PROTOCOL_ERROR; + } h2c = http2_conn_ctx_get_w_thread (hc); if (fh->stream_id <= h2c->last_opened_stream_id) { HTTP_DBG (1, "stream closed, ignoring frame"); + http2_send_stream_error (hc, fh->stream_id, + HTTP2_ERROR_STREAM_CLOSED, 0); return HTTP2_ERROR_NO_ERROR; } else return HTTP2_ERROR_PROTOCOL_ERROR; } - /* bogus state, connection error */ - if (req->stream_state == HTTP2_STREAM_STATE_HALF_CLOSED) + /* bogus state */ + if (hc->flags & HTTP_CONN_F_IS_SERVER && + req->stream_state != HTTP2_STREAM_STATE_OPEN) { HTTP_DBG (1, "error: stream already half-closed"); http2_stream_error (hc, req, HTTP2_ERROR_STREAM_CLOSED, 0); @@ -872,14 +909,12 @@ http2_handle_goaway_frame (http_conn_t *hc, http2_frame_header_t *fh) else { /* connection error */ - hc->state = HTTP_CONN_STATE_CLOSED; - if (!(hc->flags & HTTP_CONN_F_HAS_REQUEST)) - return HTTP2_ERROR_NO_ERROR; h2c = http2_conn_ctx_get_w_thread (hc); hash_foreach (stream_id, req_index, h2c->req_by_stream_id, ({ req = http2_req_get (req_index, hc->c_thread_index); session_transport_reset_notify (&req->base.connection); })); + http_shutdown_transport (hc); } return HTTP2_ERROR_NO_ERROR; @@ -890,14 +925,17 @@ http2_handle_ping_frame (http_conn_t *hc, http2_frame_header_t *fh) { u8 *rx_buf, *resp = 0; - if (fh->stream_id != 0 || fh->length != HTTP2_PING_PAYLOAD_LEN || - fh->flags & HTTP2_FRAME_FLAG_ACK) + if (fh->stream_id != 0 || fh->length != HTTP2_PING_PAYLOAD_LEN) return HTTP2_ERROR_PROTOCOL_ERROR; rx_buf = http_get_rx_buf (hc); vec_validate (rx_buf, fh->length - 1); http_io_ts_read (hc, rx_buf, fh->length, 0); + /* RFC9113 6.7: The endpoint MUST NOT respond to PING frames with ACK */ + if (fh->flags & HTTP2_FRAME_FLAG_ACK) + return HTTP2_ERROR_NO_ERROR; + http2_frame_write_ping (1, rx_buf, &resp); http_io_ts_write (hc, resp, vec_len (resp), 0); vec_free (resp); @@ -906,6 +944,18 @@ http2_handle_ping_frame (http_conn_t *hc, http2_frame_header_t *fh) return HTTP2_ERROR_NO_ERROR; } +static http2_error_t +http2_handle_push_promise (http_conn_t *hc, http2_frame_header_t *fh) +{ + if (hc->flags & HTTP_CONN_F_IS_SERVER) + { + HTTP_DBG (1, "error: server received PUSH_PROMISE"); + return HTTP2_ERROR_PROTOCOL_ERROR; + } + /* TODO: client */ + return HTTP2_ERROR_INTERNAL_ERROR; +} + static_always_inline int http2_expect_preface (http_conn_t *hc, http2_conn_ctx_t *h2c) { @@ -1089,6 +1139,7 @@ http2_transport_connected_callback (http_conn_t *hc) static void http2_transport_rx_callback (http_conn_t *hc) { + http2_main_t *h2m = &http2_main; http2_frame_header_t fh; u32 to_deq; u8 *rx_buf; @@ -1137,16 +1188,31 @@ http2_transport_rx_callback (http_conn_t *hc) while (to_deq >= HTTP2_FRAME_HEADER_SIZE) { rx_buf = http_get_rx_buf (hc); - http_io_ts_read (hc, rx_buf, HTTP2_FRAME_HEADER_SIZE, 0); + http_io_ts_read (hc, rx_buf, HTTP2_FRAME_HEADER_SIZE, 1); to_deq -= HTTP2_FRAME_HEADER_SIZE; http2_frame_header_read (rx_buf, &fh); + if (fh.length > h2m->settings.max_frame_size) + { + HTTP_DBG (1, "frame length %lu exceeded SETTINGS_MAX_FRAME_SIZE %lu", + fh.length, h2m->settings.max_frame_size); + http2_connection_error (hc, HTTP2_ERROR_FRAME_SIZE_ERROR, 0); + return; + } if (fh.length > to_deq) { - HTTP_DBG (1, "incomplete frame, to deq %lu, frame length %lu", - to_deq, fh.length); - http2_connection_error (hc, HTTP2_ERROR_PROTOCOL_ERROR, 0); + HTTP_DBG ( + 1, "frame payload not yet received, to deq %lu, frame length %lu", + to_deq, fh.length); + if (http_io_ts_fifo_size (hc, 1) < + (fh.length + HTTP2_FRAME_HEADER_SIZE)) + { + clib_warning ("ts rx fifo too small to hold frame (%u)", + fh.length + HTTP2_FRAME_HEADER_SIZE); + http2_connection_error (hc, HTTP2_ERROR_PROTOCOL_ERROR, 0); + } return; } + http_io_ts_drain (hc, HTTP2_FRAME_HEADER_SIZE); to_deq -= fh.length; HTTP_DBG (1, "frame type 0x%02x", fh.type); @@ -1178,13 +1244,13 @@ http2_transport_rx_callback (http_conn_t *hc) rv = HTTP2_ERROR_INTERNAL_ERROR; break; case HTTP2_FRAME_TYPE_PUSH_PROMISE: - /* TODO */ - rv = HTTP2_ERROR_PROTOCOL_ERROR; + rv = http2_handle_push_promise (hc, &fh); break; case HTTP2_FRAME_TYPE_PRIORITY: /* deprecated */ default: /* ignore unknown frame type */ http_io_ts_drain (hc, fh.length); + rv = HTTP2_ERROR_NO_ERROR; break; } diff --git a/src/plugins/http/http_private.h b/src/plugins/http/http_private.h index f74f92b0949..c20694c6d90 100644 --- a/src/plugins/http/http_private.h +++ b/src/plugins/http/http_private.h @@ -347,6 +347,15 @@ int http_v_find_index (u8 *vec, u32 offset, u32 num, char *str); */ void http_disconnect_transport (http_conn_t *hc); +/** + * Shutdown HTTP connection. + * + * Close TX side of the underlying transport. + * + * @param hc HTTP connection to shutdown. + */ +void http_shutdown_transport (http_conn_t *hc); + /** * Convert numeric representation of status code to @c http_status_code_t. * @@ -622,6 +631,16 @@ http_io_as_drain_all (http_req_t *req) /* Abstraction of transport session fifo operations */ +always_inline u32 +http_io_ts_fifo_size (http_conn_t *hc, u8 is_rx) +{ + session_t *ts = session_get_from_handle (hc->hc_tc_session_handle); + if (is_rx) + return svm_fifo_size (ts->rx_fifo); + else + return svm_fifo_size (ts->tx_fifo); +} + always_inline u32 http_io_ts_max_read (http_conn_t *hc) {