Fix bypassing Request Body scanning with trailers #288
Conversation
The present implementation of WASM plugin is missing a scenario that onRequestBody method will never be called with end_of_stream parameter set to true. Such situation will happen for HTTP 2 request with trailers as the presence of trailers means that the last chunk of payload is still not the end of the request stream. This is problematic because existing implementation triggers threat scanning when onRequestBody is called with parameter end_of_stream set to true as it was expected to be a proper place: * right after collecting entire payload * before completing the request stream This is true only for scenarios that do NOT include request trailers (unless it is HTTP1 as Envoy Proxy ignores request trailers for that protocol - the explanation is that trailers in HTTP1 are very often not handled properly by various servers even though HTTP standard allows it). To fix it, onRequestTrailers method was implemented to cover the situation when request payload was collected as onRequestTrailers is the method which applies to the situation: * right after collecting entire payload * before completing the request stream To maintain the existing logic, this method calls onRequestBody with end_of_stream set to true, to share the same logic. The onResponseTrailers method was not implemented but perhaps it should be implemented as well.
iosetek
force-pushed
the
fix-bypassing-request-body-scanning
branch
from
9a5fadc to
7d46156
Compare
There was a problem hiding this comment.
Thanks for the very detailed issue/PR. Writing a few initial comments, I will take a deep look at it as soon as possible over the weekend.
Also, would you think is possible to have a smaller reproducer that can fit go tests or, eventually, an e2e?
| The end_of_stream parameter definition may be unclear - it is set to | ||
| true if: | ||
| * there are no more payload chunks to process | ||
| * there are no HTTP trailers |
There was a problem hiding this comment.
I'm aware of the convoluted (and lack of) documentation, but by any chance, do you have any reference about this? I just found these lines that might be making me think in that direction: https://github.com/proxy-wasm/spec/pull/42/files#diff-39c0fcbaec7f6ffb311e442b6f0774bf3c050cd9b3318f51f55c2d6c38d4976dR609-R610, but any reference to docs/code would be great
There was a problem hiding this comment.
Further note that end_stream is only true if there are no trailers.
https://github.com/envoyproxy/envoy/blob/121a541dd3fadef7131963f23e42a41e0c93e102/envoy/http/filter.h#L913
| // If no payload was collected so far, then there is nothing to do. | ||
| if ctx.bodyReadIndex == 0 { | ||
| return types.ActionContinue | ||
| } |
There was a problem hiding this comment.
I would delegate that decision to the existing logic by also calling in that case ctx.OnHttpRequestBody.
If I'm getting this right, OnHttpRequestTrailers is going to be called just once and it will happen after all the OnHttpRequestBody calls. So, from a Coraza point of view, what we have to do now is process phase:2 rules independently from the fact that they will analyze the payload or not.
The present implementation of WASM plugin is missing a scenario that onRequestBody method will never be called with end_of_stream parameter set to true. Such situation will happen for HTTP 2 request with trailers as the presence of trailers means that the last chunk of payload is still not the end of the request stream.
This is problematic because existing implementation triggers threat scanning when onRequestBody is called with parameter end_of_stream set to true as it was expected to be a proper place:
right after collecting entire payload
before completing the request stream
This is true only for scenarios that do NOT include request trailers (unless it is HTTP1 as Envoy Proxy ignores request trailers for that protocol - the explanation is that trailers in HTTP1 are very often not handled properly by various servers even though HTTP standard allows it).
To fix it, onRequestTrailers method was implemented to cover the situation when request payload was collected as onRequestTrailers is the method which applies to the situation:
right after collecting entire payload
before completing the request stream
To maintain the existing logic, this method calls onRequestBody with end_of_stream set to true, to share the same logic.
The onResponseTrailers method was not implemented but perhaps it should be implemented as well.