Move header name/value pair validation to on_*_received functions

lib/nghttp2_frame.c

@@ -585,13 +585,30 @@ nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv,
   return iv_copy;
 }
 
-int nghttp2_nv_array_check_null(const nghttp2_nv *nva, size_t nvlen)
+int nghttp2_nv_array_check_nocase(const nghttp2_nv *nva, size_t nvlen)
 {
   size_t i;
   for(i = 0; i < nvlen; ++i) {
     if(!nghttp2_check_header_name_nocase(nva[i].name, nva[i].namelen)) {
       return 0;
     }
+    if(!nghttp2_check_header_value(nva[i].value, nva[i].valuelen)) {
+      return 0;
+    }
+  }
+  return 1;
+}
+
+int nghttp2_nv_array_check(const nghttp2_nv *nva, size_t nvlen)
+{
+  size_t i;
+  for(i = 0; i < nvlen; ++i) {
+    if(!nghttp2_check_header_name(nva[i].name, nva[i].namelen)) {
+      return 0;
+    }
+    if(!nghttp2_check_header_value(nva[i].value, nva[i].valuelen)) {
+      return 0;
+    }
   }
   return 1;
 }

lib/nghttp2_frame.h

@@ -552,13 +552,20 @@ int nghttp2_nv_equal(const nghttp2_nv *a, const nghttp2_nv *b);
 void nghttp2_nv_array_del(nghttp2_nv *nva);
 
 /*
- * Checks names are not empty string and do not contain control
+ * Checks header name/value pair is well-formed. This function allows
- * characters. This function allows captital alphabet letters in name.
+ * captital alphabet letters in name.
  *
  * This function returns nonzero if it succeeds, or 0.
  */
-int nghttp2_nv_array_check_null(const nghttp2_nv *nva, size_t nvlen);
+int nghttp2_nv_array_check_nocase(const nghttp2_nv *nva, size_t nvlen);
 
+/*
+ * Checks header name/value pair is well-formed. This function does
+ * not allow captital alphabet letters in name.
+ *
+ * This function returns nonzero if it succeeds, or 0.
+ */
+int nghttp2_nv_array_check(const nghttp2_nv *nva, size_t nvlen);
 
 /*
  * Checks that the |iv|, which includes |niv| entries, does not have

lib/nghttp2_hd.c

@@ -1354,12 +1354,6 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater,
       }
       in += namelen;
 
-      if(!nghttp2_check_header_name(nv.name, nv.namelen)) {
-        free(decoded_huffman_name);
-        rv = NGHTTP2_ERR_HEADER_COMP;
-        goto fail;
-      }
-
       if(in == last) {
         DEBUGF(fprintf(stderr, "No value found\n"));
         free(decoded_huffman_name);

lib/nghttp2_helper.c

@@ -219,6 +219,19 @@ int nghttp2_check_header_name_nocase(const uint8_t *name, size_t len)
   return check_header_name(name, len, 1);
 }
 
+int nghttp2_check_header_value(const uint8_t* value, size_t len)
+{
+  size_t i;
+  for(i = 0; i < len; ++i) {
+    /* Only allow NUL or ASCII range [0x20, 0x7e], inclusive, to match
+       HTTP/1 sematics */
+    if(value[i] != '\0' && (0x20u > value[i] || value[i] > 0x7eu)) {
+      return 0;
+    }
+  }
+  return 1;
+}
+
 const char* nghttp2_strerror(int error_code)
 {
   switch(error_code) {

lib/nghttp2_helper.h

@@ -119,20 +119,27 @@ int nghttp2_should_send_window_update(int32_t local_window_size,
                                       int32_t recv_window_size);
 
 /*
- * Checks the header name in |name| with |len| bytes is valid.
+ * Checks the header name in |name| with |len| bytes is well-formed.
  *
  * This function returns nonzero if it succeeds, or 0.
  */
 int nghttp2_check_header_name(const uint8_t *name, size_t len);
 
 /*
- * Checks the header name in |name| with |len| bytes is valid. This
+ * Checks the header name in |name| with |len| bytes is
- * function accepts also characters in [A-Z].
+ * well-formed. This function accepts also characters in [A-Z].
  *
  * This function returns nonzero if it succeeds, or 0.
  */
 int nghttp2_check_header_name_nocase(const uint8_t *name, size_t len);
 
+/*
+ * Checks the header value in |value| with |len| bytes is well-formed.
+ *
+ * This function returns nonzero if it succeeds, or 0.
+ */
+int nghttp2_check_header_value(const uint8_t* value, size_t len);
+
 /*
  * Deallocates memory space pointed by |ptr|. This function exists for
  * the application to free the memory space allocated by the library

lib/nghttp2_session.c

@@ -1909,6 +1909,10 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session,
                                                      NGHTTP2_PROTOCOL_ERROR);
   }
   session->last_recv_stream_id = frame->hd.stream_id;
+  if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) {
+    return nghttp2_session_handle_invalid_stream(session, frame,
+                                                 NGHTTP2_PROTOCOL_ERROR);
+  }
   error_code = nghttp2_session_validate_request_headers(session,
                                                         &frame->headers);
   if(error_code != NGHTTP2_NO_ERROR) {
@@ -1965,6 +1969,10 @@ int nghttp2_session_on_response_headers_received(nghttp2_session *session,
     return nghttp2_session_handle_invalid_stream(session, frame,
                                                  NGHTTP2_STREAM_CLOSED);
   }
+  if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) {
+    return nghttp2_session_handle_invalid_stream(session, frame,
+                                                 NGHTTP2_PROTOCOL_ERROR);
+  }
   stream->state = NGHTTP2_STREAM_OPENED;
   rv = nghttp2_session_call_on_frame_received(session, frame);
   if(rv != 0) {
@@ -2001,6 +2009,10 @@ int nghttp2_session_on_push_response_headers_received(nghttp2_session *session,
     /* We don't accept new stream after GOAWAY is sent or received. */
     return 0;
   }
+  if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) {
+    return nghttp2_session_handle_invalid_stream(session, frame,
+                                                 NGHTTP2_PROTOCOL_ERROR);
+  }
   rv = nghttp2_session_validate_request_headers(session, &frame->headers);
   if(rv != 0) {
     return nghttp2_session_handle_invalid_stream(session, frame, rv);
@@ -2055,6 +2067,10 @@ int nghttp2_session_on_headers_received(nghttp2_session *session,
     return nghttp2_session_handle_invalid_stream(session, frame,
                                                  NGHTTP2_STREAM_CLOSED);
   }
+  if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) {
+    return nghttp2_session_handle_invalid_stream(session, frame,
+                                                 NGHTTP2_PROTOCOL_ERROR);
+  }
   if(nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) {
     if(stream->state == NGHTTP2_STREAM_OPENED) {
       r = nghttp2_session_call_on_frame_received(session, frame);
@@ -2561,12 +2577,13 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session,
        frame->push_promise.promised_stream_id,
        NGHTTP2_REFUSED_STREAM);
   }
-
   if(!nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) {
     return nghttp2_session_handle_invalid_connection(session, frame,
                                                      NGHTTP2_PROTOCOL_ERROR);
   }
-  if(stream->shut_flags & NGHTTP2_SHUT_RD) {
+  if((stream->shut_flags & NGHTTP2_SHUT_RD) ||
+     !nghttp2_nv_array_check(frame->push_promise.nva,
+                             frame->push_promise.nvlen)) {
     if(session->callbacks.on_invalid_frame_recv_callback) {
       if(session->callbacks.on_invalid_frame_recv_callback
          (session, frame, NGHTTP2_PROTOCOL_ERROR, session->user_data) != 0) {

lib/nghttp2_submit.c

@@ -110,7 +110,7 @@ static int nghttp2_submit_headers_shared_nva
 {
   ssize_t rv;
   nghttp2_nv *nva_copy;
-  if(!nghttp2_nv_array_check_null(nva, nvlen)) {
+  if(!nghttp2_nv_array_check_nocase(nva, nvlen)) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
   rv = nghttp2_nv_array_copy(&nva_copy, nva, nvlen);
@@ -199,7 +199,7 @@ int nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags,
   uint8_t flags_copy;
   int rv;
 
-  if(!nghttp2_nv_array_check_null(nva, nvlen)) {
+  if(!nghttp2_nv_array_check_nocase(nva, nvlen)) {
     return NGHTTP2_ERR_INVALID_ARGUMENT;
   }
   if(nghttp2_session_get_stream(session, stream_id) == NULL) {

tests/main.c

@@ -215,8 +215,8 @@ int main(int argc, char* argv[])
                    test_nghttp2_frame_pack_goaway) ||
       !CU_add_test(pSuite, "frame_pack_window_update",
                    test_nghttp2_frame_pack_window_update) ||
-      !CU_add_test(pSuite, "nv_array_check_null",
+      !CU_add_test(pSuite, "nv_array_check",
-                   test_nghttp2_nv_array_check_null) ||
+                   test_nghttp2_nv_array_check) ||
       !CU_add_test(pSuite, "nv_array_copy", test_nghttp2_nv_array_copy) ||
       !CU_add_test(pSuite, "iv_check", test_nghttp2_iv_check) ||
       !CU_add_test(pSuite, "hd_deflate", test_nghttp2_hd_deflate) ||
@@ -250,7 +250,9 @@ int main(int argc, char* argv[])
       !CU_add_test(pSuite, "adjust_local_window_size",
                    test_nghttp2_adjust_local_window_size) ||
       !CU_add_test(pSuite, "check_header_name",
-                   test_nghttp2_check_header_name)
+                   test_nghttp2_check_header_name) ||
+      !CU_add_test(pSuite, "check_header_value",
+                   test_nghttp2_check_header_value)
       ) {
      CU_cleanup_registry();
      return CU_get_error();

tests/nghttp2_frame_test.c

@@ -361,7 +361,7 @@ void test_nghttp2_frame_pack_window_update(void)
   nghttp2_frame_window_update_free(&frame);
 }
 
-void test_nghttp2_nv_array_check_null(void)
+void test_nghttp2_nv_array_check(void)
 {
   nghttp2_nv nva1[] = {MAKE_NV("path", "/"),
                        MAKE_NV("host", "a")};
@@ -370,11 +370,28 @@ void test_nghttp2_nv_array_check_null(void)
   nghttp2_nv nva3[] = {MAKE_NV("path", "/"),
                        MAKE_NV("host\x01", "a")};
   nghttp2_nv nva4[] = {MAKE_NV("PATH", "/")};
+  nghttp2_nv nva5[] = {MAKE_NV("path", "foo"),
+                       MAKE_NV("host", "foo\x00")};
+  nghttp2_nv nva6[] = {MAKE_NV("path", "foo"),
+                       MAKE_NV("host", "foo\x01")};
 
-  CU_ASSERT(nghttp2_nv_array_check_null(nva1, ARRLEN(nva1)));
+  CU_ASSERT(nghttp2_nv_array_check(nva1, ARRLEN(nva1)));
-  CU_ASSERT(0 == nghttp2_nv_array_check_null(nva2, ARRLEN(nva2)));
+  CU_ASSERT(nghttp2_nv_array_check_nocase(nva1, ARRLEN(nva1)));
-  CU_ASSERT(0 == nghttp2_nv_array_check_null(nva3, ARRLEN(nva3)));
+
-  CU_ASSERT(nghttp2_nv_array_check_null(nva4, ARRLEN(nva4)));
+  CU_ASSERT(0 == nghttp2_nv_array_check(nva2, ARRLEN(nva2)));
+  CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva2, ARRLEN(nva2)));
+
+  CU_ASSERT(0 == nghttp2_nv_array_check(nva3, ARRLEN(nva3)));
+  CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva3, ARRLEN(nva3)));
+
+  CU_ASSERT(0 == nghttp2_nv_array_check(nva4, ARRLEN(nva4)));
+  CU_ASSERT(nghttp2_nv_array_check_nocase(nva4, ARRLEN(nva4)));
+
+  CU_ASSERT(nghttp2_nv_array_check(nva5, ARRLEN(nva5)));
+  CU_ASSERT(nghttp2_nv_array_check_nocase(nva5, ARRLEN(nva5)));
+
+  CU_ASSERT(0 == nghttp2_nv_array_check(nva6, ARRLEN(nva6)));
+  CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva6, ARRLEN(nva6)));
 }
 
 void test_nghttp2_nv_array_copy(void)

tests/nghttp2_frame_test.h

@@ -34,7 +34,7 @@ void test_nghttp2_frame_pack_push_promise(void);
 void test_nghttp2_frame_pack_ping(void);
 void test_nghttp2_frame_pack_goaway(void);
 void test_nghttp2_frame_pack_window_update(void);
-void test_nghttp2_nv_array_check_null(void);
+void test_nghttp2_nv_array_check(void);
 void test_nghttp2_nv_array_copy(void);
 void test_nghttp2_iv_check(void);
 

tests/nghttp2_helper_test.c

@@ -181,3 +181,18 @@ void test_nghttp2_check_header_name(void)
   CU_ASSERT(!check_header_name_nocase(""));
   CU_ASSERT(!check_header_name_nocase(":"));
 }
+
+#define check_header_value(S)                                   \
+  nghttp2_check_header_value((const uint8_t*)S, sizeof(S) - 1)
+
+void test_nghttp2_check_header_value(void)
+{
+  uint8_t goodval[] = { 'a', '\0', 'b' };
+  uint8_t badval1[] = { 'a', 0x1fu, 'b' };
+  uint8_t badval2[] = { 'a', 0x7fu, 'b' };
+
+  CU_ASSERT(check_header_value(" !|}~"));
+  CU_ASSERT(check_header_value(goodval));
+  CU_ASSERT(!check_header_value(badval1));
+  CU_ASSERT(!check_header_value(badval2));
+}

tests/nghttp2_helper_test.h

@@ -27,5 +27,6 @@
 
 void test_nghttp2_adjust_local_window_size(void);
 void test_nghttp2_check_header_name(void);
+void test_nghttp2_check_header_value(void);
 
 #endif /* NGHTTP2_HELPER_TEST_H */

tests/nghttp2_session_test.c

@@ -794,6 +794,9 @@ void test_nghttp2_session_on_request_headers_received(void)
   nghttp2_frame frame;
   nghttp2_stream *stream;
   int32_t stream_id = 1;
+  nghttp2_nv malformed_nva[] = { MAKE_NV(":path", "\x01") };
+  nghttp2_nv *nva;
+  size_t nvlen;
 
   memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
   callbacks.on_frame_recv_callback = on_frame_recv_callback;
@@ -839,6 +842,22 @@ void test_nghttp2_session_on_request_headers_received(void)
   nghttp2_frame_headers_free(&frame.headers);
 
   nghttp2_session_del(session);
+
+  /* Check malformed headers */
+  nghttp2_session_server_new(&session, &callbacks, &user_data);
+  nvlen = nghttp2_nv_array_copy(&nva, malformed_nva, ARRLEN(malformed_nva));
+  nghttp2_frame_headers_init(&frame.headers,
+                             NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_PRIORITY,
+                             1, 1 << 20, nva, nvlen);
+  user_data.frame_recv_cb_called = 0;
+  user_data.invalid_frame_recv_cb_called = 0;
+  CU_ASSERT(0 == nghttp2_session_on_request_headers_received(session, &frame));
+  CU_ASSERT(0 == user_data.frame_recv_cb_called);
+  CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called);
+
+  nghttp2_frame_headers_free(&frame.headers);
+
+  nghttp2_session_del(session);
 }
 
 void test_nghttp2_session_on_response_headers_received(void)
@@ -1137,6 +1156,9 @@ void test_nghttp2_session_on_push_promise_received(void)
   nghttp2_frame frame;
   nghttp2_stream *stream, *promised_stream;
   nghttp2_outbound_item *item;
+  nghttp2_nv malformed_nva[] = { MAKE_NV(":path", "\x01") };
+  nghttp2_nv *nva;
+  size_t nvlen;
 
   memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
   callbacks.send_callback = null_send_callback;
@@ -1255,6 +1277,7 @@ void test_nghttp2_session_on_push_promise_received(void)
   CU_ASSERT(0 == user_data.frame_recv_cb_called);
   CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called);
 
+  nghttp2_frame_push_promise_free(&frame.push_promise);
   nghttp2_session_del(session);
 
   /* Disable PUSH */
@@ -1276,6 +1299,30 @@ void test_nghttp2_session_on_push_promise_received(void)
   CU_ASSERT(0 == user_data.frame_recv_cb_called);
   CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called);
 
+  nghttp2_frame_push_promise_free(&frame.push_promise);
+  nghttp2_session_del(session);
+
+  /* Check malformed headers */
+  nghttp2_session_client_new(&session, &callbacks, &user_data);
+  stream = nghttp2_session_open_stream(session, 1, NGHTTP2_FLAG_NONE,
+                                       NGHTTP2_PRI_DEFAULT,
+                                       NGHTTP2_STREAM_OPENING, NULL);
+  nvlen = nghttp2_nv_array_copy(&nva, malformed_nva, ARRLEN(malformed_nva));
+  nghttp2_frame_push_promise_init(&frame.push_promise,
+                                  NGHTTP2_FLAG_END_PUSH_PROMISE, 1, 2,
+                                  nva, nvlen);
+  user_data.frame_recv_cb_called = 0;
+  user_data.invalid_frame_recv_cb_called = 0;
+  CU_ASSERT(0 == nghttp2_session_on_push_promise_received(session, &frame));
+
+  CU_ASSERT(0 == user_data.frame_recv_cb_called);
+  CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called);
+  item = nghttp2_session_get_next_ob_item(session);
+  CU_ASSERT(NGHTTP2_RST_STREAM == OB_CTRL_TYPE(item));
+  CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == OB_CTRL(item)->rst_stream.error_code);
+  CU_ASSERT(2 == OB_CTRL(item)->hd.stream_id);
+
+  nghttp2_frame_push_promise_free(&frame.push_promise);
   nghttp2_session_del(session);
 }