From abdf3942a848b3de8c4fcdbccf15139b1ed0d9c2 Mon Sep 17 00:00:00 2001
From: Lubos Uhliarik <luhliari@redhat.com>
Date: Mon, 3 Aug 2020 16:48:15 +0200
Subject: [PATCH] Fix for CVE-2020-15049
---
src/HttpHeader.cc | 85 ++++++------
src/HttpHeaderTools.cc | 27 ++++
src/HttpHeaderTools.h | 8 +-
src/http/ContentLengthInterpreter.cc | 190 +++++++++++++++++++++++++++
src/http/ContentLengthInterpreter.h | 66 ++++++++++
src/http/Makefile.am | 2 +
src/http/Makefile.in | 4 +-
7 files changed, 337 insertions(+), 45 deletions(-)
create mode 100644 src/http/ContentLengthInterpreter.cc
create mode 100644 src/http/ContentLengthInterpreter.h
diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 7e8c77e..ef60c02 100644
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -11,6 +11,7 @@
#include "squid.h"
#include "base64.h"
#include "globals.h"
+#include "http/ContentLengthInterpreter.h"
#include "HttpHdrCc.h"
#include "HttpHdrContRange.h"
#include "HttpHdrSc.h"
@@ -588,7 +589,6 @@ int
HttpHeader::parse(const char *header_start, const char *header_end)
{
const char *field_ptr = header_start;
- HttpHeaderEntry *e, *e2;
int warnOnError = (Config.onoff.relaxed_header_parser <= 0 ? DBG_IMPORTANT : 2);
PROF_start(HttpHeaderParse);
@@ -605,6 +605,7 @@ HttpHeader::parse(const char *header_start, const char *header_end)
return reset();
}
+ Http::ContentLengthInterpreter clen(warnOnError);
/* common format headers are "<name>:[ws]<value>" lines delimited by <CRLF>.
* continuation lines start with a (single) space or tab */
while (field_ptr < header_end) {
@@ -681,6 +682,7 @@ HttpHeader::parse(const char *header_start, const char *header_end)
break; /* terminating blank line */
}
+ HttpHeaderEntry *e;
if ((e = HttpHeaderEntry::parse(field_start, field_end)) == NULL) {
debugs(55, warnOnError, "WARNING: unparseable HTTP header field {" <<
getStringPrefix(field_start, field_end) << "}");
@@ -693,45 +695,19 @@ HttpHeader::parse(const char *header_start, const char *header_end)
return reset();
}
- // XXX: RFC 7230 Section 3.3.3 item #4 requires sending a 502 error in
- // several cases that we do not yet cover. TODO: Rewrite to cover more.
- if (e->id == HDR_CONTENT_LENGTH && (e2 = findEntry(e->id)) != NULL) {
- if (e->value != e2->value) {
- int64_t l1, l2;
- debugs(55, warnOnError, "WARNING: found two conflicting content-length headers in {" <<
- getStringPrefix(header_start, header_end) << "}");
-
- if (!Config.onoff.relaxed_header_parser) {
- delete e;
- PROF_stop(HttpHeaderParse);
- return reset();
- }
- if (!httpHeaderParseOffset(e->value.termedBuf(), &l1)) {
- debugs(55, DBG_IMPORTANT, "WARNING: Unparseable content-length '" << e->value << "'");
- delete e;
- continue;
- } else if (!httpHeaderParseOffset(e2->value.termedBuf(), &l2)) {
- debugs(55, DBG_IMPORTANT, "WARNING: Unparseable content-length '" << e2->value << "'");
- delById(e2->id);
- } else {
- if (l1 != l2)
- conflictingContentLength_ = true;
- delete e;
- continue;
- }
- } else {
- debugs(55, warnOnError, "NOTICE: found double content-length header");
- delete e;
+ if (e->id == HDR_CONTENT_LENGTH && !clen.checkField(e->value)) {
+ delete e;
- if (Config.onoff.relaxed_header_parser)
- continue;
+ if (Config.onoff.relaxed_header_parser)
+ continue; // clen has printed any necessary warnings
- PROF_stop(HttpHeaderParse);
- return reset();
- }
+ PROF_stop(HttpHeaderParse);
+ clean();
+ return 0;
}
+
if (e->id == HDR_OTHER && stringHasWhitespace(e->name.termedBuf())) {
debugs(55, warnOnError, "WARNING: found whitespace in HTTP header name {" <<
getStringPrefix(field_start, field_end) << "}");
@@ -746,6 +722,32 @@ HttpHeader::parse(const char *header_start, const char *header_end)
addEntry(e);
}
+ if (clen.headerWideProblem) {
+ debugs(55, warnOnError, "WARNING: " << clen.headerWideProblem <<
+ " Content-Length field values in" <<
+ Raw("header", header_start, (size_t)(header_end - header_start)));
+ }
+
+ if (chunked()) {
+ // RFC 2616 section 4.4: ignore Content-Length with Transfer-Encoding
+ // RFC 7230 section 3.3.3 #3: Transfer-Encoding overwrites Content-Length
+ delById(HDR_CONTENT_LENGTH);
+
+ // and clen state becomes irrelevant
+ } else if (clen.sawBad) {
+ // ensure our callers do not accidentally see bad Content-Length values
+ delById(HDR_CONTENT_LENGTH);
+ conflictingContentLength_ = true; // TODO: Rename to badContentLength_.
+ } else if (clen.needsSanitizing) {
+ // RFC 7230 section 3.3.2: MUST either reject or ... [sanitize];
+ // ensure our callers see a clean Content-Length value or none at all
+ delById(HDR_CONTENT_LENGTH);
+ if (clen.sawGood) {
+ putInt64(HDR_CONTENT_LENGTH, clen.value);
+ debugs(55, 5, "sanitized Content-Length to be " << clen.value);
+ }
+ }
+
if (chunked()) {
// RFC 2616 section 4.4: ignore Content-Length with Transfer-Encoding
delById(HDR_CONTENT_LENGTH);
@@ -1722,6 +1724,7 @@ HttpHeaderEntry::getInt() const
assert_eid (id);
assert (Headers[id].type == ftInt);
int val = -1;
+
int ok = httpHeaderParseInt(value.termedBuf(), &val);
httpHeaderNoteParsedEntry(id, value, !ok);
/* XXX: Should we check ok - ie
@@ -1733,15 +1736,11 @@ HttpHeaderEntry::getInt() const
int64_t
HttpHeaderEntry::getInt64() const
{
- assert_eid (id);
- assert (Headers[id].type == ftInt64);
int64_t val = -1;
- int ok = httpHeaderParseOffset(value.termedBuf(), &val);
- httpHeaderNoteParsedEntry(id, value, !ok);
- /* XXX: Should we check ok - ie
- * return ok ? -1 : value;
- */
- return val;
+
+ const bool ok = httpHeaderParseOffset(value.termedBuf(), &val);
+ httpHeaderNoteParsedEntry(id, value, ok);
+ return val; // remains -1 if !ok (XXX: bad method API)
}
static void
diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc
index d8c29d8..02087cd 100644
--- a/src/HttpHeaderTools.cc
+++ b/src/HttpHeaderTools.cc
@@ -188,6 +188,33 @@ httpHeaderParseInt(const char *start, int *value)
return 1;
}
+bool
+httpHeaderParseOffset(const char *start, int64_t *value, char **endPtr)
+{
+ char *end = nullptr;
+ errno = 0;
+
+ const int64_t res = strtoll(start, &end, 10);
+ if (errno && !res) {
+ debugs(66, 7, "failed to parse malformed offset in " << start);
+ return false;
+ }
+ if (errno == ERANGE && (res == LLONG_MIN || res == LLONG_MAX)) { // no overflow
+ debugs(66, 7, "failed to parse huge offset in " << start);
+ return false;
+ }
+ if (start == end) {
+ debugs(66, 7, "failed to parse empty offset");
+ return false;
+ }
+ *value = res;
+ if (endPtr)
+ *endPtr = end;
+ debugs(66, 7, "offset " << start << " parsed as " << res);
+ return true;
+}
+
+
int
httpHeaderParseOffset(const char *start, int64_t * value)
{
diff --git a/src/HttpHeaderTools.h b/src/HttpHeaderTools.h
index 509d940..2d97ad4 100644
--- a/src/HttpHeaderTools.h
+++ b/src/HttpHeaderTools.h
@@ -113,7 +113,13 @@ public:
bool quoted;
};
-int httpHeaderParseOffset(const char *start, int64_t * off);
+/// A strtoll(10) wrapper that checks for strtoll() failures and other problems.
+/// XXX: This function is not fully compatible with some HTTP syntax rules.
+/// Just like strtoll(), allows whitespace prefix, a sign, and _any_ suffix.
+/// Requires at least one digit to be present.
+/// Sets "off" and "end" arguments if and only if no problems were found.
+/// \return true if and only if no problems were found.
+bool httpHeaderParseOffset(const char *start, int64_t *offPtr, char **endPtr = nullptr);
HttpHeaderFieldInfo *httpHeaderBuildFieldsInfo(const HttpHeaderFieldAttrs * attrs, int count);
void httpHeaderDestroyFieldsInfo(HttpHeaderFieldInfo * info, int count);
diff --git a/src/http/ContentLengthInterpreter.cc b/src/http/ContentLengthInterpreter.cc
new file mode 100644
index 0000000..1d40f4a
--- /dev/null
+++ b/src/http/ContentLengthInterpreter.cc
@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+/* DEBUG: section 55 HTTP Header */
+
+#include "squid.h"
+#include "base/CharacterSet.h"
+#include "Debug.h"
+#include "http/ContentLengthInterpreter.h"
+#include "HttpHeaderTools.h"
+#include "SquidConfig.h"
+#include "SquidString.h"
+#include "StrList.h"
+
+Http::ContentLengthInterpreter::ContentLengthInterpreter(const int aDebugLevel):
+ value(-1),
+ headerWideProblem(nullptr),
+ debugLevel(aDebugLevel),
+ sawBad(false),
+ needsSanitizing(false),
+ sawGood(false)
+{
+}
+
+/// characters HTTP permits tolerant parsers to accept as delimiters
+static const CharacterSet &
+RelaxedDelimiterCharacters()
+{
+ // RFC 7230 section 3.5
+ // tolerant parser MAY accept any of SP, HTAB, VT (%x0B), FF (%x0C),
+ // or bare CR as whitespace between request-line fields
+ static const CharacterSet RelaxedDels =
+ (CharacterSet::SP +
+ CharacterSet::HTAB +
+ CharacterSet("VT,FF","\x0B\x0C") +
+ CharacterSet::CR).rename("relaxed-WSP");
+
+ return RelaxedDels;
+}
+
+const CharacterSet &
+Http::ContentLengthInterpreter::WhitespaceCharacters()
+{
+ return Config.onoff.relaxed_header_parser ?
+ RelaxedDelimiterCharacters() : CharacterSet::WSP;
+}
+
+const CharacterSet &
+Http::ContentLengthInterpreter::DelimiterCharacters()
+{
+ return Config.onoff.relaxed_header_parser ?
+ RelaxedDelimiterCharacters() : CharacterSet::SP;
+}
+
+/// 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 = 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
+{
+ // optimize for the common case that does not need delimiters
+ if (suffix == end)
+ return true;
+
+ for (const CharacterSet &delimiters = DelimiterCharacters();
+ suffix < end; ++suffix) {
+ if (!delimiters[*suffix])
+ return false;
+ }
+ // needsSanitizing = true; // TODO: Always remove trailing whitespace?
+ return true; // including empty suffix
+}
+
+/// handles a single-token Content-Length value
+/// rawValue null-termination requirements are those of httpHeaderParseOffset()
+bool
+Http::ContentLengthInterpreter::checkValue(const char *rawValue, const int valueSize)
+{
+ 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;
+
+ if (!httpHeaderParseOffset(digits, &latestValue, &suffix)) {
+ debugs(55, DBG_IMPORTANT, "WARNING: Malformed" << Raw("Content-Length", rawValue, valueSize));
+ sawBad = true;
+ return false;
+ }
+
+ if (latestValue < 0) {
+ debugs(55, debugLevel, "WARNING: Negative" << Raw("Content-Length", rawValue, valueSize));
+ sawBad = true;
+ return false;
+ }
+
+ // check for garbage after the number
+ if (!goodSuffix(suffix, valueEnd)) {
+ debugs(55, debugLevel, "WARNING: Trailing garbage in" << Raw("Content-Length", rawValue, valueSize));
+ sawBad = true;
+ return false;
+ }
+
+ if (sawGood) {
+ /* we have found at least two, possibly identical values */
+
+ needsSanitizing = true; // replace identical values with a single value
+
+ const bool conflicting = value != latestValue;
+ if (conflicting)
+ headerWideProblem = "Conflicting"; // overwrite any lesser problem
+ else if (!headerWideProblem) // preserve a possibly worse problem
+ headerWideProblem = "Duplicate";
+
+ // with relaxed_header_parser, identical values are permitted
+ sawBad = !Config.onoff.relaxed_header_parser || conflicting;
+ return false; // conflicting or duplicate
+ }
+
+ sawGood = true;
+ value = latestValue;
+ return true;
+}
+
+/// handles Content-Length: a, b, c
+bool
+Http::ContentLengthInterpreter::checkList(const String &list)
+{
+ Must(!sawBad);
+
+ if (!Config.onoff.relaxed_header_parser) {
+ debugs(55, debugLevel, "WARNING: List-like" << Raw("Content-Length", list.rawBuf(), list.size()));
+ sawBad = true;
+ return false;
+ }
+
+ needsSanitizing = true; // remove extra commas (at least)
+
+ const char *pos = nullptr;
+ const char *item = nullptr;;
+ int ilen = -1;
+ while (strListGetItem(&list, ',', &item, &ilen, &pos)) {
+ if (!checkValue(item, ilen) && sawBad)
+ break;
+ // keep going after a duplicate value to find conflicting ones
+ }
+ return false; // no need to keep this list field; it will be sanitized away
+}
+
+bool
+Http::ContentLengthInterpreter::checkField(const String &rawValue)
+{
+ if (sawBad)
+ return false; // one rotten apple is enough to spoil all of them
+
+ // TODO: Optimize by always parsing the first integer first.
+ return rawValue.pos(',') ?
+ checkList(rawValue) :
+ checkValue(rawValue.rawBuf(), rawValue.size());
+}
+
diff --git a/src/http/ContentLengthInterpreter.h b/src/http/ContentLengthInterpreter.h
new file mode 100644
index 0000000..ba7080c
--- /dev/null
+++ b/src/http/ContentLengthInterpreter.h
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_HTTP_CONTENTLENGTH_INTERPRETER_H
+#define SQUID_SRC_HTTP_CONTENTLENGTH_INTERPRETER_H
+
+class String;
+
+namespace Http
+{
+
+/// Finds the intended Content-Length value while parsing message-header fields.
+/// Deals with complications such as value lists and/or repeated fields.
+class ContentLengthInterpreter
+{
+public:
+ explicit ContentLengthInterpreter(const int aDebugLevel);
+
+ /// updates history based on the given message-header field
+ /// \return true iff the field should be added/remembered for future use
+ bool checkField(const String &field);
+
+ /// intended Content-Length value if sawGood is set and sawBad is not set
+ /// meaningless otherwise
+ int64_t value;
+
+ /* for debugging (declared here to minimize padding) */
+ const char *headerWideProblem; ///< worst header-wide problem found (or nil)
+ const int debugLevel; ///< debugging level for certain warnings
+
+ /// whether a malformed Content-Length value was present
+ bool sawBad;
+
+ /// whether all remembered fields should be removed
+ /// removed fields ought to be replaced with the intended value (if known)
+ /// irrelevant if sawBad is set
+ bool needsSanitizing;
+
+ /// whether a valid field value was present, possibly among problematic ones
+ /// irrelevant if sawBad is set
+ bool sawGood;
+
+ /// Whitespace between protocol elements in restricted contexts like
+ /// request line, status line, asctime-date, and credentials
+ /// Seen in RFCs as SP but may be "relaxed" by us.
+ /// See also: WhitespaceCharacters().
+ /// XXX: Misnamed and overused.
+ static const CharacterSet &DelimiterCharacters();
+
+ static const CharacterSet &WhitespaceCharacters();
+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);
+};
+
+} // namespace Http
+
+#endif /* SQUID_SRC_HTTP_CONTENTLENGTH_INTERPRETER_H */
+
diff --git a/src/http/Makefile.am b/src/http/Makefile.am
index 7887ef0..78b503e 100644
--- a/src/http/Makefile.am
+++ b/src/http/Makefile.am
@@ -11,6 +11,8 @@ include $(top_srcdir)/src/TestHeaders.am
noinst_LTLIBRARIES = libsquid-http.la
libsquid_http_la_SOURCES = \
+ ContentLengthInterpreter.cc \
+ ContentLengthInterpreter.h \
MethodType.cc \
MethodType.h \
ProtocolVersion.h \
diff --git a/src/http/Makefile.in b/src/http/Makefile.in
index f5b62fb..c7891ae 100644
--- a/src/http/Makefile.in
+++ b/src/http/Makefile.in
@@ -160,7 +160,7 @@ CONFIG_CLEAN_VPATH_FILES =
LTLIBRARIES = $(noinst_LTLIBRARIES)
libsquid_http_la_LIBADD =
am_libsquid_http_la_OBJECTS = MethodType.lo StatusCode.lo \
- StatusLine.lo
+ StatusLine.lo ContentLengthInterpreter.lo
libsquid_http_la_OBJECTS = $(am_libsquid_http_la_OBJECTS)
AM_V_lt = $(am__v_lt_@AM_V@)
am__v_lt_ = $(am__v_lt_@AM_DEFAULT_V@)
@@ -694,6 +694,8 @@ COMPAT_LIB = $(top_builddir)/compat/libcompat-squid.la $(LIBPROFILER)
subst_perlshell = sed -e 's,[@]PERL[@],$(PERL),g' <$(srcdir)/$@.pl.in >$@ || ($(RM) -f $@ ; exit 1)
noinst_LTLIBRARIES = libsquid-http.la
libsquid_http_la_SOURCES = \
+ ContentLengthInterpreter.cc \
+ ContentLengthInterpreter.h \
MethodType.cc \
MethodType.h \
ProtocolVersion.h \
--
2.21.0