Reject HTTP/2 header values with invalid characters #12760

chrisvest · 2022-08-31T22:47:54Z

This PR builds upon #12755.

Motivation:
In https://datatracker.ietf.org/doc/html/rfc7540#section-10.3 it says that only certain characters are valid in a header value:

Any request or response that contains a character not permitted
in a header field value MUST be treated as malformed (Section 8.1.2.6).
Valid characters are defined by the "field-content" ABNF rule in
Section 3.2 of [RFC7230].

Modification:
Add a header value validation step to HpackDecoder.

Result:
Header values are now validated against the Section 10.3, etc. rules.

normanmaurer · 2022-09-02T07:59:03Z

@ejona86 PTAL as well

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

...c-http2/src/test/resources/io/netty/handler/codec/http2/testdata/testStaticTableEntries.json

chrisvest · 2022-09-02T15:34:35Z

@normanmaurer Comments addressed.

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

idelpivnitskiy · 2022-09-02T16:33:51Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

@@ -416,6 +416,9 @@ private static HeaderType validate(int streamId, CharSequence name,
            throw streamError(streamId, PROTOCOL_ERROR,
                    "Illegal value specified for the 'TE' header (only 'trailers' is allowed).");
        }
+        if (!isValidHeaderValue(value)) {


I'm agreed that this validation is important and we should follow the spec. On the other hand, header values are tend to be quite long. Validating each value may be a significant performance hit. There are cases when communication is trusted between peers and such validation is not necessary. Can we consider an opt-out config options from this validation?

We have a config for validating the headers. You want a separate one for header value validation?

Yes, validation of the header-name is more important (they are more strict and most attackers target header names) and is used by many netty users by default (header names usually has more predictable length). If we add more validation for header values, we should let users preserve pre-existing behavior of validating only header names but not values.

Actually, since the same validation rule applies for HTTP/2 and HTTP/1.x, have you considered moving this validation to the upper layer where it can be shared between two?

ServiceTalk already does validation of the header values for both protocols, if netty also does it internally for HTTP/2 only without option to opt-out or share validation between HTTP/2 and HTTP/1.x, ServiceTalk users may end up doing expensive validation 2 times.

I want to address that as a separate PR, because moving the validation around might have unpredictable effects on our existing tests.

I would leave it up to @normanmaurer. The benefit of doing this now instead of a separate PR is the API impact. For example, if you end up moving values validation to DefaultHeaders to share that between HTTP/2 and HTTP/1.X, you won't need any API changes for DefaultHttp2HeadersDecoder made in this PR.

Btw, moving this validation to DefaultHeaders has another benefit: users will be able to control it for inbound and outbound messages. In my practice, users frequently make mistakes in outbound header values, like adding \n at the end of the value (especially when the value is pulled from somewhere). When an illegal header sent, they see a weird error at transport level that doesn't describe the cause for request failure. Very hard to debug unless you have a validation before sending the request that describes where is the mistake.

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

normanmaurer · 2022-09-05T12:57:10Z

@chrisvest let me know once this is ready for re-review

chrisvest · 2022-09-06T17:38:53Z

@normanmaurer This is ready for a second review now.

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDynamicTableTest.java

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java

codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoderTest.java

bodhili

Exposed Used for testing only! Default values used in the initial settings frame are overridden intentionally

bodhili

Exposed used for testing only! Default values used in the initial settings frame are overridden intentionally

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java

chrisvest · 2022-09-15T22:11:29Z

@normanmaurer @idelpivnitskiy I've consolidated the validation and pushed it down into the headers implementation, except for the validation that HTTP/2 headers are decoded pseudo-headers first, since I don't think we can compatibly place such a temporal restriction on the Http2Headers interface.

codec-http2/src/test/java/io/netty/handler/codec/http2/HttpConversionUtilTest.java

@@ -145,7 +145,7 @@

    @Test
    public void stripTEHeadersAccountsForOWS() {
-        HttpHeaders inHeaders = new DefaultHttpHeaders();
+        HttpHeaders inHeaders = new DefaultHttpHeaders(false);


codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValidationUtil.java

codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java

codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java

codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDynamicTableTest.java

idelpivnitskiy

LGTM!

codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java

codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java

…ng header names and values

chrisvest · 2022-09-20T21:12:45Z

@normanmaurer @idelpivnitskiy Addressed your comments. Please take a look.

normanmaurer · 2022-09-21T14:39:39Z

Ship it...

idelpivnitskiy

🚢

codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java

…aders Netty fixed the checks for HTTP/2 headers' compliance with RFC netty/netty#12760 This fixes the build against Netty 4.1 SNAPSHOT https://github.com/reactor/reactor-netty/actions/runs/3106167960

…aders (#2501) Netty fixed the checks for HTTP/2 headers' compliance with RFC netty/netty#12760 This fixes the build against Netty 4.1 SNAPSHOT https://github.com/reactor/reactor-netty/actions/runs/3106167960

Motivation: Netty 4.1.84 includes netty/netty#12760 which moves validation of connection-related headers for HTTP/2 from `HpackDecoder` to `DefaultHttp2Headers`. This breaks our existing control flow because in case there is no `content-length` header, we always unconditionally add `transfer-encoding: chunked` header that will be removed later in HTTP/2 pipeline if the HTTP/2 is the actual negotiated protocol. Modification: - Improve our control flow to take protocol into account when it adds `transfer-encoding: chunked` header; - Adjust existing tests that target `transfer-encoding: chunked` handling to always use `DefaultHttpHeadersFactory`, even when testing HTTP/2, it helps to avoid validation in netty's `DefaultHttp2Headers`; Result: All tests pass with both versions of netty: 4.1.82 and 4.1.84.

Motivation: netty#12755 added validation for presence of connection-related headers while `HpackDecoder` decodes the incoming frame. Then netty#12760 moved this validation from `HpackDecoder` to `DefaultHttp2Headers`. As the result, existing use-case that could use `DefaultHttp2Headers` for HTTP/2 and HTTP/1.X broke when users add any of the mentioned prohibited headers. The HTTP/1.X to HTTP/2 translation logic usually has sanitization process that removes connection-related headers. It's enough to run this validation only for incoming messages and we should preserve backward compatibility for 4.1. Modifications: - Move `isConnectionHeader` and `te` validations from `DefaultHttp2Headers` back to `HpackDecoder`; - Add tests to verify `HpackDecoder` fails incoming headers as expected; - Add tests to verify mentioned headers can be added to `DefaultHttp2Headers`; Result: Backward compatibility is preserved, while validation for connection-related headers is done in `HpackDecoder`.

#12975) Motivation: #12755 added validation for presence of connection-related headers while `HpackDecoder` decodes the incoming frame. Then #12760 moved this validation from `HpackDecoder` to `DefaultHttp2Headers`. As the result, existing use-case that could use `DefaultHttp2Headers` for HTTP/2 and HTTP/1.X broke when users add any of the mentioned prohibited headers. The HTTP/1.X to HTTP/2 translation logic usually has sanitization process that removes connection-related headers. It's enough to run this validation only for incoming messages and we should preserve backward compatibility for 4.1. Modifications: - Move `isConnectionHeader` and `te` validations from `DefaultHttp2Headers` back to `HpackDecoder`; - Add tests to verify `HpackDecoder` fails incoming headers as expected; - Add tests to verify mentioned headers can be added to `DefaultHttp2Headers`; Result: Backward compatibility is preserved, while validation for connection-related headers is done in `HpackDecoder`.

valfirst · 2022-11-16T15:14:43Z

codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java

-                default:
-                    break;
+        public void validate(CharSequence value) {
+            int index = HttpHeaderValidationUtil.validateValidHeaderValue(value);


should the same validation rules be applied for both HTTP/1 and HTTP/2? This change introduces backward compatibility issues

@chrisvest this looks like omission. value validation should be off by default for all protocols with opt-in flag. I see that for DefaultHttp2Headers but not for `DefaultHttpHeaders

chrisvest added this to the 4.1.81.Final milestone Aug 31, 2022

chrisvest requested review from normanmaurer and idelpivnitskiy August 31, 2022 22:47

normanmaurer requested a review from ejona86 September 2, 2022 07:58

normanmaurer requested changes Sep 2, 2022

View reviewed changes

normanmaurer approved these changes Sep 2, 2022

View reviewed changes

chrisvest force-pushed the 4.1-filter-header-values branch from a0f5d3d to b3d925c Compare September 2, 2022 16:05

idelpivnitskiy reviewed Sep 2, 2022

View reviewed changes

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java Outdated Show resolved Hide resolved

idelpivnitskiy reviewed Sep 2, 2022

View reviewed changes

chrisvest mentioned this pull request Sep 2, 2022

Reject connection related headers in HTTP/2 frames #12755

Merged

idelpivnitskiy reviewed Sep 7, 2022

View reviewed changes

chrisvest force-pushed the 4.1-filter-header-values branch from 0d88318 to f5715b8 Compare September 7, 2022 21:58

chrisvest marked this pull request as draft September 7, 2022 22:27

normanmaurer requested changes Sep 8, 2022

View reviewed changes

chrisvest removed this from the 4.1.81.Final milestone Sep 8, 2022

bodhili reviewed Sep 12, 2022

View reviewed changes

bodhili suggested changes Sep 12, 2022

View reviewed changes

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java Outdated Show resolved Hide resolved

chrisvest force-pushed the 4.1-filter-header-values branch 2 times, most recently from 2bdfffb to 72f5fe7 Compare September 15, 2022 22:05

chrisvest marked this pull request as ready for review September 15, 2022 22:06

github-advanced-security bot found potential problems Sep 15, 2022

View reviewed changes

chrisvest requested review from normanmaurer and idelpivnitskiy September 16, 2022 00:17

normanmaurer requested changes Sep 18, 2022

View reviewed changes

idelpivnitskiy reviewed Sep 20, 2022

View reviewed changes

chrisvest added 6 commits September 20, 2022 13:35

Specialize validation in HpackDecoder since it only produce AsciiStri…

cdf3258

…ng header names and values

Address some more comments

fcd4f4f

Make illegal http header char tests parameterized

9cc54d6

Consolidate HTTP header value and name validation

23620a6

Small javadoc update

bd406d3

Address PR review comments

73d119f

chrisvest force-pushed the 4.1-filter-header-values branch from a914a47 to 73d119f Compare September 20, 2022 21:10

normanmaurer approved these changes Sep 20, 2022

View reviewed changes

idelpivnitskiy approved these changes Sep 21, 2022

View reviewed changes

codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java Outdated Show resolved Hide resolved

Address nit

e45ea6a

chrisvest merged commit 980f48a into netty:4.1 Sep 21, 2022

chrisvest deleted the 4.1-filter-header-values branch September 21, 2022 16:10

chrisvest added this to the 4.1.83.Final milestone Sep 21, 2022

chrisvest mentioned this pull request Sep 21, 2022

Reject HTTP/2 header values with invalid characters #12832

Merged

violetagg mentioned this pull request Sep 23, 2022

[test] Ensure HTTP/2 tests comply with RFC requirements for HTTP/2 headers reactor/reactor-netty#2501

Merged

idelpivnitskiy mentioned this pull request Oct 11, 2022

Prepare for upgrade to Netty 4.1.84 apple/servicetalk#2392

Merged

pietrygamat mentioned this pull request Nov 1, 2022

Revert "Bump netty.version from 4.1.82.Final to 4.1.84.Final" LittleProxy/LittleProxy#178

Merged

idelpivnitskiy mentioned this pull request Nov 8, 2022

Move validation of connection headers in HTTP/2 back to HpackDecoder #12975

Merged

valfirst reviewed Nov 16, 2022

View reviewed changes

valfirst mentioned this pull request Nov 16, 2022

New HTTP header validation rules introduce backward compatibility issues #13005

Closed

dggcv mentioned this pull request Dec 21, 2022

netty is adding content-length=0 header in 204 response #13069

Closed

sergiitk mentioned this pull request Dec 27, 2022

CVE CVE-2022-41915: Incorrect range. Releases < 4.1.83.Final not affected #13084

Closed

wangbinhang mentioned this pull request Feb 13, 2023

CVE-2022-41881 spring-cloud/spring-cloud-gateway#2866

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject HTTP/2 header values with invalid characters #12760

Reject HTTP/2 header values with invalid characters #12760

chrisvest commented Aug 31, 2022

normanmaurer commented Sep 2, 2022

chrisvest commented Sep 2, 2022

idelpivnitskiy Sep 2, 2022

chrisvest Sep 2, 2022

idelpivnitskiy Sep 2, 2022 •

edited

idelpivnitskiy Sep 2, 2022

idelpivnitskiy Sep 2, 2022

chrisvest Sep 3, 2022

idelpivnitskiy Sep 7, 2022

normanmaurer commented Sep 5, 2022

chrisvest commented Sep 6, 2022

bodhili left a comment

bodhili left a comment

chrisvest commented Sep 15, 2022

idelpivnitskiy left a comment

chrisvest commented Sep 20, 2022

normanmaurer commented Sep 21, 2022

idelpivnitskiy left a comment

valfirst Nov 16, 2022

idelpivnitskiy Nov 16, 2022

Reject HTTP/2 header values with invalid characters #12760

Reject HTTP/2 header values with invalid characters #12760

Conversation

chrisvest commented Aug 31, 2022

normanmaurer commented Sep 2, 2022

chrisvest commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Sep 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Sep 5, 2022

chrisvest commented Sep 6, 2022

bodhili left a comment

Choose a reason for hiding this comment

bodhili left a comment

Choose a reason for hiding this comment

chrisvest commented Sep 15, 2022

idelpivnitskiy left a comment

Choose a reason for hiding this comment

chrisvest commented Sep 20, 2022

normanmaurer commented Sep 21, 2022

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Sep 2, 2022 •

edited