From abdf3942a848b3de8c4fcdbccf15139b1ed0d9c2 Mon Sep 17 00:00:00 2001 From: Lubos Uhliarik 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 ":[ws]" lines delimited by . * 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