Ignore regular headers if it includes illegal characters.

This commit only affects the library behaviour unless
nghttp2_option_set_no_http_messaging() is used.

We like strict validation against header field name and value against
RFC 7230, but we have already so much web sites and libraries in
public internet which do not obey these rules.  Simply just
terminating stream because of this may break web sites and it is too
disruptive.  So we decided that we should be conservative here so
those header fields containing illegal characters are just ignored.
But we are conservative only for regular headers.  We are strict for
pseudo headers since it is new to HTTP/2 and new implementations
should know the rules better.

lib/nghttp2_http.c

@@ -384,8 +384,22 @@ int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream,
                            nghttp2_frame *frame, nghttp2_nv *nv, int trailer) {
   if (!nghttp2_check_header_name(nv->name, nv->namelen) ||
       !nghttp2_check_header_value(nv->value, nv->valuelen)) {
+    /* We are strict for pseudo header field.  One bad character
+       should lead to fail.  OTOH, we should be a bit forgiving for
+       regular headers, since existing public internet has so much
+       illegal headers floating around and if we kill the stream
+       because of this, we may disrupt many web sites and/or
+       libraries.  So we become conservative here, and just ignore
+       those illegal regular headers. */
+    if (nv->namelen > 0 && nv->name[0] == ':') {
       return -1;
     }
+    /* When ignoring regular headers, we set this flag so that we
+       still enforce header field ordering rule for pseudo header
+       fields. */
+    stream->http_flags |= NGHTTP2_HTTP_FLAG_PSEUDO_HEADER_DISALLOWED;
+    return 1;
+  }
 
   if (session->server || frame->hd.type == NGHTTP2_PUSH_PROMISE) {
     return http_request_on_header(stream, nv, trailer);

lib/nghttp2_http.h

@@ -37,7 +37,9 @@
  * This function is called when HTTP header field |nv| in |frame| is
  * received for |stream|.  This function will validate |nv| against
  * the current state of stream.  This function returns 0 if it
- * succeeds, or -1.
+ * succeeds, or 1 if the header field should be ignored, or -1 if the
+ * header field contains totally unforgivable piece of junk and stream
+ * should be killed.
  */
 int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream,
                            nghttp2_frame *frame, nghttp2_nv *nv, int trailer);

lib/nghttp2_session.c

@@ -3166,10 +3166,11 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
     DEBUGF(fprintf(stderr, "recv: proclen=%zd\n", proclen));
 
     if (call_header_cb && (inflate_flags & NGHTTP2_HD_INFLATE_EMIT)) {
+      rv = 0;
       if (subject_stream && session_enforce_http_messaging(session)) {
         rv = nghttp2_http_on_header(session, subject_stream, frame, &nv,
                                     trailer);
-        if (rv != 0) {
+        if (rv == -1) {
           DEBUGF(fprintf(
               stderr, "recv: HTTP error: type=%d, id=%d, header %.*s: %.*s\n",
               frame->hd.type, subject_stream->stream_id, (int)nv.namelen,
@@ -3180,9 +3181,15 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
             return rv;
           }
           return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
+        } else if (rv == 1) {
+          /* header is ignored */
+          DEBUGF(fprintf(
+              stderr, "recv: HTTP ignored: type=%d, id=%d, header %.*s: %.*s\n",
+              frame->hd.type, subject_stream->stream_id, (int)nv.namelen,
+              nv.name, (int)nv.valuelen, nv.value));
         }
       }
-      if (call_header_cb) {
+      if (rv == 0 && call_header_cb) {
         rv = session_call_on_header(session, frame, &nv);
         /* This handles NGHTTP2_ERR_PAUSE and
            NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */

tests/main.c

@@ -269,6 +269,8 @@ int main(int argc _U_, char *argv[] _U_) {
                    test_nghttp2_http_non_final_response) ||
       !CU_add_test(pSuite, "http_trailer_headers",
                    test_nghttp2_http_trailer_headers) ||
+      !CU_add_test(pSuite, "http_ignore_regular_header",
+                   test_nghttp2_http_ignore_regular_header) ||
       !CU_add_test(pSuite, "http_ignore_content_length",
                    test_nghttp2_http_ignore_content_length) ||
       !CU_add_test(pSuite, "http_record_request_method",

tests/nghttp2_session_test.c

@@ -6895,6 +6895,13 @@ void test_nghttp2_http_mandatory_headers(void) {
       MAKE_NV(":scheme", "https"), MAKE_NV(":method", "GET"),
       MAKE_NV(":authority", "localhost"), MAKE_NV(":path", "/"),
       MAKE_NV("connection", "close")};
+  const nghttp2_nv badauthority_reqnv[] = {
+      MAKE_NV(":scheme", "https"), MAKE_NV(":method", "GET"),
+      MAKE_NV(":authority", "\x0d\x0alocalhost"), MAKE_NV(":path", "/")};
+  const nghttp2_nv badhdbtw_reqnv[] = {
+      MAKE_NV(":scheme", "https"), MAKE_NV(":method", "GET"),
+      MAKE_NV("foo", "\x0d\x0a"), MAKE_NV(":authority", "localhost"),
+      MAKE_NV(":path", "/")};
 
   mem = nghttp2_mem_default();
 
@@ -6983,6 +6990,17 @@ void test_nghttp2_http_mandatory_headers(void) {
   check_nghttp2_http_recv_headers_fail(session, &deflater, 13, -1, badhd_reqnv,
                                        ARRLEN(badhd_reqnv));
 
+  /* request header has :authority header field containing illegal
+     characters */
+  check_nghttp2_http_recv_headers_fail(session, &deflater, 15, -1,
+                                       badauthority_reqnv,
+                                       ARRLEN(badauthority_reqnv));
+
+  /* request header has regular header field containing illegal
+     character before all mandatory header fields are seen. */
+  check_nghttp2_http_recv_headers_fail(session, &deflater, 17, -1,
+                                       badhdbtw_reqnv, ARRLEN(badhdbtw_reqnv));
+
   nghttp2_hd_deflate_free(&deflater);
 
   nghttp2_session_del(session);
@@ -7417,6 +7435,66 @@ void test_nghttp2_http_trailer_headers(void) {
   nghttp2_bufs_free(&bufs);
 }
 
+void test_nghttp2_http_ignore_regular_header(void) {
+  nghttp2_session *session;
+  nghttp2_session_callbacks callbacks;
+  nghttp2_hd_deflater deflater;
+  nghttp2_mem *mem;
+  nghttp2_bufs bufs;
+  ssize_t rv;
+  my_user_data ud;
+  const nghttp2_nv bad_reqnv[] = {
+      MAKE_NV(":authority", "localhost"), MAKE_NV(":scheme", "https"),
+      MAKE_NV(":path", "/"),              MAKE_NV(":method", "GET"),
+      MAKE_NV("foo", "\x0zzz"),           MAKE_NV("bar", "buzz"),
+  };
+  const nghttp2_nv bad_ansnv[] = {
+      MAKE_NV(":authority", "localhost"), MAKE_NV(":scheme", "https"),
+      MAKE_NV(":path", "/"), MAKE_NV(":method", "GET"), MAKE_NV("bar", "buzz")};
+  ssize_t proclen;
+  size_t i;
+
+  mem = nghttp2_mem_default();
+  frame_pack_bufs_init(&bufs);
+
+  memset(&callbacks, 0, sizeof(nghttp2_session_callbacks));
+  callbacks.send_callback = null_send_callback;
+  callbacks.on_header_callback = pause_on_header_callback;
+
+  nghttp2_session_server_new(&session, &callbacks, &ud);
+  nghttp2_hd_deflate_init(&deflater, mem);
+
+  rv = pack_headers(&bufs, &deflater, 1,
+                    NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_END_STREAM,
+                    bad_reqnv, ARRLEN(bad_reqnv), mem);
+
+  CU_ASSERT_FATAL(0 == rv);
+
+  proclen = 0;
+
+  for (i = 0; i < 4; ++i) {
+    rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos + proclen,
+                                  nghttp2_buf_len(&bufs.head->buf) - proclen);
+    CU_ASSERT_FATAL(rv > 0);
+    proclen += rv;
+    CU_ASSERT(nghttp2_nv_equal(&bad_ansnv[i], &ud.nv));
+  }
+
+  rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos + proclen,
+                                nghttp2_buf_len(&bufs.head->buf) - proclen);
+  CU_ASSERT_FATAL(rv > 0);
+  /* header field "foo" must be ignored because it has illegal value.
+     So we have "bar" header field for 5th header. */
+  CU_ASSERT(nghttp2_nv_equal(&bad_ansnv[4], &ud.nv));
+  proclen += rv;
+
+  CU_ASSERT(nghttp2_buf_len(&bufs.head->buf) == proclen);
+
+  nghttp2_hd_deflate_free(&deflater);
+  nghttp2_session_del(session);
+  nghttp2_bufs_free(&bufs);
+}
+
 void test_nghttp2_http_ignore_content_length(void) {
   nghttp2_session *session;
   nghttp2_session_callbacks callbacks;

tests/nghttp2_session_test.h

@@ -127,6 +127,7 @@ void test_nghttp2_http_content_length(void);
 void test_nghttp2_http_content_length_mismatch(void);
 void test_nghttp2_http_non_final_response(void);
 void test_nghttp2_http_trailer_headers(void);
+void test_nghttp2_http_ignore_regular_header(void);
 void test_nghttp2_http_ignore_content_length(void);
 void test_nghttp2_http_record_request_method(void);
 void test_nghttp2_http_push_promise(void);