Skip to content

Commit

Permalink
[1.4] Fix CVE-2020-25017 (#46) (#272)
Browse files Browse the repository at this point in the history
* doc: version 1.12.7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix wasm compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix typo

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Co-authored-by: Yuchen Dai <silentdai@gmail.com>
  • Loading branch information
brian-avery and lambdai authored Oct 8, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 8e9db51 commit e43f60f
Showing 47 changed files with 682 additions and 127 deletions.
4 changes: 4 additions & 0 deletions bazel/external/quiche.BUILD
Original file line number Diff line number Diff line change
@@ -59,6 +59,10 @@ quiche_copts = select({
"-Wno-unused-parameter",
"-Wno-unused-function",
"-Wno-type-limits",
"-Wno-unused-function",
"-Wno-unknown-warning-option",
"-Wno-deprecated-copy",
"-Wno-range-loop-construct",
# quic_inlined_frame.h uses offsetof() to optimize memory usage in frames.
"-Wno-invalid-offsetof",
"-Wno-type-limits",
29 changes: 18 additions & 11 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
Version history
---------------

1.12.3 (Pending)
==========================
* buffer: force copy when appending small slices to OwnedImpl buffer to avoid fragmentation.
* listeners: fixed issue where :ref:`TLS inspector listener filter <config_listener_filters_tls_inspector>` could have been bypassed by a client using only TLS 1.3.
* sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases.
* http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`.
1.12.5 (Pending)
1.12.7 (pending)
================
* http: the :ref:`stream_idle_timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
now also defends against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client.
* listener: add runtime support for `per-listener limits <config_listeners_runtime>` on active/accepted connections.
* overload management: add runtime support for :ref:`global limits <config_overload_manager>` on active/accepted connections.
* http: fixed CVE-2020-25017. Previously header matching did not match on all headers for non-inline headers. This patch
changes the default behavior to always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what is done with inline headers. This
makes the behavior effectively consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false".

Targeted fixes have been additionally performed on the following extensions which make them
consider all duplicate headers by default as a comma concatenated list:

1. Any extension using CEL matching on headers.
2. The header to metadata filter.
3. The JWT filter.
4. The Lua filter.

Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false.

1.12.6 (July 7, 2020)
=====================
* tls: fixed a bug where wilcard matching for "\*.foo.com" also matched domains of the form "a.b.foo.com". This behavior can be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_wildcard_matching` to false.
3 changes: 3 additions & 0 deletions include/envoy/http/BUILD
Original file line number Diff line number Diff line change
@@ -86,6 +86,9 @@ envoy_cc_library(
envoy_cc_library(
name = "header_map_interface",
hdrs = ["header_map.h"],
external_deps = [
"abseil_inlined_vector",
],
deps = [
"//source/common/common:assert_lib",
"//source/common/common:hash_lib",
32 changes: 32 additions & 0 deletions include/envoy/http/header_map.h
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@
#include "common/common/hash.h"
#include "common/common/macros.h"

#include "absl/container/inlined_vector.h"
#include "absl/strings/string_view.h"

namespace Envoy {
@@ -129,6 +130,12 @@ class HeaderString {
*/
char* buffer() { return buffer_.dynamic_; }

/**
* Trim trailing whitespaces from the HeaderString.
* v1.12 supports both Inline and Dynamic, but not Reference type.
*/
void rtrim();

/**
* Get an absl::string_view. It will NOT be NUL terminated!
*
@@ -501,6 +508,31 @@ class HeaderMap {
virtual const HeaderEntry* get(const LowerCaseString& key) const PURE;
virtual HeaderEntry* get(const LowerCaseString& key) PURE;

/**
* This is a wrapper for the return result from getAll(). It avoids a copy when translating from
* non-const HeaderEntry to const HeaderEntry and only provides const access to the result.
*/
using NonConstGetResult = absl::InlinedVector<HeaderEntry*, 1>;
class GetResult {
public:
GetResult() = default;
explicit GetResult(NonConstGetResult&& result) : result_(std::move(result)) {}

bool empty() const { return result_.empty(); }
size_t size() const { return result_.size(); }
const HeaderEntry* operator[](size_t i) const { return result_[i]; }

private:
NonConstGetResult result_;
};

/**
* Get a header by key.
* @param key supplies the header key.
* @return all header entries matching the key.
*/
virtual GetResult getAll(const LowerCaseString& key) const PURE;

// aliases to make iterate() and iterateReverse() callbacks easier to read
enum class Iterate { Continue, Break };

1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
@@ -236,6 +236,7 @@ envoy_cc_library(
"//source/common/common:empty_string",
"//source/common/common:non_copyable",
"//source/common/common:utility_lib",
"//source/common/runtime:runtime_features_lib",
"//source/common/singleton:const_singleton",
],
)
6 changes: 3 additions & 3 deletions source/common/http/async_client_impl.h
Original file line number Diff line number Diff line change
@@ -144,9 +144,9 @@ class AsyncStreamImpl : public AsyncClient::Stream,
}
absl::optional<std::chrono::milliseconds> maxInterval() const override { return absl::nullopt; }

const std::vector<uint32_t> retriable_status_codes_;
const std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_;
const std::vector<Http::HeaderMatcherSharedPtr> retriable_request_headers_;
const std::vector<uint32_t> retriable_status_codes_{};
const std::vector<Http::HeaderMatcherSharedPtr> retriable_headers_{};
const std::vector<Http::HeaderMatcherSharedPtr> retriable_request_headers_{};
};

struct NullShadowPolicy : public Router::ShadowPolicy {
50 changes: 43 additions & 7 deletions source/common/http/header_map_impl.cc
Original file line number Diff line number Diff line change
@@ -150,6 +150,15 @@ void HeaderString::append(const char* data, uint32_t size) {
string_length_ += size;
}

void HeaderString::rtrim() {
ASSERT(type() == Type::Inline || type() == Type::Dynamic);
absl::string_view original = getStringView();
absl::string_view rtrimmed = StringUtil::rtrim(original);
if (original.size() != rtrimmed.size()) {
string_length_ = rtrimmed.size();
}
}

void HeaderString::clear() {
switch (type_) {
case Type::Reference: {
@@ -496,13 +505,12 @@ uint64_t HeaderMapImpl::byteSizeInternal() const {
}

const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const {
for (const HeaderEntryImpl& header : headers_) {
if (header.key() == key.get().c_str()) {
return &header;
}
}
const auto result = getAll(key);
return result.empty() ? nullptr : result[0];
}

return nullptr;
HeaderMap::GetResult HeaderMapImpl::getAll(const LowerCaseString& key) const {
return HeaderMap::GetResult(const_cast<HeaderMapImpl*>(this)->getExisting(key));
}

HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) {
@@ -512,10 +520,38 @@ HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) {
return &header;
}
}

return nullptr;
}

HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& key) {
// Attempt a trie lookup first to see if the user is requesting an O(1) header. This may be
// relatively common in certain header matching / routing patterns.
// TODO(mattklein123): Add inline handle support directly to the header matcher code to support
// this use case more directly.
HeaderMap::NonConstGetResult ret;

EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get());
if (cb) {
StaticLookupResponse ref_lookup_response = cb(*this);
if (*ref_lookup_response.entry_) {
ret.push_back(*ref_lookup_response.entry_);
}
return ret;
}
// If the requested header is not an O(1) header we do a full scan. Doing the trie lookup is
// wasteful in the miss case, but is present for code consistency with other functions that do
// similar things.
// TODO(mattklein123): The full scan here and in remove() are the biggest issues with this
// implementation for certain use cases. We can either replace this with a totally different
// implementation or potentially create a lazy map if the size of the map is above a threshold.
for (HeaderEntryImpl& header : headers_) {
if (header.key() == key.get().c_str()) {
ret.push_back(&header);
}
}
return ret;
}

void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const {
for (const HeaderEntryImpl& header : headers_) {
if (cb(header, context) == HeaderMap::Iterate::Break) {
2 changes: 2 additions & 0 deletions source/common/http/header_map_impl.h
Original file line number Diff line number Diff line change
@@ -85,6 +85,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
uint64_t byteSizeInternal() const override;
const HeaderEntry* get(const LowerCaseString& key) const override;
HeaderEntry* get(const LowerCaseString& key) override;
HeaderMap::GetResult getAll(const LowerCaseString& key) const override;
void iterate(ConstIterateCb cb, void* context) const override;
void iterateReverse(ConstIterateCb cb, void* context) const override;
Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override;
@@ -203,6 +204,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key);
HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
HeaderString&& value);
HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key);
HeaderEntryImpl* getExistingInline(absl::string_view key);

void removeInline(HeaderEntryImpl** entry);
51 changes: 41 additions & 10 deletions source/common/http/header_utility.cc
Original file line number Diff line number Diff line change
@@ -4,8 +4,10 @@
#include "common/common/utility.h"
#include "common/http/header_map_impl.h"
#include "common/protobuf/utility.h"
#include "common/runtime/runtime_features.h"

#include "absl/strings/match.h"
#include "absl/strings/str_join.h"
#include "nghttp2/nghttp2.h"

namespace Envoy {
@@ -92,36 +94,65 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers,
return true;
}

HeaderUtility::GetAllOfHeaderAsStringResult
HeaderUtility::getAllOfHeaderAsString(const HeaderMap& headers, const Http::LowerCaseString& key) {
GetAllOfHeaderAsStringResult result;
const auto header_value = headers.getAll(key);

if (header_value.empty()) {
// Empty for clarity. Avoid handling the empty case in the block below if the runtime feature
// is disabled.
} else if (header_value.size() == 1 ||
!Runtime::runtimeFeatureEnabled(
"envoy.reloadable_features.http_match_on_all_headers")) {
result.result_ = header_value[0]->value().getStringView();
} else {
// In this case we concatenate all found headers using a ',' delimiter before performing the
// final match. We use an InlinedVector of absl::string_view to invoke the optimized join
// algorithm. This requires a copying phase before we invoke join. The 3 used as the inline
// size has been arbitrarily chosen.
// TODO(mattklein123): Do we need to normalize any whitespace here?
absl::InlinedVector<absl::string_view, 3> string_view_vector;
string_view_vector.reserve(header_value.size());
for (size_t i = 0; i < header_value.size(); i++) {
string_view_vector.push_back(header_value[i]->value().getStringView());
}
result.result_backing_string_ = absl::StrJoin(string_view_vector, ",");
}

return result;
}

bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderData& header_data) {
const HeaderEntry* header = request_headers.get(header_data.name_);
const auto header_value = getAllOfHeaderAsString(request_headers, header_data.name_);

if (header == nullptr) {
if (!header_value.result().has_value()) {
return header_data.invert_match_ && header_data.header_match_type_ == HeaderMatchType::Present;
}

bool match;
const absl::string_view header_view = header->value().getStringView();
switch (header_data.header_match_type_) {
case HeaderMatchType::Value:
match = header_data.value_.empty() || header_view == header_data.value_;
match = header_data.value_.empty() || header_value.result().value() == header_data.value_;
break;
case HeaderMatchType::Regex:
match = header_data.regex_->match(header_view);
match = header_data.regex_->match(header_value.result().value());
break;
case HeaderMatchType::Range: {
int64_t header_value = 0;
match = absl::SimpleAtoi(header_view, &header_value) &&
header_value >= header_data.range_.start() && header_value < header_data.range_.end();
int64_t header_int_value = 0;
match = absl::SimpleAtoi(header_value.result().value(), &header_int_value) &&
header_int_value >= header_data.range_.start() &&
header_int_value < header_data.range_.end();
break;
}
case HeaderMatchType::Present:
match = true;
break;
case HeaderMatchType::Prefix:
match = absl::StartsWith(header_view, header_data.value_);
match = absl::StartsWith(header_value.result().value(), header_data.value_);
break;
case HeaderMatchType::Suffix:
match = absl::EndsWith(header_view, header_data.value_);
match = absl::EndsWith(header_value.result().value(), header_data.value_);
break;
default:
NOT_REACHED_GCOVR_EXCL_LINE;
29 changes: 29 additions & 0 deletions source/common/http/header_utility.h
Original file line number Diff line number Diff line change
@@ -32,6 +32,35 @@ class HeaderUtility {
static void getAllOfHeader(const HeaderMap& headers, absl::string_view key,
std::vector<absl::string_view>& out);

/**
* Get all header values as a single string. Multiple headers are concatenated with ','.
*/
class GetAllOfHeaderAsStringResult {
public:
// The ultimate result of the concatenation. If absl::nullopt, no header values were found.
// If the final string required a string allocation, the memory is held in
// backingString(). This allows zero allocation in the common case of a single header
// value.
absl::optional<absl::string_view> result() const {
// This is safe for move/copy of this class as the backing string will be moved or copied.
// Otherwise result_ is valid. The assert verifies that both are empty or only 1 is set.
ASSERT((!result_.has_value() && result_backing_string_.empty()) ||
(result_.has_value() ^ !result_backing_string_.empty()));
return !result_backing_string_.empty() ? result_backing_string_ : result_;
}

const std::string& backingString() const { return result_backing_string_; }

private:
absl::optional<absl::string_view> result_;
// Valid only if result_ relies on memory allocation that must live beyond the call. See above.
std::string result_backing_string_;

friend class HeaderUtility;
};
static GetAllOfHeaderAsStringResult getAllOfHeaderAsString(const HeaderMap& headers,
const Http::LowerCaseString& key);

// A HeaderData specifies one of exact value or regex or range element
// to match in a request's header, specified in the header_match_type_ member.
// It is the runtime equivalent of the HeaderMatchSpecifier proto in RDS API.
16 changes: 13 additions & 3 deletions source/common/http/http1/codec_impl.cc
Original file line number Diff line number Diff line change
@@ -408,6 +408,11 @@ void ConnectionImpl::completeLastHeader() {
checkHeaderNameForUnderscores();
if (!current_header_field_.empty()) {
toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size());
// Strip trailing whitespace of the current header value if any. Leading whitespace was trimmed
// in ConnectionImpl::onHeaderValue. http_parser does not strip leading or trailing whitespace
// as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4
current_header_value_.rtrim();

current_header_map_->addViaMove(std::move(current_header_field_),
std::move(current_header_value_));
}
@@ -518,9 +523,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
return;
}

// Work around a bug in http_parser where trailing whitespace is not trimmed
// as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4
const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length));
absl::string_view header_value{data, length};

if (strict_header_validation_) {
if (!Http::HeaderUtility::headerValueIsValid(header_value)) {
@@ -538,6 +541,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
}

header_parsing_state_ = HeaderParsingState::Value;
if (current_header_value_.empty()) {
// Strip leading whitespace if the current header value input contains the first bytes of the
// encoded header value. Trailing whitespace is stripped once the full header value is known in
// ConnectionImpl::completeLastHeader. http_parser does not strip leading or trailing whitespace
// as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 .
header_value = StringUtil::ltrim(header_value);
}
current_header_value_.append(header_value.data(), header_value.length());

checkMaxHeadersSize();
Loading

0 comments on commit e43f60f

Please sign in to comment.