Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AppSec crash when parsing integer http headers #3790

Merged
merged 4 commits into from
Jul 24, 2024
+31 −1
Merged

Fix AppSec crash when parsing integer http headers #3790

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
50a7ef9
Fix AppSec crash when parsing integer http headers
vpellan Jul 17, 2024
86b882a
Fix header collection for case insensitive duplicates
vpellan Jul 19, 2024
fa699c8
Applied performance related suggestions
vpellan Jul 19, 2024
2701a76
Revert to Rack compliant behaviour
vpellan Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/datadog/appsec/contrib/rack/gateway/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,14 @@ def method

def headers
result = request.env.each_with_object({}) do |(k, v), h|
h[k.gsub(/^HTTP_/, '').downcase!.tr('_', '-')] = v if k =~ /^HTTP_/
# When multiple headers with the same name are present, they are concatenated with a comma
# https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
# Because headers are case insensitive, HTTP_FOO and HTTP_Foo is the same, and should be merged
Copy link
Member

@lloeki lloeki Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When could this happen?

  • The Rack spec means upcasing HTTP headers into the HTTP_UPCASED_HEADER key format.
  • Given that request.env.each_with_object iterates on the rack env (which is a hash) there can be no duplicates except for mixed case, which cannot happen when Rack compliant due to the above.
  • The Rack env does not concatenate multiple identical headers into a single hash key, instead making use of a single one (can't recall if first or last).

It follows that the only thing that could fill in some HTTP_MiXeD_CaSe is some non-Rack compliant middleware or monkeypatch that inject pseudo headers that actually don't exist on the HTTP request AND would execute before Datadog's middlewares, which are designed to be run first at the top of the Rack stack.

According to the HTTP spec concatenation MAY happen but MUST only happen for headers that are lists. As is this code does so for ALL headers so it's very much incorrect, and possibly prone to injections or opportunities for a WAF bypass (e.g Authorization headers being concatenated in a way that make it valid for libddwaf rules but would in isolation - as seen by the application - be dangerous)

I'm OK with making code robust in face of strange stuff, but trying to make sense of hypothetical non-compliant nonsense can only lead to problems (GIGO), therefore I question the cost of bearing the handling of these hypothetical use cases on every request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is an extremely rare edge case, that would be introduced by some sort of weird middleware, and I believe that we should not be responsible for a client modifying the core behaviour of Rack, especially when it is only hypothetical. And a quick benchmark shows that this is 50x slower than your first suggestion with a bit of modification (replacing gsub by delete_prefix and regex in condition by start_with), which I believe is more important for our clients.

next unless k.start_with?('HTTP_')

key = k.delete_prefix('HTTP_').tap(&:downcase!).tap { |s| s.tr!('_', '-') }
current_val = h[key]
h[key] = current_val.nil? ? v : "#{current_val}, #{v}"
Copy link
Member

@lloeki lloeki Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work to transform an individual string for the single header case into an array, delegating the meaning of having multiple identical HTTP headers to whatever is downstream of the Reactive Engine.

Suggested change
h[key] = current_val.nil? ? v : "#{current_val}, #{v}"
h[key] = current_val.nil? ? v : [current_val, v]

end

result['content-type'] = request.content_type if request.content_type
Expand Down
32 changes: 32 additions & 0 deletions spec/datadog/appsec/contrib/rack/gateway/request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,38 @@
}
expect(request.headers).to eq(expected_headers)
end

context 'with malformed headers' do
let(:request) do
described_class.new(
Rack::MockRequest.env_for(
'http://example.com:8080/?a=foo&a=bar&b=baz',
{
'REQUEST_METHOD' => 'GET', 'REMOTE_ADDR' => '10.10.10.10', 'CONTENT_TYPE' => 'text/html',
'HTTP_COOKIE' => 'foo=bar', 'HTTP_USER_AGENT' => 'WebKit',
'HTTP_' => 'empty header', 'HTTP_123' => 'numbered header',
'HTTP_123_FOO' => 'alphanumerical header', 'HTTP_FOO_123' => 'reverse alphanumerical header',
'HTTP_foo' => 'lowercase header', 'HTTP_Foo' => 'mixed case header'
}
)
)
end

it 'returns the header information. Strip the HTTP_ prefix and append content-type and content-length information' do
expected_headers = {
'content-type' => 'text/html',
'cookie' => 'foo=bar',
'user-agent' => 'WebKit',
'content-length' => '0',
'' => 'empty header',
'123' => 'numbered header',
'123-foo' => 'alphanumerical header',
'foo-123' => 'reverse alphanumerical header',
'foo' => 'lowercase header, mixed case header'
}
expect(request.headers).to eq(expected_headers)
end
end
end

describe '#body' do
Expand Down
Loading