Added check for pseudo-headers in trailers by isaacrivriv · Pull Request #13603 · netty/netty

isaacrivriv · 2023-09-11T18:46:33Z

Motivation:
According to the RFC for HTTP2 on Section 8.1.2.1, 'Pseudo-header fields MUST NOT appear in trailers. Endpoints MUST treat a request or response that contains undefined or invalid pseudo-header fields as malformed (Section 8.1.2.6).'

Modifications:
Updated Http2Decoder to check trailers for Pseudo-Headers and treat them as invalid requests according to RFC 7540 Section 8.1.2.6

Result:
After this change, requests with Pseudo-Headers sent inside trailers will be treated as malformed and a stream error is thrown.
Fixes: #13602

Motivation: According to the RFC for HTTP2 on Section 8.1.2.1, 'Pseudo-header fields MUST NOT appear in trailers. Endpoints MUST treat a request or response that contains undefined or invalid pseudo-header fields as malformed (Section 8.1.2.6).' Modifications: Updated Http2Decoder to check trailers for Pseudo-Headers and treat them as invalid requests according to RFC 7540 Section 8.1.2.6 Result: After this change, requests with Pseudo-Headers sent inside trailers will be treated as malformed and a stream error is thrown

normanmaurer · 2023-09-12T06:47:58Z

@isaacrivriv did you sign our icla ? https://netty.io/s/icla ?

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

isaacrivriv · 2023-09-12T15:15:15Z

Yes it should have been signed already! I followed the steps in the developer guide and signed it before pushing the PR

Co-authored-by: Norman Maurer <norman_maurer@apple.com>

bryce-anderson

A few small suggestions but otherwise looks good!

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

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

ejona86

The (old) RFC 7540 has:

Pseudo-header fields defined for requests MUST NOT appear
in responses; pseudo-header fields defined for responses MUST NOT
appear in requests. Pseudo-header fields MUST NOT appear in
trailers.

Where are we doing that first part? I'd expect to see the mirror of this logic elsewhere. Do we just not have that logic anywhere?

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

normanmaurer · 2023-09-15T08:11:09Z

@isaacrivriv please let us know once this is ready for review again

isaacrivriv · 2023-10-03T19:44:43Z

Sorry for the delay, got back to this again this week. I'm not seeing code where Netty validates that sending pseudo-headers in a request/response. I created issue #13646 to follow up. I tried a quick test locally to add code to verify but a lot of the tests started failing. Will verify this but in the mean time I will push the clean up comments for this specific issue

isaacrivriv · 2023-10-03T19:52:42Z

Should be ready for review, tagged you all when you get a chance. Thanks!

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

As suggested, removed unused import, used iterator instead of header.names(), updated header name in tests for case where `randomString` can generate a string with a colon at the start, and updated comments to match newer RFC Co-authored-by: Bryce Anderson <bryce_anderson@apple.com>

isaacrivriv · 2023-10-05T20:14:41Z

Should be ready for review again

normanmaurer · 2023-10-06T06:14:24Z

@vietj PTAL as well

normanmaurer

one nit. ...

normanmaurer · 2023-10-06T06:15:54Z

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

+            } else {
+                // Need to check trailers don't contain pseudo headers. According to RFC 9113
+                // Trailers MUST NOT include pseudo-header fields (Section 8.3).
+                if (!headers.isEmpty()) {
+                    for (Iterator<Entry<CharSequence, CharSequence>> iterator =
+                        headers.iterator(); iterator.hasNext();) {
+                        if (Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat(iterator.next().getKey())) {
+                            throw streamError(stream.id(), PROTOCOL_ERROR,
+                                    "Found invalid Pseudo-Header in trailers");
+                        }
+                    }
+                }
            }


Suggested change

} else {

// Need to check trailers don't contain pseudo headers. According to RFC 9113

// Trailers MUST NOT include pseudo-header fields (Section 8.3).

if (!headers.isEmpty()) {

for (Iterator<Entry<CharSequence, CharSequence>> iterator =

headers.iterator(); iterator.hasNext();) {

if (Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat(iterator.next().getKey())) {

throw streamError(stream.id(), PROTOCOL_ERROR,

"Found invalid Pseudo-Header in trailers");

}

}

}

}

} else if (!headers.isEmpty()) {

// Need to check trailers don't contain pseudo headers. According to RFC 9113

// Trailers MUST NOT include pseudo-header fields (Section 8.3).

for (Iterator<Entry<CharSequence, CharSequence>> iterator =

headers.iterator(); iterator.hasNext();) {

if (Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat(iterator.next().getKey())) {

throw streamError(stream.id(), PROTOCOL_ERROR,

"Found invalid Pseudo-Header in trailers");

}

}

}

vietj · 2023-10-06T09:18:55Z

I cannot see any test that triggers the bug that is fixed here, only existing test modifications that do not affect it

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

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

connection decoder for verifying trailer headers.

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

normanmaurer · 2023-10-18T10:20:55Z

I did some cleanup and added a test case /cc @vietj

idelpivnitskiy

Thanks for the contribution!
One minor suggestion:

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

normanmaurer · 2023-10-19T08:13:48Z

@isaacrivriv thanks a lot!

Motivation: According to the RFC for HTTP2 on Section 8.1.2.1, 'Pseudo-header fields MUST NOT appear in trailers. Endpoints MUST treat a request or response that contains undefined or invalid pseudo-header fields as malformed (Section 8.1.2.6).' Modifications: Updated Http2Decoder to check trailers for Pseudo-Headers and treat them as invalid requests according to RFC 7540 Section 8.1.2.6 Result: After this change, requests with Pseudo-Headers sent inside trailers will be treated as malformed and a stream error is thrown. Fixes: #13602 --------- Co-authored-by: Norman Maurer <norman_maurer@apple.com> Co-authored-by: Bryce Anderson <bryce_anderson@apple.com>

pkoenig10 · 2023-11-13T17:15:28Z

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

                                "Multiple content-length headers received");
                    }
                }
+            } else if (validateHeaders && !headers.isEmpty()) {


This change broke users of grpc-netty.

grpc/grpc-java#10663

This is clearly a grpc-netty bug so there's no action required in Netty - I just wanted to comment here to make the maintainers aware of this issue.

chrisvest · 2024-01-19T23:16:13Z

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

+                // Need to check trailers don't contain pseudo headers. According to RFC 9113
+                // Trailers MUST NOT include pseudo-header fields (Section 8.3).
+                for (Iterator<Entry<CharSequence, CharSequence>> iterator =
+                    headers.iterator(); iterator.hasNext();) {


@ejona86 @pkoenig10 The usage of isEmpty was fixed in grpc/grpc-java#10663 and #13717, but we did not look closely enough at the problem, because on this line we also call the iterator() method on the Http2Headers, which AbstractHttp2Headers still does not implement: https://github.com/grpc/grpc-java/blob/75492c8b36c9392d94dfce09fc132ce88c94325e/netty/src/main/java/io/grpc/netty/AbstractHttp2Headers.java#L499

I see there's already a grpc-java issue on this: grpc/grpc-java#10837

This looks like it was supposed to be fixed in 1.55.0 with grpc/grpc-java#9979. The report I got was using 1.51.1, and grpc/grpc-java#10837 reports using 1.30.2, so this is probably not an issue for those who are on newer versions!

Oh that's great to hear. I saw the issue this morning as felt, "oh no." Glad it is already added. (Although obviously, better than recently being added would have been "always had been implemented"... We should re-run the benchmarks to see how much of a performance difference it makes to use our own impl, although benchmarking garbage creation is terribly difficult to get meaningful results.)

normanmaurer requested review from bryce-anderson, chrisvest and ejona86 September 12, 2023 06:47

normanmaurer requested changes Sep 12, 2023

View reviewed changes

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

Clean up reusing utility methods

5513a09

Co-authored-by: Norman Maurer <norman_maurer@apple.com>

bryce-anderson reviewed Sep 12, 2023

View reviewed changes

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java Outdated Show resolved Hide resolved

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

bryce-anderson approved these changes Sep 12, 2023

View reviewed changes

ejona86 reviewed Sep 12, 2023

View reviewed changes

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

isaacrivriv mentioned this pull request Oct 3, 2023

Validate pseudo-headers in requests/responses #13646

Open

isaacrivriv force-pushed the add-pseudo-header-trailer-check branch from a56dc1f to 7396915 Compare October 3, 2023 19:51

isaacrivriv requested review from bryce-anderson, ejona86 and normanmaurer October 3, 2023 19:52

isaacrivriv force-pushed the add-pseudo-header-trailer-check branch from 7396915 to d2a175d Compare October 3, 2023 20:09

normanmaurer approved these changes Oct 5, 2023

View reviewed changes

mostroverkhov suggested changes Oct 5, 2023

View reviewed changes

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

isaacrivriv force-pushed the add-pseudo-header-trailer-check branch from d2a175d to 8ecc840 Compare October 5, 2023 20:13

normanmaurer requested a review from idelpivnitskiy October 6, 2023 06:14

normanmaurer requested changes Oct 6, 2023

View reviewed changes

idelpivnitskiy reviewed Oct 6, 2023

View reviewed changes

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

normanmaurer requested changes Oct 12, 2023

View reviewed changes

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

isaacrivriv force-pushed the add-pseudo-header-trailer-check branch 2 times, most recently from 18f2f5a to 4d85799 Compare October 14, 2023 04:02

Made changes to add header validation flag in the default http2

4fe3207

connection decoder for verifying trailer headers.

isaacrivriv force-pushed the add-pseudo-header-trailer-check branch from 4d85799 to 4fe3207 Compare October 14, 2023 04:08

normanmaurer requested review from idelpivnitskiy, mostroverkhov and vietj October 18, 2023 09:19

normanmaurer requested changes Oct 18, 2023

View reviewed changes

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

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

normanmaurer added 2 commits October 18, 2023 11:22

Apply suggestions from code review

67c843c

Add testcase

74efd64

normanmaurer approved these changes Oct 18, 2023

View reviewed changes

idelpivnitskiy approved these changes Oct 18, 2023

View reviewed changes

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

Add pseudo header to exception message

b0b731c

mostroverkhov approved these changes Oct 19, 2023

View reviewed changes

normanmaurer added this to the 4.1.101.Final milestone Oct 19, 2023

normanmaurer merged commit 2657079 into netty:4.1 Oct 19, 2023

pkoenig10 reviewed Nov 13, 2023

View reviewed changes

idelpivnitskiy mentioned this pull request Nov 13, 2023

grpc-java is not compatible with the latest Netty version 4.1.101.Final grpc/grpc-java#10665

Closed

chrisvest reviewed Jan 19, 2024

View reviewed changes

isaacrivriv mentioned this pull request Feb 28, 2024

Add validation for pseudo-headers on requests/responses #13647

Draft

Uh oh!

Conversation

isaacrivriv commented Sep 11, 2023

Uh oh!

normanmaurer commented Sep 12, 2023

Uh oh!

Uh oh!

isaacrivriv commented Sep 12, 2023

Uh oh!

bryce-anderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

normanmaurer commented Sep 15, 2023

Uh oh!

isaacrivriv commented Oct 3, 2023

Uh oh!

isaacrivriv commented Oct 3, 2023

Uh oh!

Uh oh!

isaacrivriv commented Oct 5, 2023

Uh oh!

normanmaurer commented Oct 6, 2023

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

vietj commented Oct 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Oct 18, 2023

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

normanmaurer commented Oct 19, 2023

Uh oh!

pkoenig10 Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

chrisvest Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

chrisvest Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

chrisvest Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

ejona86 Jan 20, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ejona86 left a comment •

edited

Loading