Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 #9865
Conversation
This file contains 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
|
This is also what http_parser does: https://github.com/nodejs/http-parser/blob/master/http_parser.c#L1430 |
|
@amizurov maybe you also want to review |
normanmaurer
force-pushed
the
headers_verify
branch
from
5b336ad
to
bafc633
Compare
normanmaurer
changed the title
normanmaurer
force-pushed
the
headers_verify
branch
2 times, most recently
from
9f5f64d
to
fc8b1d5
Compare
jayv
approved these changes
Dec 11, 2019
codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java
Show resolved
Hide resolved
…-length and transfer-encoding: chunked header when using HTTP/1.1 Motivation: RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked Modifications: - Check for multiple content-length headers and if found mark message as invalid - Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid - Add unit test Result: Fixes #9861
normanmaurer
force-pushed
the
headers_verify
branch
from
c0856ad
to
559e0fd
Compare
|
@netty-bot test this please |
|
@netty-bot test this please |
amizurov
reviewed
Dec 12, 2019
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
Outdated
Show resolved
Hide resolved
amizurov
approved these changes
Dec 12, 2019
|
@netty-bot test this please |
NiteshKant
approved these changes
Dec 12, 2019
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
Outdated
Show resolved
Hide resolved
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
Outdated
Show resolved
Hide resolved
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
Outdated
Show resolved
Hide resolved
codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java
Show resolved
Hide resolved
normanmaurer
force-pushed
the
headers_verify
branch
from
67667b1
to
66ce706
Compare
|
@netty-bot test this please |
normanmaurer
added a commit
that referenced
this pull request
Dec 13, 2019
…-length and transfer-encoding: chunked header when using HTTP/1.1 (#9865) Motivation: RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked Modifications: - Check for multiple content-length headers and if found mark message as invalid - Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid - Add unit test Result: Fixes #9861
dalaro
pushed a commit
to dalaro/netty
that referenced
this pull request
Mar 30, 2020
…-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865) Motivation: RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked Modifications: - Check for multiple content-length headers and if found mark message as invalid - Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid - Add unit test Result: Fixes netty#9861 (cherry picked from commit 8494b04)
dalaro
added a commit
to dalaro/netty
that referenced
this pull request
Apr 7, 2020
Compared against 4.1.25.6.dse, this tag cherry-picks upstream commits that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two intermediate refactoring commits that indirectly affect those bugfix commits. What follows is a list of PR links, issue links, CVE links, and hashes associated with the cherry-picked commits. Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865) https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238 netty#9861 netty#9865 8494b04 Detect missing colon when parsing http headers with no value (netty#9871) GHSA-cqqj-4p63-rrmm netty#9866 netty#9871 a7c18d4 Fix typos in javadocs (netty#9527) skipped Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585) https://nvd.nist.gov/vuln/detail/CVE-2019-16869 netty#9571 netty#9585 39cafcb Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492) netty#9492 85fcf4e use checkPositive/checkPositiveOrZero (netty#8835) netty#8835 4c64c98 HttpObjectDecoder ignores HTTP trailer header when empty line is rece… (netty#8799) netty#8736 netty#8799 91d3920
dalaro
added a commit
to dalaro/netty
that referenced
this pull request
Apr 7, 2020
Compared against 4.1.34.2.dse, this tag cherry-picks upstream commits that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two intermediate refactoring commits that indirectly affect those bugfix commits. What follows is a list of PR links, issue links, CVE links, and hashes associated with the cherry-picked commits. Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865) https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238 netty#9861 netty#9865 8494b04 Detect missing colon when parsing http headers with no value (netty#9871) https://nvd.nist.gov/vuln/detail/CVE-2019-20444 netty#9866 netty#9871 a7c18d4 Fix typos in javadocs (netty#9527) skipped Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585) https://nvd.nist.gov/vuln/detail/CVE-2019-16869 netty#9571 netty#9585 39cafcb Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492) netty#9492 85fcf4e
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
May 25, 2020
...and introduce HttpDecoderOption & HttpDecoderBuilder in doing so. Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. While it makes sense for Netty to bias towards the safest option, Netty is used as a library for a variety of different use cases, and there is usually no one-size-fits-all for parsing behavior like this... It seems like the ideal solution would be to default to the safest behavior but to also make these types of decisions configurable. This is further motivated by the fact that the HTTP RFCs are often vague or ambiguous in many areas and we have seen other back-and-forth conversations (e.g., netty#10003) based on differing interpretations that would probably be most amicably resolved by simply exposing better configuration. Modifications * Deprecate HttpRequestDecoder's telescoping constructors and introduce AbstractHttpObjectDecoderBuilder and HttpRequestDecoderBuilder. (The current HttpObjectDecoder implementations are only minimally configurable, and any further configuration will not scale well with the existing telescoping constructors.) * Move default decoder values to the HttpObjectDecoder class and reference them accordingly. * Introduce HttpDecoderOption class. This option is similar in vein to the ChannelOption class, where it is capable of exposing a wide possibility of different configurations. * Introduce the first decoder option: MultipleContentLengthHeadersBehavior. This option sets the precedent of using an interface for its implementation (as opposed to boolean/enum) so that users can easily define their own implementations or even "listen in" on implementations for logging purposes (without having to override). * Current available options are: ALWAYS_REJECT, IF_DIFFERENT_REJECT_ELSE_DEDUPE, and IF_DIFFERENT_REJECT_ELSE_ALLOW (see Javadoc for more). * Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException. * Extend HttpObjectDecoder to accept a map of options and use these options to resolve multiple Content-Length scenarios. * In doing so, also apply this logic to HTTP/1.0 (in addition to 1.1). Even though the cited RFCs were for 1.1, it seems that the risks they are trying to mitigate could still apply to messages in 1.0 that specify a content length. * Also include HTTP/1.0 in the existing "handleTransferEncodingChunkedWithContentLength" logic for the same reason (this logic could also probably be better served by an HttpDecoderOption in the future). * Include new unit tests to test all of the possible combinations of MultipleContentLengthHeadersBehavior and multiple content lengths. Result This is a backwards-compatible change with no functional change to the existing behavior. Users will now be able to customize the behavior for handling multiple content lengths, and we will be able to more easily add additional options in the future if needed. To limit the scope of this commit, it does not attempt to introduce the builder or decoder options to the other relevant classes (HttpResponseDecoder, HttpClientCodec, HttpServerCodec), but we may wish to follow-up with those changes.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
May 28, 2020
...and introduce HttpDecoderOption & HttpDecoderBuilder in doing so. Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. While it makes sense for Netty to bias towards the safest option, Netty is used as a library for a variety of different use cases, and there is usually no one-size-fits-all for parsing behavior like this... It seems like the ideal solution would be to default to the safest behavior but to also make these types of decisions configurable. This is further motivated by the fact that the HTTP RFCs are often vague or ambiguous in many areas and we have seen other back-and-forth conversations (e.g., netty#10003) based on differing interpretations that would probably be most amicably resolved by simply exposing better configuration. Modifications * Deprecate HttpRequestDecoder's telescoping constructors and introduce AbstractHttpObjectDecoderBuilder and HttpRequestDecoderBuilder. (The current HttpObjectDecoder implementations are only minimally configurable, and any further configuration will not scale well with the existing telescoping constructors.) * Move default decoder values to the HttpObjectDecoder class and reference them accordingly. * Introduce HttpDecoderOption class. This option is similar in vein to the ChannelOption class, where it is capable of exposing a wide possibility of different configurations. * Introduce the first decoder option: MultipleContentLengthHeadersBehavior. This option sets the precedent of using an interface for its implementation (as opposed to boolean/enum) so that users can easily define their own implementations or even "listen in" on implementations for logging purposes (without having to override). * Current available options are: ALWAYS_REJECT, IF_DIFFERENT_REJECT_ELSE_DEDUPE, and IF_DIFFERENT_REJECT_ELSE_ALLOW (see Javadoc for more). * Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException. * Extend HttpObjectDecoder to accept a map of options and use these options to resolve multiple Content-Length scenarios. * In doing so, also apply this logic to HTTP/1.0 (in addition to 1.1). Even though the cited RFCs were for 1.1, it seems that the risks they are trying to mitigate could still apply to messages in 1.0 that specify a content length. * Also include HTTP/1.0 in the existing "handleTransferEncodingChunkedWithContentLength" logic for the same reason (this logic could also probably be better served by an HttpDecoderOption in the future). * Include new unit tests to test all of the possible combinations of MultipleContentLengthHeadersBehavior and multiple content lengths. Result This is a backwards-compatible change with no functional change to the existing behavior. Users will now be able to customize the behavior for handling multiple content lengths, and we will be able to more easily add additional options in the future if needed. To limit the scope of this commit, it does not attempt to introduce the builder or decoder options to the other relevant classes (HttpResponseDecoder, HttpClientCodec, HttpServerCodec), but we may wish to follow-up with those changes.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 12, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 12, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 15, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 24, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 24, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
Bennett-Lynch
added a commit
to Bennett-Lynch/netty
that referenced
this pull request
Jun 25, 2020
Motivation Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
normanmaurer
pushed a commit
that referenced
this pull request
Jul 6, 2020
…10349) Motivation: Since #9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in #9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications: * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result: This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
normanmaurer
pushed a commit
that referenced
this pull request
Jul 6, 2020
…10349) Motivation: Since #9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in #9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications: * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result: This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
ihanyong
pushed a commit
to ihanyong/netty
that referenced
this pull request
Jul 31, 2020
…-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865) Motivation: RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked Modifications: - Check for multiple content-length headers and if found mark message as invalid - Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid - Add unit test Result: Fixes netty#9861
ihanyong
pushed a commit
to ihanyong/netty
that referenced
this pull request
Jul 31, 2020
…etty#10349) Motivation: Since netty#9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in netty#9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > If a message is received that has multiple Content-Length header > fields with field-values consisting of the same decimal value, or a > single Content-Length header field with a field value containing a > list of identical decimal values (e.g., "Content-Length: 42, 42"), > indicating that duplicate Content-Length header fields have been > generated or combined by an upstream message processor, then the > recipient MUST either reject the message as invalid or replace the > duplicated field-values with a single valid Content-Length field > containing that decimal value prior to determining the message body > length or forwarding the message. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications: * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result: This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change.
suneet-s
added a commit
to suneet-s/netty
that referenced
this pull request
Aug 19, 2020
Backport netty#9865
suneet-s
added a commit
to implydata/netty
that referenced
this pull request
Aug 19, 2020
Backport netty#9865
This was referenced Aug 1, 2022
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.
Motivation:
RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked
Modifications:
Result:
Fixes #9861