Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 4 commits into
base: 4.1
Choose a base branch
from

Conversation

isaacrivriv
Copy link
Contributor

Motivation:
According to the (old) RFC 7540 on Section 8.1.2.1, 'Pseudo-header fields defined for requests MUST NOT appear in responses; pseudo-header fields defined for responses MUST NOT appear in requests.'

Modifications:
Updated Http2Encoder to check headers while writing for Pseudo-Headers and treat them as invalid requests or response if the Pseudo-Headers are not expected

Result:
After this change, requests/responses with invalid Pseudo-Headers will be treated as malformed and a stream error is thrown

Fixes #13646

@isaacrivriv isaacrivriv force-pushed the add-pseudo-header-write-validation branch from 34d8175 to 788b7b3 Compare October 3, 2023 20:13
@KingMob
Copy link

KingMob commented Oct 5, 2023

It might also be worth checking to ensure that mandatory pseudo-headers are present. Netty currently checks that the values are correct, but not that the headers themselves are present.

From RFC 9113 § 8.3.1 and 8.3.2:

All HTTP/2 requests MUST include exactly one valid value for the ":method", ":scheme", and ":path" pseudo-header fields, unless they are CONNECT requests. An HTTP request that omits mandatory pseudo-header fields is malformed (Section 8.1.1).

For HTTP/2 responses, a single ":status" pseudo-header field is defined that carries the HTTP status code field (see Section 15 of [HTTP]). This pseudo-header field MUST be included in all responses, including interim responses; otherwise, the response is malformed (Section 8.1.1).

That would fix part of #13630 (but not all of it)

*/
private boolean validatePseudoHeaders(Http2Headers headers) {
boolean isResponseHeaders = connection.isServer();
for (CharSequence name : headers.names()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers.names() copies names into new LinkedHashSet - for iteration It is sufficient to use Http2Headers.iterator() which does not do redundant copy (possibly guarded by isEmpty() check returning Collections.emptyIterator())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make the change, thanks for the comment!

Motivation:
According to the RFC 9113 on Section 8.3, 'Pseudo-header fields defined for requests MUST NOT appear in responses; pseudo-header fields defined for responses MUST NOT appear in requests.'

Modifications:
Updated Http2Encoder to check headers while writing for Pseudo-Headers and treat them as invalid requests or response if the Pseudo-Headers are not expected

Result:
After this change, requests/responses with invalid Pseudo-Headers will be treated as malformed and a stream error is thrown
private boolean validatePseudoHeaders(Http2Headers headers) {
boolean isResponseHeaders = connection.isServer();
for (CharSequence name : headers.names()) {
if (Http2Headers.PseudoHeaderName.isPseudoHeader(name) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super costy: try first to check if the prefix is a pseudoheader one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isPseudoHeader call eventually leads down to getPseudoHeaderName which has a check for the prefix so it should be okay.

boolean isResponseHeaders = connection.isServer();
for (CharSequence name : headers.names()) {
if (Http2Headers.PseudoHeaderName.isPseudoHeader(name) &&
(isResponseHeaders && Http2Headers.PseudoHeaderName.getPseudoHeader(name).isRequestOnly()) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make sense to modify the pseudo header names to have an array/list of all the ones request only/not request only. in order to perform the match upfront and eventually not perform any iteration/match, if not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iteration could be super costly especially with a large number of headers so I agree if we could skip the iteration I would. I'm thinking of an alternative to this by adding flags to the Http2 Header class itself and checking them when passing through the encoder which in my mind should be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually on second thought, the request pseudoHeaders are actually always on the front to be iterated so it makes it more efficient to just stop going through the headers once we stop reading pseudoHeaders. I'm making changes to follow this approach.

@normanmaurer
Copy link
Member

@isaacrivriv what is the status of this one ?

@isaacrivriv
Copy link
Contributor Author

I ran into some failures with I believe was the H2 Multiplexer set of tests which I couldn't figure out. I'll try to get back to this next week.

- Moved flags up from builders to establish header validation will take
  place
- Added check if headers are empty
- Updated where possible to use correct headers
- Switched off header validation on more complicated tests
- Added test for verifying validation is happening as expected
@isaacrivriv isaacrivriv force-pushed the add-pseudo-header-write-validation branch from 788b7b3 to eb3bddc Compare February 26, 2024 21:05
@isaacrivriv
Copy link
Contributor Author

isaacrivriv commented Feb 26, 2024

I pushed some changes which led me to passing all the h2 tests and included some tests to verify the functionality. I did have to disable most of the header validation for the tests since most of them used just empty headers or invalid headers to do a quick test of other functionality. I'm working on another approach that hopefully skips iterating while writing the headers to compare.

@isaacrivriv
Copy link
Contributor Author

I pushed out changes that should make this more efficient since the pseudo headers go first and there are already checks in place that confirm that custom pseudo headers can't be added as stated by RFC 9113 Section 8.3

Endpoints MUST NOT generate pseudo-header fields other than those defined in this document

The checks should happen pretty quickly. Following the approach in #13603 I also used the validate headers flag from the builders to decide if we would do the check or not.

The only checks that occur in this PR are to verify that request headers don't contain response pseudo headers and response headers don't contain request pseudo headers. I don't check if the required pseudo headers are found according to RFC 9113 Section 8.3.1 so this could be something to further build on this change

All HTTP/2 requests MUST include exactly one valid value for the ":method", ":scheme", and ":path" pseudo-header fields, unless they are CONNECT requests (Section 8.5). An HTTP request that omits mandatory pseudo-header fields is malformed (Section 8.1.1).

And also stated for the response as well

For HTTP/2 responses, a single ":status" pseudo-header field is defined that carries the HTTP status code field (see Section 15 of [HTTP]). This pseudo-header field MUST be included in all responses, including interim responses; otherwise, the response is malformed (Section 8.1.1).

- Stop verifying headers after the first non pseudoheader is found
- Don't fail on empty headers
- Clean up tests that didn't need changes
@isaacrivriv isaacrivriv force-pushed the add-pseudo-header-write-validation branch from 325fa09 to 122dd90 Compare February 28, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate pseudo-headers in requests/responses
5 participants