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 <[email protected]>

diff --git a/src/plugins/hs_apps/test_builtins.c b/src/plugins/hs_apps/test_builtins.c
index c314e71..4c324d5 100644 (file)
--- 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 f159079..ff32a1f 100644 (file)
--- 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 1a1ecd4..76021ae 100644 (file)
--- 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 9e0bb06..9dbca64 100644 (file)
--- 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 f74f92b..c20694c 100644 (file)
--- 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)
 {