Extra Header Validation - #26115
Conversation
Signed-off-by: Andrew James Pielage <andrew.pielage@azul.com>
|
Hi @Pandrex247, which issue does this address? Grizzly should handle this already by ignoring Conten-tLength header if Transfer-Encoding header is present, as explained in eclipse-ee4j/glassfish-grizzly#2237 you linked. Is there a reason why GlassFish needs this validation on top of Grizzly and refuse requests with both headers instead of just ignoring the Content-Length header? |
|
Grizzly itself does, but not necessarily Payara/GlassFish. This is stricter validation. |
|
@Pandrex247 , I replied to you on Gitlab: https://gitlab.eclipse.org/security/vulnerability-reports/-/work_items/293#note_7934313 In short, I believe that the fix in Grizzly is enough and it works in GlassFish 8.0.3, this this PR is not needed. It adds a performance penalty and I don't see how it addresses any issue that isn't already addressed. |
|
Can we close this? The fix was distributed with Grizzly 5.0.2 and GlassFish 8.0.3. Basically it is enabled the strict validation by default, while older releases were benevolent by default. The extra validation seems redundant to me now. |
|
I understand now. Grizzly now addresses the issue by ignoring the Content-Length header and continues processing. This PR for GlassFish instead raises an error in this situation and doesn't continue processing. This is explained in https://gitlab.eclipse.org/security/vulnerability-reports/-/work_items/293#note_8157118 The RFC explicitly says that the Content-Length header should be ignored, but also strongly suggests raising an error instead of processing normally without the Content-Length header. So the solution proposed here is more robust, however it introduces performance penalty because the check happens for every request and is not very cheap. |
| final HttpRequestPacket request = (HttpRequestPacket) httpHeader; | ||
|
|
||
| // Here we can add extra validation to prevent Http Server invalid use of headers | ||
| if (request.containsHeader(Header.ContentLength) && request.containsHeader(Header.TransferEncoding)) { |
There was a problem hiding this comment.
This doesn't seem to be a valid check because Grizzly always computes the value of the Content-Length header from the content so I believe that request.containsHeader(Header.ContentLength) always returns true. That would lead to refusing all requests that contain Transfer-Encoding header.
@Pandrex247, can you verify that correct requests with just the Transfer-Encoding are not always refused?
3 participants
Spun off from eclipse-ee4j/glassfish-grizzly#2237