Repository navigation
Merge branches from forks (#17063) - #17065
Merged
Merged
Conversation
--- Stomp: Limit headers per frame 83ce0a8 Motivation: We need to enforce a lmit of headers per frame as otherwise a remote peer can flood us. Modifications: - Enforce limit of number of headers - Add unit test Result: Guard against high memory usage caused by frames with unbound number of headers --- DNS: Don't leak buffer in DNS Record Decoder via Malformed Domain Names 50649d7 Motivation: We missed to release allocated buffers in case of maformed domain names. Modifications: Correctly release buffers in all cases Result: No more memory leak on malformed domain names --- SPDY: Limit max number of settings per settings frame 0b0b899 Motivation: There was no real limit of how many settings a settings frame could include and so could result in very high memory usage on the receiver side. Modifications: - Add a default limit of 64 - Add unit test Result: Guard against high memory usage by default --- HaProxy: Correctly treat version as unsigned byte 94b04f7 Motivation: We did not correctly tread the version as unsigned byte which could cause us to buffer received bytes forever and so cause an OOME Modifications: - Correctly treat version as unsigned byte - Add unit test Result: Correctly detect invalid version and not buffer forever --- SPDY: Correctly release message once removed from internal storage 4ff0898 Motivation: We missed to release the stored FullHttpMessage in some cases which could result in a memory leak Modifications: - Add missing release calls - Also release when handler is removed before channel becomes inactive Result: No more leaks --- CORS: Correctly handle origin afca14b Motivation: Netty's CorsHandler provides a shortCircuit() configuration designed to reject unauthorized cross-origin requests immediately, acting as a security control before requests reach the application. However, due to a logical operator error in the origin evaluation process, this protection can be entirely bypassed. An attacker can bypass the short-circuit mechanism by sending a request with an Origin: null header. This failure forwards unauthorized requests to the backend application, bypassing intended access controls. Modifications: - Replace || with && Result: Fix bypass --- XML: Disable risky features when decoding XML ee10fe7 Motivation: We did not diable external entities, DTD and replacing of entity references when creating the AsyncXMLInputFactory, which means we were in the mercy of the defaults of aalto-xml. Modifications: - Set properties to disable risky features and so reduce risk when decoding XML via the network Result: Minimize risk when decoding XML from untrusted sources --- HTTP2: Don't leak buffer when using compression and the stream is clo… e9cec3f …sed while still decompress data Motivation: We need to ensure we not leak the buffer if the used EmbeddedChannel is already closed when trying to decompress data. This can happen if we receive an END_STREAM in between. Modifications: - Explicit check if the EmbeddedChannel is open before retain the buffer Result: No more leak --- Redis: Clear partial state and release messages before throwing excep… 5f5c65b …tion Motivation: We did not always clear the partial state and release nested message before throwing exception which could lead to unnessary memory usage until the channel is finally closed Modifications: Always clear partial state and release messages before throwing Result: Clear state and release memory in a timely manner when exception accours --- OCSP: Correctly validate cert id matches dec6f8e Motivation: We did miss to also validate the cert id and so could be affected by replay attacks Modifications: - Add validation of cert id - Add unit test Result: Correctly validate cert --- OCSP: TOCTOU in OcspServerCertificateValidator 1c7bf68 Motivation: Netty's OcspServerCertificateValidator forwards the SslHandshakeCompletionEvent before the asynchronous OCSP validation completes. This allows the client's downstream handlers to send sensitive application data (e.g., HTTP requests) to a revoked server before the channel is closed by the OCSP check. Modifications: - Correctly only fire the event after we did the ocsp check - Enforce some sort of timeout for the OCSP query - Buffer bytes until we were able to process the OCSP query - Add missing early return Result: Only fire event / forward data after the OCSP processing is done --- Improve OCSP response validity-window handling in OcspServerCertifica… e091e0f …teValidator Motivation: The thisUpdate / nextUpdate fields of an OCSP response are optional, and the freshness check did not always stop processing a response that failed the window check. Modifications: Handle absent thisUpdate / nextUpdate and return once an out-of-date response is detected. Result: More robust and consistent OCSP response validity-window handling. --- Validate STOMP CONNECT/CONNECTED command headers 536d64a Motivation: The CONNECT/CONNECTED command headers do not support escaping, but we still need to validate that they contain no illegal characters. Additionally, the NUL ascii character has no escape sequence and is always illegal. The only normally-escaped character that CONNECT/CONNECTED commands pass through raw is the backslash. Modification: - Update the test to match the specification exactly. - Add validation to CONNECT/CONNECTED command headers. - Add a throws case in `escape` for the NUL character. Result: It's no longer possible to inject headers or smuggle commands through STOMP CONNECT/CONNECTED headers. --- Add multipart filename validation 15c62b0 Motivation: The rules for encoding filenames in `multipart/form-data` means we have to avoid certain characters to prevent parser-desync problems, and to ensure spec compliance. Modification: - The DiskFileUpload and MemoryFileUpload constructor and setFilename methods now validate that the given filename is safe to be included verbatim into the multipart filename field, and will throw an exception if not. - The HttpPostMultipartRequestDecoder now nerfs all illegal characters - even those delivered through percent encoding - to prevent round-trip validation errors. Result: Parser-desync and smuggling through the multipart/form-data filename field is no longer possible. --- HAProxy: Reject illegal characters in UNIX socket files 409621c Motivation: The PROXY V1 protocol for HAProxy uses CR LF bytes as header delimiters, and space as header field delimiters. This means these characters are not allowed within the fields, and must be rejected. The V2 protocol has no such problem because it uses a TLV binary encoding. Modification: Add checks and throw an exception from the HAProxyMessage constructor if AF_UNIX socket addresses contain CR, LF, or space, and the protocol version is V1. Add tests to verify the exception is thrown on V1, and not on V2. Result: PROXY field injection through AF_UNIX socket filenames is prevented. --- Bzip2: Correctly detect malformated stream and not infinite loop 0b7fb9b Motivation: Due a bug we could end up in an infinite loop while read from the bzip2 stream Modifications: - Correctly detect malformated stream and throw exception - Add unit tests Result: Correctly throw exception when malformated stream is detected --- HTTP/2: Lack of Host Header Deduplication in HTTP/2→HTTP/1.x Translat… 0ff87c6 …ion Leads to Request Routing Bypass Motivation: Netty's HTTP/2-to-HTTP/1.x translation layer (`Http2StreamFrameToHttpObjectCodec` and `InboundHttp2ToHttpAdapter`) fails to deduplicate or validate `Host` headers when an HTTP/2 client supplies both the `:authority` pseudo-header and a literal `host` header in a single HEADERS frame. The translator maps `:authority` to `Host` and separately copies the literal `host` header, producing an `HttpRequest` object containing two `Host` headers with attacker-controlled differing values. Modifications: - Detect duplicated host headers and if detected throw stream error - add unit test Result: Correctly detect invalid headers during translation --- HTTP: Enforce pipeline limit in HttpContentEncoder 1d5cdac Motivation: We did not enforce any pipeline limit and so it was possible for a remote peer to cause A DOS Modifications: - Add constructor that allows to set a limit (use default of 128). - Add unit test Result: Guard against DOS caused by concurrent pipelined requests --- SPDY: Limit the maximum number of bytes that can be decompressed per … 69562fa …header block. Motivation: We should limit the number of bytes that can be dcompressed per header block to guard against DOS attacks Modifications: - Add some limit - Add unit tests Result: Limit the number of bytes for compressed header blocks --- Avoid repeated closing-tag scans in XmlFrameDecoder efb0044 Motivation: XmlFrameDecoder scanned forward from every closing tag start to find a terminating '>'. Repeated malformed closing tags could therefore force repeated scans over the cumulated buffer. Modification: Track a pending closing tag in the main decode loop, reject nested '<' before the closing tag terminator, and cover malformed repeated closing tags plus valid split closing tags in tests. Result: Malformed repeated closing tags fail fast while valid split closing tags continue to frame correctly. --- WebSockets: V07/V08 handshaker missing Connection/Upgrade validation 52addff Motivation: An attacker can force WebSocket upgrade via the lax V07 (or V08) handshaker by sending Sec-WebSocket-Version: 7 and omitting Connection: Upgrade / Upgrade: websocket headers, completing a protocol switch that a proxy would not recognize as an Upgrade request and enabling HTTP request smuggling / protocol-confusion attacks. Modifications: - Add header checks - Add unit testing Result: Correctly validate headers for V07 and V08 Co-authored-by: Norman Maurer <norman_maurer@apple.com> Co-authored-by: Violeta Georgieva <696661+violetagg@users.noreply.github.com> Co-authored-by: yawkat <jonas.konrad@oracle.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stomp: Limit headers per frame
83ce0a8
Motivation:
We need to enforce a lmit of headers per frame as otherwise a remote peer can flood us.
Modifications:
Result:
Guard against high memory usage caused by frames with unbound number of headers
DNS: Don't leak buffer in DNS Record Decoder via Malformed Domain Names 50649d7
Motivation:
We missed to release allocated buffers in case of maformed domain names.
Modifications:
Correctly release buffers in all cases
Result:
No more memory leak on malformed domain names
SPDY: Limit max number of settings per settings frame 0b0b899
Motivation:
There was no real limit of how many settings a settings frame could include and so could result in very high memory usage on the receiver side.
Modifications:
Result:
Guard against high memory usage by default
HaProxy: Correctly treat version as unsigned byte
94b04f7
Motivation:
We did not correctly tread the version as unsigned byte which could cause us to buffer received bytes forever and so cause an OOME
Modifications:
Result:
Correctly detect invalid version and not buffer forever
SPDY: Correctly release message once removed from internal storage 4ff0898
Motivation:
We missed to release the stored FullHttpMessage in some cases which could result in a memory leak
Modifications:
Result:
No more leaks
CORS: Correctly handle origin
afca14b
Motivation:
Netty's CorsHandler provides a shortCircuit() configuration designed to reject unauthorized cross-origin requests immediately, acting as a security control before requests reach the application. However, due to a logical operator error in the origin evaluation process, this protection can be entirely bypassed. An attacker can bypass the short-circuit mechanism by sending a request with an Origin: null header. This failure forwards unauthorized requests to the backend application, bypassing intended access controls.
Modifications:
Result:
Fix bypass
XML: Disable risky features when decoding XML
ee10fe7
Motivation:
We did not diable external entities, DTD and replacing of entity references when creating the AsyncXMLInputFactory, which means we were in the mercy of the defaults of aalto-xml.
Modifications:
Result:
Minimize risk when decoding XML from untrusted sources
HTTP2: Don't leak buffer when using compression and the stream is clo… e9cec3f
…sed while still decompress data
Motivation:
We need to ensure we not leak the buffer if the used EmbeddedChannel is already closed when trying to decompress data. This can happen if we receive an END_STREAM in between.
Modifications:
Result:
No more leak
Redis: Clear partial state and release messages before throwing excep… 5f5c65b
…tion
Motivation:
We did not always clear the partial state and release nested message before throwing exception which could lead to unnessary memory usage until the channel is finally closed
Modifications:
Always clear partial state and release messages before throwing
Result:
Clear state and release memory in a timely manner when exception accours
OCSP: Correctly validate cert id matches
dec6f8e
Motivation:
We did miss to also validate the cert id and so could be affected by replay attacks
Modifications:
Result:
Correctly validate cert
OCSP: TOCTOU in OcspServerCertificateValidator
1c7bf68
Motivation:
Netty's OcspServerCertificateValidator forwards the SslHandshakeCompletionEvent before the asynchronous OCSP validation completes. This allows the client's downstream handlers to send sensitive application data (e.g., HTTP requests) to a revoked server before the channel is closed by the OCSP check.
Modifications:
Result:
Only fire event / forward data after the OCSP processing is done
Improve OCSP response validity-window handling in OcspServerCertifica… e091e0f
…teValidator
Motivation:
The thisUpdate / nextUpdate fields of an OCSP response are optional, and the freshness check did not always stop processing a response that failed the window check.
Modifications:
Handle absent thisUpdate / nextUpdate and return once an out-of-date response is detected.
Result:
More robust and consistent OCSP response validity-window handling.
Validate STOMP CONNECT/CONNECTED command headers
536d64a
Motivation:
The CONNECT/CONNECTED command headers do not support escaping, but we still need to validate that they contain no illegal characters.
Additionally, the NUL ascii character has no escape sequence and is always illegal.
The only normally-escaped character that CONNECT/CONNECTED commands pass through raw is the backslash.
Modification:
escapefor the NUL character.Result:
It's no longer possible to inject headers or smuggle commands through STOMP CONNECT/CONNECTED headers.
Add multipart filename validation
15c62b0
Motivation:
The rules for encoding filenames in
multipart/form-datameans we have to avoid certain characters to prevent parser-desync problems, and to ensure spec compliance.Modification:
Result:
Parser-desync and smuggling through the multipart/form-data filename field is no longer possible.
HAProxy: Reject illegal characters in UNIX socket files 409621c
Motivation:
The PROXY V1 protocol for HAProxy uses CR LF bytes as header delimiters, and space as header field delimiters. This means these characters are not allowed within the fields, and must be rejected. The V2 protocol has no such problem because it uses a TLV binary encoding.
Modification:
Add checks and throw an exception from the HAProxyMessage constructor if AF_UNIX socket addresses contain CR, LF, or space, and the protocol version is V1. Add tests to verify the exception is thrown on V1, and not on V2.
Result:
PROXY field injection through AF_UNIX socket filenames is prevented.
Bzip2: Correctly detect malformated stream and not infinite loop 0b7fb9b
Motivation:
Due a bug we could end up in an infinite loop while read from the bzip2 stream
Modifications:
Result:
Correctly throw exception when malformated stream is detected
HTTP/2: Lack of Host Header Deduplication in HTTP/2→HTTP/1.x Translat… 0ff87c6
…ion Leads to Request Routing Bypass
Motivation:
Netty's HTTP/2-to-HTTP/1.x translation layer (
Http2StreamFrameToHttpObjectCodecandInboundHttp2ToHttpAdapter) fails to deduplicate or validateHostheaders when an HTTP/2 client supplies both the:authoritypseudo-header and a literalhostheader in a single HEADERS frame. The translator maps:authoritytoHostand separately copies the literalhostheader, producing anHttpRequestobject containing twoHostheaders with attacker-controlled differing values.Modifications:
Result:
Correctly detect invalid headers during translation
HTTP: Enforce pipeline limit in HttpContentEncoder 1d5cdac
Motivation:
We did not enforce any pipeline limit and so it was possible for a remote peer to cause A DOS
Modifications:
Result:
Guard against DOS caused by concurrent pipelined requests
SPDY: Limit the maximum number of bytes that can be decompressed per … 69562fa
…header block.
Motivation:
We should limit the number of bytes that can be dcompressed per header block to guard against DOS attacks
Modifications:
Result:
Limit the number of bytes for compressed header blocks
Avoid repeated closing-tag scans in XmlFrameDecoder efb0044
Motivation:
XmlFrameDecoder scanned forward from every closing tag start to find a terminating '>'. Repeated malformed closing tags could therefore force repeated scans over the cumulated buffer.
Modification:
Track a pending closing tag in the main decode loop, reject nested '<' before the closing tag terminator, and cover malformed repeated closing tags plus valid split closing tags in tests.
Result:
Malformed repeated closing tags fail fast while valid split closing tags continue to frame correctly.
WebSockets: V07/V08 handshaker missing Connection/Upgrade validation 52addff
Motivation:
An attacker can force WebSocket upgrade via the lax V07 (or V08) handshaker by sending Sec-WebSocket-Version: 7 and omitting Connection: Upgrade / Upgrade: websocket headers, completing a protocol switch that a proxy would not recognize as an Upgrade request and enabling HTTP request smuggling / protocol-confusion attacks.
Modifications:
Result:
Correctly validate headers for V07 and V08