-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
http inspector: use http_parser in http inspector #8808
Conversation
Signed-off-by: crazyxy <yxyan@google.com>
Considering that http-parser needs to be replaced (#5155) should we increase its usage? Or will this code be portable / usable by llhttp (if that is what we use to replace http-parser)? |
The problem here is that current parsing logic in http inspector is different from http-parser. There might be some case that traffic is detected as non http, but http-parser can parse successfully, e.g.
I didn't do investigation and don't know if llhttp will give the same result as http-parser when inspecting the incoming packets and I think we need to migrate this code to llhttp when http-parser is completely replaced by llhttp for http/1x codec. |
@moderation we can migrate both place to llhttp later if that's the choice, at this point the focus of this PR is use same parser so we get consistent result. llhttp has pretty similar interface to http_parser so increasing usage here shouldn't affect too much on migration. |
Signed-off-by: crazyxy <yxyan@google.com>
@@ -108,6 +128,7 @@ ParseState Filter::onRead() { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using MSG_PEEK repetitively leads to O(N^2) cost if the attacker sends 1 byte messages. The 8K limit is AFAICT only for Apache, other servers can accept longer URLs.
In many cases a URL can be padded out in a way that the server will still accept, for example by providing unnecessary query key-value pairs. This makes it possible to fool the parser and still have the URL accepted by the backend.
You can also use a callback to abort further parsing in on_headers_complete by returning an error and checking for CB_headers_complete failure before declaring a ParseState::Error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to validate the request line only as discussed in the below comment.
source/extensions/filters/listener/http_inspector/http_inspector.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/listener/http_inspector/http_inspector.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: crazyxy <yxyan@google.com>
Signed-off-by: crazyxy <yxyan@google.com>
Hmm.. I added a test for larger request line and the test failed under both asan and tasn. asan failed with error
tsan failed silently Working on it |
It seems the ci failure is not related with the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http_parser support stream interface
The semantic of http_parser_execute is feeding new data.
We should use the assumption that the data we peek last time should remain the same. Suppose last time we see 6k data, and this time we see 8k, we should feed the slice of (6k, 8k) using http_parser_execute(&parser_/last state/, &setting_, request_line.data() + 6k, request_line.length()-6k);
source/extensions/filters/listener/http_inspector/http_inspector.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/listener/http_inspector/http_inspector.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/listener/http_inspector/http_inspector.h
Outdated
Show resolved
Hide resolved
source/extensions/filters/listener/http_inspector/http_inspector.h
Outdated
Show resolved
Hide resolved
test/extensions/filters/listener/http_inspector/http_inspector_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Yan Xue <yxyan@google.com>
.WillOnce( | ||
Invoke([&data, len](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { | ||
ASSERT(length >= data.size()); | ||
memcpy(buffer, data.data(), data.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len
instead of data.size()
? (ditto for below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yxue I meant len
here exactly 😅to copy less memory, not size()
vs length()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😳 let me change it back
Description: use http_parser in http inspector Risk Level: Med Testing: Unit test Docs Changes: N/A Release Notes: N/A Signed-off-by: crazyxy <yxyan@google.com> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
…120) Description: use http_parser in http inspector Risk Level: Med Testing: Unit test Docs Changes: N/A Release Notes: N/A Signed-off-by: crazyxy <yxyan@google.com> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: crazyxy yxyan@google.com
Description: use http_parser in http inspector
Risk Level: Med
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]