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

HTTP/2 improvements for CVE-2023-36478 #9749

Merged
merged 9 commits into from May 27, 2023

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented May 8, 2023

Addressing CVE-2023-36478

  • Implemented a few required error handlings.
  • Changed Parser.init() to directly take the listener, rather than wrapping it. The reason for this change was to be able to reconfigure the Parser upon receiving a SETTINGS frame.

* Implemented a few required error handlings.
* Changed `Parser.init()` to directly take the listener, rather than wrapping it.
  The reason for this change was to be able to reconfigure the Parser upon receiving a SETTINGS frame.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from gregw and lorban May 8, 2023 16:38
gregw
gregw previously requested changes May 15, 2023
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

This is only a partial review, as I have lots of questions and am -0 on somethings.
Perhaps we need to hangout, but it would be good if the comments/javadoc was good enough so that explanation of these things was not needed.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from gregw May 15, 2023 17:21
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@gregw gregw dismissed their stale review May 19, 2023 19:59

My comments were addressed

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Initially setting the encoder and decoder max table capacity at the default of 4096, as per spec.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented May 26, 2023

@lachlan-roberts I think there is an inherent race that cannot be fixed unless delaying the use of API until both initial SETTINGS frame have been acknowledged.

For example, the client has no special configuration, all defaults, so client.decoder.maxTableCapacity=4096.
The client sends the initial SETTINGS frame that may or may not contain that value.
Then the client can send a request that may or may not contain the instruction to resize the table.

Meanwhile the server has not sent the initial SETTINGS frame, but it is configured so that its decoder has server.decoder.maxTableCapacity=0.
The server does not read until it has sent the initial SETTINGS frame; when it reads, it reads the client's SETTINGS, which applies to its encoder, and then it reads the request that has been sent using the default values, which may contain a resize instruction that is invalid for the server decoder configuration.

The specification says, section 3.4 of RFC 9113:

To avoid unnecessary latency, clients are permitted to send additional frames to the server immediately after sending the client connection preface, without waiting to receive the server connection preface.

So it is clear that the spec allows for this window to happen.
Waiting for the SETTINGS reply does not help, as the client should wait for the server initial SETTINGS, which is not mandatory as the above paragraph says.

So yes it's racy, but it is allowed, and it has been fixed in HTTP/3 where the table initial sizes are 0 and all communication about table sizes happens as parts of HEADERS frames.

However, I filed #9801 about this.

Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

Looks good. All my comments have been addressed.

We have some pre-existing race conditions which will be address separately as part of #9801 so I am happy to go ahead and merge this one.

@sbordet sbordet merged commit 420ec7c into jetty-10.0.x May 27, 2023
4 checks passed
@sbordet sbordet deleted the fix/jetty-10-http2-improvements branch May 27, 2023 22:23
@joakime joakime changed the title HTTP/2 improvements. HTTP/2 improvements for CVE-2023-36478 Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants