-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
* 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 <[email protected]>
There was a problem hiding this 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.
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Outdated
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Outdated
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Outdated
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Show resolved
Hide resolved
jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Connection.java
Outdated
Show resolved
Hide resolved
jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/SettingsTest.java
Show resolved
Hide resolved
jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java
Show resolved
Hide resolved
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Show resolved
Hide resolved
...erver/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
.../http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
Outdated
Show resolved
Hide resolved
Initially setting the encoder and decoder max table capacity at the default of 4096, as per spec. Signed-off-by: Simone Bordet <[email protected]>
@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 Meanwhile the server has not sent the initial SETTINGS frame, but it is configured so that its decoder has The specification says, section 3.4 of RFC 9113:
So it is clear that the spec allows for this window to happen. 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. |
There was a problem hiding this 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.
Addressing CVE-2023-36478
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.