Validate Content-Length value prefix (#629)

The new code detects all invalid Content-Length prefixes but the old
code was already rejecting most invalid prefixes using strtoll(). The
newly covered (and now rejected) invalid characters are

* explicit "+" sign;
* explicit "-" sign in "-0" values;
* isspace(3) characters that are not (relaxed) OWS characters.

In most deployment environments, the last set is probably empty because
the relaxed OWS set has all the POSIX/C isspace(3) characters but the
new line, and the new line is unlikely to sneak in past other checks.

Thank you, Amit Klein <amit.klein@safebreach.com>, for elevating the
importance of this 2016 TODO (added in commit a1b9ec2).

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 36957f2e11f25006c078872f1632ddac97d1d893..c10a22154363d90d90b3fd18431fb632c05eada6 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -25,6 +25,7 @@ Thank you!
     Alex Wu <alex_wu2012@hotmail.com>
     Alin Nastac <mrness@gentoo.org>
     Alter <alter@alter.org.ua>
+    Amit Klein <amit.klein@safebreach.com>
     Amos Jeffries
     Amos Jeffries <amosjeffries@squid-cache.org>
     Amos Jeffries <squid3@treenet.co.nz>

diff --git a/src/http/ContentLengthInterpreter.cc b/src/http/ContentLengthInterpreter.cc
index 58e14e654b59ec1cd9d702047d6c04c439e14bd9..b3c077d63d1200756062c1b88703c601b0cc0766 100644 (file)
--- a/src/http/ContentLengthInterpreter.cc
+++ b/src/http/ContentLengthInterpreter.cc
@@ -29,6 +29,24 @@ Http::ContentLengthInterpreter::ContentLengthInterpreter():
 {
 }
 
+/// checks whether all characters before the Content-Length number are allowed
+/// \returns the start of the digit sequence (or nil on errors)
+const char *
+Http::ContentLengthInterpreter::findDigits(const char *prefix, const char * const valueEnd) const
+{
+    // skip leading OWS in RFC 7230's `OWS field-value OWS`
+    const CharacterSet &whitespace = Http::One::Parser::WhitespaceCharacters();
+    while (prefix < valueEnd) {
+        const auto ch = *prefix;
+        if (CharacterSet::DIGIT[ch])
+            return prefix; // common case: a pre-trimmed field value
+        if (!whitespace[ch])
+            return nullptr; // (trimmed) length does not start with a digit
+        ++prefix;
+    }
+    return nullptr; // empty or whitespace-only value
+}
+
 /// checks whether all characters after the Content-Length are allowed
 bool
 Http::ContentLengthInterpreter::goodSuffix(const char *suffix, const char * const end) const
@@ -53,10 +71,19 @@ Http::ContentLengthInterpreter::checkValue(const char *rawValue, const int value
 {
     Must(!sawBad);
 
+    const auto valueEnd = rawValue + valueSize;
+
+    const auto digits = findDigits(rawValue, valueEnd);
+    if (!digits) {
+        debugs(55, debugLevel, "WARNING: Leading garbage or empty value in" << Raw("Content-Length", rawValue, valueSize));
+        sawBad = true;
+        return false;
+    }
+
     int64_t latestValue = -1;
     char *suffix = nullptr;
-    // TODO: Handle malformed values with leading signs (e.g., "-0" or "+1").
-    if (!httpHeaderParseOffset(rawValue, &latestValue, &suffix)) {
+
+    if (!httpHeaderParseOffset(digits, &latestValue, &suffix)) {
         debugs(55, DBG_IMPORTANT, "WARNING: Malformed" << Raw("Content-Length", rawValue, valueSize));
         sawBad = true;
         return false;
@@ -69,7 +96,7 @@ Http::ContentLengthInterpreter::checkValue(const char *rawValue, const int value
     }
 
     // check for garbage after the number
-    if (!goodSuffix(suffix, rawValue + valueSize)) {
+    if (!goodSuffix(suffix, valueEnd)) {
         debugs(55, debugLevel, "WARNING: Trailing garbage in" << Raw("Content-Length", rawValue, valueSize));
         sawBad = true;
         return false;

diff --git a/src/http/ContentLengthInterpreter.h b/src/http/ContentLengthInterpreter.h
index 37d9e696cf2f72a0247e950b6f41fe5ab437e8ee..142664e04146d3d4d237614f219f3d8f462dcc66 100644 (file)
--- a/src/http/ContentLengthInterpreter.h
+++ b/src/http/ContentLengthInterpreter.h
@@ -67,6 +67,7 @@ public:
     bool sawGood;
 
 protected:
+    const char *findDigits(const char *prefix, const char *valueEnd) const;
     bool goodSuffix(const char *suffix, const char * const end) const;
     bool checkValue(const char *start, const int size);
     bool checkList(const String &list);