[8.1.x] Backport HTTP Validations

.gitignore

@@ -104,6 +104,7 @@ iocore/hostdb/test_RefCountCache
 
 proxy/hdrs/test_mime
 proxy/hdrs/test_Huffmancode
+proxy/hdrs/test_proxy_hdrs
 proxy/hdrs/test_XPACK
 proxy/http/test_ForwardedConfig
 proxy/http2/test_libhttp2

include/tscore/ParseRules.h

@@ -126,7 +126,7 @@ class ParseRules
   static CTypeResult is_empty(char c);            // wslfcr,#
   static CTypeResult is_alnum(char c);            // 0-9,A-Z,a-z
   static CTypeResult is_space(char c);            // ' ' HT,VT,NP,CR,LF
-  static CTypeResult is_control(char c);          // 0-31 127
+  static CTypeResult is_control(char c);          // 0x00-0x08, 0x0a-0x1f, 0x7f
   static CTypeResult is_mime_sep(char c);         // ()<>,;\"/[]?{} \t
   static CTypeResult is_http_field_name(char c);  // not : or mime_sep except for @
   static CTypeResult is_http_field_value(char c); // not CR, LF, comma, or "
@@ -665,14 +665,24 @@ ParseRules::is_space(char c)
 #endif
 }
 
+/**
+   Return true if @c is a control char except HTAB(0x09) and SP(0x20).
+   If you need to check @c is HTAB or SP, use `ParseRules::is_ws`.
+ */
 inline CTypeResult
 ParseRules::is_control(char c)
 {
 #ifndef COMPILE_PARSE_RULES
   return (parseRulesCType[(unsigned char)c] & is_control_BIT);
 #else
-  if (((unsigned char)c) < 32 || ((unsigned char)c) == 127)
+  if (c == CHAR_HT || c == CHAR_SP) {
+    return false;
+  }
+
+  if (((unsigned char)c) < 0x20 || c == 0x7f) {
     return true;
+  }
+
   return false;
 #endif
 }

proxy/hdrs/MIME.cc

@@ -2693,6 +2693,21 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap, MIMEHdrImpl *mh, const char
 
     int field_name_wks_idx = hdrtoken_tokenize(field_name_first, field_name_length);
 
+    if (field_name_wks_idx < 0) {
+      for (int i = 0; i < field_name_length; ++i) {
+        if (ParseRules::is_control(field_name_first[i])) {
+          return PARSE_RESULT_ERROR;
+        }
+      }
+    }
+
+    // RFC 9110 Section 5.5. Field Values
+    for (int i = 0; i < field_value_length; ++i) {
+      if (ParseRules::is_control(field_value_first[i])) {
+        return PARSE_RESULT_ERROR;
+      }
+    }
+
     ///////////////////////////////////////////
     // build and insert the new field object //
     ///////////////////////////////////////////

proxy/hdrs/MIME.h

@@ -164,7 +164,7 @@ struct MIMEField {
   int value_get_comma_list(StrList *list) const;
 
   void name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length);
-  bool name_is_valid() const;
+  bool name_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
 
   void value_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length);
   void value_set_int(HdrHeap *heap, MIMEHdrImpl *mh, int32_t value);
@@ -176,7 +176,7 @@ struct MIMEField {
   // Other separators (e.g. ';' in Set-cookie/Cookie) are also possible
   void value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length, bool prepend_comma = false,
                     const char separator = ',');
-  bool value_is_valid() const;
+  bool value_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
   int has_dups() const;
 };
 
@@ -771,13 +771,13 @@ MIMEField::name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length
   -------------------------------------------------------------------------*/
 
 inline bool
-MIMEField::name_is_valid() const
+MIMEField::name_is_valid(uint32_t invalid_char_bits) const
 {
   const char *name;
   int length;
 
   for (name = name_get(&length); length > 0; length--) {
-    if (ParseRules::is_control(name[length - 1])) {
+    if (ParseRules::is_type(name[length - 1], invalid_char_bits)) {
       return false;
     }
   }
@@ -878,13 +878,13 @@ MIMEField::value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int l
   -------------------------------------------------------------------------*/
 
 inline bool
-MIMEField::value_is_valid() const
+MIMEField::value_is_valid(uint32_t invalid_char_bits) const
 {
   const char *value;
   int length;
 
   for (value = value_get(&length); length > 0; length--) {
-    if (ParseRules::is_control(value[length - 1])) {
+    if (ParseRules::is_type(value[length - 1], invalid_char_bits)) {
       return false;
     }
   }

proxy/hdrs/Makefile.am

@@ -67,12 +67,33 @@ load_http_hdr_LDADD = -L. -lhdrs \
 	@LIBTCL@
 
 check_PROGRAMS = \
+	test_proxy_hdrs \
 	test_mime \
 	test_Huffmancode \
 	test_XPACK
 
 TESTS = $(check_PROGRAMS)
 
+test_proxy_hdrs_CPPFLAGS = $(AM_CPPFLAGS) \
+	-I$(abs_top_srcdir)/tests/include
+
+test_proxy_hdrs_SOURCES = \
+	unit_tests/unit_test_main.cc \
+	unit_tests/test_URL.cc \
+	unit_tests/test_Hdrs.cc
+
+test_proxy_hdrs_LDADD = \
+	$(top_builddir)/src/tscore/libtscore.la \
+	-L. -lhdrs \
+	$(top_builddir)/src/tscore/libtscore.la \
+	$(top_builddir)/src/tscpp/util/libtscpputil.la \
+	$(top_builddir)/iocore/eventsystem/libinkevent.a \
+	$(top_builddir)/lib/records/librecords_p.a \
+	$(top_builddir)/mgmt/libmgmt_p.la \
+	$(top_builddir)/proxy/shared/libUglyLogStubs.a \
+	@HWLOC_LIBS@ \
+	@LIBCAP@
+
 test_mime_LDADD = -L. -lhdrs \
 	$(top_builddir)/src/tscore/libtscore.la \
 	$(top_builddir)/src/tscpp/util/libtscpputil.la \

proxy/hdrs/URL.cc

@@ -114,6 +114,36 @@ validate_host_name(std::string_view addr)
   return std::all_of(addr.begin(), addr.end(), &is_host_char);
 }
 
+/**
+   Checks if the (un-well-known) scheme is valid
+
+   RFC 3986 Section 3.1
+   These are the valid characters in a scheme:
+     scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
+   return an error if there is another character in the scheme
+*/
+bool
+validate_scheme(std::string_view scheme)
+{
+  if (scheme.empty()) {
+    return false;
+  }
+
+  if (!ParseRules::is_alpha(scheme[0])) {
+    return false;
+  }
+
+  for (size_t i = 0; i < scheme.size(); ++i) {
+    const char &c = scheme[i];
+
+    if (!(ParseRules::is_alnum(c) != 0 || c == '+' || c == '-' || c == '.')) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 /*-------------------------------------------------------------------------
   -------------------------------------------------------------------------*/
 
@@ -1140,19 +1170,9 @@ url_parse_scheme(HdrHeap *heap, URLImpl *url, const char **start, const char *en
 
         if (!(scheme_wks_idx > 0 && hdrtoken_wks_to_token_type(scheme_wks) == HDRTOKEN_TYPE_SCHEME)) {
           // Unknown scheme, validate the scheme
-
-          // RFC 3986 Section 3.1
-          // These are the valid characters in a scheme:
-          //   scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
-          // return an error if there is another character in the scheme
-          if (!ParseRules::is_alpha(*scheme_start)) {
+          if (!validate_scheme({scheme_start, static_cast<size_t>(scheme_end - scheme_start)})) {
             return PARSE_RESULT_ERROR;
           }
-          for (cur = scheme_start + 1; cur < scheme_end; ++cur) {
-            if (!(ParseRules::is_alnum(*cur) != 0 || *cur == '+' || *cur == '-' || *cur == '.')) {
-              return PARSE_RESULT_ERROR;
-            }
-          }
         }
         url_scheme_set(heap, url, scheme_start, scheme_wks_idx, scheme_end - scheme_start, copy_strings_p);
       }

proxy/hdrs/URL.h

@@ -156,6 +156,8 @@ void url_adjust(MarshalXlate *str_xlate, int num_xlate);
 
 /* Public */
 bool validate_host_name(std::string_view addr);
+bool validate_scheme(std::string_view scheme);
+
 void url_init();
 
 URLImpl *url_create(HdrHeap *heap);

proxy/hdrs/unit_tests/test_Hdrs.cc

@@ -0,0 +1,80 @@
+/** @file
+
+   Catch-based unit tests for various header logic.
+   This replaces the old regression tests in HdrTest.cc.
+
+   @section license License
+
+   Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements.
+   See the NOTICE file distributed with this work for additional information regarding copyright
+   ownership.  The ASF licenses this file to you under the Apache License, Version 2.0 (the
+   "License"); you may not use this file except in compliance with the License.  You may obtain a
+   copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software distributed under the License
+   is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+   or implied. See the License for the specific language governing permissions and limitations under
+   the License.
+ */
+
+#include <cstdio>
+
+#include "catch.hpp"
+
+#include "MIME.h"
+
+TEST_CASE("HdrTest", "[proxy][hdrtest]")
+{
+  mime_init();
+
+  SECTION("Field Char Check")
+  {
+    static struct {
+      std::string_view line;
+      ParseResult expected;
+    } test_cases[] = {
+      ////
+      // Field Name
+      {"Content-Length: 10\r\n", PARSE_RESULT_CONT},
+      {"Content-Length\x0b: 10\r\n", PARSE_RESULT_ERROR},
+      ////
+      // Field Value
+      // SP
+      {"Content-Length: 10\r\n", PARSE_RESULT_CONT},
+      {"Foo: ab cd\r\n", PARSE_RESULT_CONT},
+      // HTAB
+      {"Foo: ab\td/cd\r\n", PARSE_RESULT_CONT},
+      // VCHAR
+      {"Foo: ab\x21/cd\r\n", PARSE_RESULT_CONT},
+      {"Foo: ab\x7e/cd\r\n", PARSE_RESULT_CONT},
+      // DEL
+      {"Foo: ab\x7f/cd\r\n", PARSE_RESULT_ERROR},
+      // obs-text
+      {"Foo: ab\x80/cd\r\n", PARSE_RESULT_CONT},
+      {"Foo: ab\xff/cd\r\n", PARSE_RESULT_CONT},
+      // control char
+      {"Content-Length: 10\x0b\r\n", PARSE_RESULT_ERROR},
+      {"Content-Length:\x0b 10\r\n", PARSE_RESULT_ERROR},
+      {"Foo: ab\x1d/cd\r\n", PARSE_RESULT_ERROR},
+    };
+
+    MIMEHdr hdr;
+    MIMEParser parser;
+    mime_parser_init(&parser);
+
+    for (const auto &t : test_cases) {
+      mime_parser_clear(&parser);
+
+      const char *start = t.line.data();
+      const char *end   = start + t.line.size();
+
+      int r = hdr.parse(&parser, &start, end, false, false);
+      if (r != t.expected) {
+        std::printf("Expected %s is %s, but not", t.line.data(), t.expected == PARSE_RESULT_ERROR ? "invalid" : "valid");
+        CHECK(false);
+      }
+    }
+  }
+}

proxy/hdrs/unit_tests/test_URL.cc

@@ -0,0 +1,46 @@
+/** @file
+
+   Catch-based unit tests for URL
+
+   @section license License
+
+   Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements.
+   See the NOTICE file distributed with this work for additional information regarding copyright
+   ownership.  The ASF licenses this file to you under the Apache License, Version 2.0 (the
+   "License"); you may not use this file except in compliance with the License.  You may obtain a
+   copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software distributed under the License
+   is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+   or implied. See the License for the specific language governing permissions and limitations under
+   the License.
+ */
+
+#include <cstdio>
+
+#include "catch.hpp"
+
+#include "URL.h"
+
+TEST_CASE("Validate Scheme", "[proxy][validscheme]")
+{
+  static const struct {
+    std::string_view text;
+    bool valid;
+  } scheme_test_cases[] = {{"http", true},      {"https", true},      {"example", true},    {"example.", true},
+                           {"example++", true}, {"example--.", true}, {"++example", false}, {"--example", false},
+                           {".example", false}, {"example://", false}};
+
+  for (auto i : scheme_test_cases) {
+    // it's pretty hard to debug with
+    //     CHECK(validate_scheme(i.text) == i.valid);
+
+    std::string_view text = i.text;
+    if (validate_scheme(text) != i.valid) {
+      std::printf("Validation of scheme: \"%s\", expected %s, but not\n", text.data(), (i.valid ? "true" : "false"));
+      CHECK(false);
+    }
+  }
+}

proxy/hdrs/unit_tests/unit_test_main.cc

@@ -0,0 +1,44 @@
+/** @file
+
+  This file used for catch based tests. It is the main() stub.
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#include "HTTP.h"
+
+#define CATCH_CONFIG_RUNNER
+#include "catch.hpp"
+
+extern int cmd_disable_pfreelist;
+
+int
+main(int argc, char *argv[])
+{
+  // No thread setup, forbid use of thread local allocators.
+  cmd_disable_pfreelist = true;
+  // Get all of the HTTP WKS items populated.
+  http_init();
+
+  int result = Catch::Session().run(argc, argv);
+
+  // global clean-up...
+
+  return result;
+}