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

Jetty response with an invalid HTTP2 packet if the client set the hpack table size as 0 #10805

Closed
terryyrliang opened this issue Oct 28, 2023 · 28 comments · Fixed by #11056, #11422 or #11445
Closed
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@terryyrliang
Copy link

terryyrliang commented Oct 28, 2023

Jetty version(s)

Jetty 12.0.1

Jetty Environment

ee10

Java version/vendor (use: java -version)
17.0.8.1

OS type/version
suse-150400.3.30.1-x8664, Linux kernel is 5.14.21-150400.24.63-default

Description

Our client application is sending the http2 request with hpack table size 0, and Jetty cannot response an valid http2 packet.

// In the java org.eclipse.jetty.http2.hpack.HpackEncoder
    public HpackEncoder()
    {
       _context = new HpackContext(0); // It should set the value from 0 to HpackContext.DEFAULT_MAX_TABLE_CAPACITY
       _debug = LOG.isDebugEnabled();
        setMaxTableCapacity(HpackContext.DEFAULT_MAX_TABLE_CAPACITY);
       setTableCapacity(HpackContext.DEFAULT_MAX_TABLE_CAPACITY);
    }

How to reproduce?
In our http2 client, set the hpack table size as 0 when sending HTTP2 setting to the Jetty server. And then we get an invalid http2 response.

@terryyrliang terryyrliang added the Bug For general bugs on Jetty side label Oct 28, 2023
@joakime
Copy link
Contributor

joakime commented Oct 28, 2023

There have been several fixes to the HPACK code in Jetty 12.0.2, can you retest on Jetty 12.0.2?

@terryyrliang
Copy link
Author

There have been several fixes to the HPACK code in Jetty 12.0.2, can you retest on Jetty 12.0.2?
-- Resend by replying.
Thanks @joakime reply, I just had a test based on the Jetty 12.0.1 and replacing the jar jetty-http2-hpack-12.0.1.jar with jetty-http2-hpack-12.0.2.jar (Forgive me using this way to test, because the package is very heavy...), but result is still failed..
Could you kindly confirm at which jar the fix located?

@terryyrliang
Copy link
Author

There have been several fixes to the HPACK code in Jetty 12.0.2, can you retest on Jetty 12.0.2?
-- Resend by replying.
Thanks @joakime reply, I just had a test based on the Jetty 12.0.1 and replacing the jar jetty-http2-hpack-12.0.1.jar with jetty-http2-hpack-12.0.2.jar (Forgive me using this way to test, because the package is very heavy...), but result is still failed..
Could you kindly confirm at which jar the fix located?

After further test with org.eclipse.jetty.http2.server_12.0.2.jar, org.eclipse.jetty.http2.hpack_12.0.2.jar and org.eclipse.jetty.http2.common_12.0.2.jar, still not work...

@terryyrliang
Copy link
Author

terryyrliang commented Oct 31, 2023

Hi @joakime , I finally found out the change is introduced by #9749 ,
// org.eclipse.jetty.http2.hpack.HpackEncoder
int tableCapacity = getTableCapacity(); // this is the peer table capacity, it's zero in my case
if (tableCapacity != _context.getMaxDynamicTableSize()) // here is zero too as the initial value in HpackEncoder
encodeMaxDynamicTableSize(buffer, tableCapacity); // both of above value is zero, then this line of code will NOT be executed

@terryyrliang terryyrliang changed the title Jetty response with an invalid HTTP2 response if the client set the hpack table size as 0 Jetty response with an invalid HTTP2 packet if the client set the hpack table size as 0 Oct 31, 2023
@terryyrliang
Copy link
Author

Hi @joakime , from the HTTP2 specification, the HTTP2 server should support SETTINGS_HEADER_TABLE_SIZE is 0. However, per our test, Jetty 12 does not support right now.

Reference is https://datatracker.ietf.org/doc/html/rfc7541#section-4.2
This mechanism can be used to completely clear entries from the
dynamic table by setting a maximum size of 0, which can subsequently
be restored.

@sbordet
Copy link
Contributor

sbordet commented Dec 13, 2023

@terryyrliang sorry but there is not enough information.
I wrote test cases for the encoder max table capacity to be 0 and they all pass.

Can you please detail exactly what is your configuration?

Better yet, can you provide a reproducible test case?

@sbordet
Copy link
Contributor

sbordet commented Dec 13, 2023

What client are you using? HttpClient or HTTP2Client?
What exactly do you configure on the client?
What exactly do you configure on the server?

@sbordet
Copy link
Contributor

sbordet commented Dec 14, 2023

@terryyrliang see tests in #11056, they pass cleanly, so we need to know what exactly you are doing.

@sbordet sbordet self-assigned this Dec 14, 2023
@sbordet
Copy link
Contributor

sbordet commented Dec 14, 2023

@terryyrliang I'm closing this.
Please comment or reopen if you have a reproducible case.

@sbordet sbordet closed this as completed Dec 14, 2023
@JohnNewComer
Copy link

Hi, sorry to jump in. I am also experiencing this issue, even though we are using 9.4.51 and 9.4.53.
In 9.4.51, http2 jetty response looks ok, while in 9.4.53, http2 jetty response has a compression_error.
I saw the difference between 9.4.51 and 9.4.53 is the "Header Blocker Fragment" in response.
In 9.4.51, "Header Block Fragment" starts from "Header table size update", while in 9.4.53, "Header Block Fragment" doesn't have this value. Is this a bit against the description in HPACK “This dynamic table size update MUST occur at the beginning of the first header block following the change to the dynamic table size.”
I listed the packet details between 9.4.51 and 9.4.53 below. Thanks in advance.
9.4.51 Jetty:
packet flow:
9 4 51 capture
request:
request(MAGIC,SETTINGS 0 ) 9 4 51
window update:
window update 9 4 51
response:
response(HEADERS 1  401 Unauthorized ) 9 4 51

9.4.53 Jetty:
packet flow:
9 4 53 capture
request:
request(MAGIC,SETTINGS 0 ) 9 4 53
window update:
window update 9 4 53
response:
response(HEADERS 0 , DATA 1 ) 9 4 53

@sbordet
Copy link
Contributor

sbordet commented Feb 19, 2024

@JohnNewComer Jetty 9.4.x is at End of Community Support (#7958) as well as Jetty 10/11 (#10485).
Can you please try with Jetty 12.0.x and report back if the problem still persists?

@joakime
Copy link
Contributor

joakime commented Feb 19, 2024

@JohnNewComer there is also a sponsored newer Jetty 9 release than you have tested - https://github.com/jetty/jetty.project/releases/tag/jetty-9.4.54.v20240208

@gregw
Copy link
Contributor

gregw commented Feb 20, 2024

@JohnNewComer if you can test the latest jetty-9, we will probably look at this a bit even though we are at EoCS, as it is plausible this is a problem introduced with changes we made in
jetty-9.4.53.v20231009 - 09 October 2023

  • 10546 backport jetty-http Huffman encoders/decoders from Jetty 10.0.x
  • 10573 backport hpack improvements from Jetty 10.0.x (CVE-2023-36478)
  • 10679 backport HTTP/2 rate control from Jetty 10.0.x (CVE-2023-44487)

@JohnNewComer
Copy link

Thanks for quick response.
I just have tried on 9.4.54.v20240208, sorry to say the issue still exists.
As for Jetty 12.0.x, I haven't tried yet and this may take a while for me.

@gregw
Copy link
Contributor

gregw commented Feb 20, 2024

@JohnNewComer See #11422, I've added our unit tests for zero sized dynamic tables to Jetty-9.4.x, which pass. Can you look at those tests and see if they are any different to your use case? Ideally we can adapt them to make a reproduction of your issue.

gregw added a commit that referenced this issue Feb 20, 2024
@gregw
Copy link
Contributor

gregw commented Feb 20, 2024

Captures look OK... although I don't really like the RSTs after the tests have finished, but you can see they happen after the response.

Screenshot from 2024-02-20 11-41-17

@JohnNewComer
Copy link

I tried this test case in some scenarios, the results make me unsure if it‘s a backward compatibility issue for jetty.
I have two wireshark(Version 2.6.2 and Version 4.2.3)
In wireshark 2.6.2:
For jetty 9.4.54, it couldn't decode Headers with Header Table Size 0 which is the issue as we are facing:
image
HEADERS[1]:
image

While for jetty 9.4.51, it could decode Headers with Header Table Size 0
image
HEADERS[1]:
image

In wireshark 4.2.3:
For jetty 9.4.54, it decodes Headers with Header Table Size 0 well which just like your captures:
image
HEADER[1]:
image

For jetty 9.4.51, it also decodes well:
image
HEADERS[1]:
image

Looks like there are some behavior changes in "Header Block Fragment" between 9.4.51 and 9.4.54, which couldn't be perfectly compatible to old wireshark 2.6.2(or other client).

@gregw
Copy link
Contributor

gregw commented Feb 21, 2024

@JohnNewComer because some of the CVEs these fixes for were across all implementations of HTTP/2, there were a lot of changes to HTTP/2 around this time on many platforms. Not all of these changes were CVE related, but were often "cleanups" or design changes that were forced by addressing the CVEs. So it is plausible that there are issues in clients/tools predating these changes.

So can you confirm if you do or don't have a real world problem with these releases? Do you have real clients having errors or is it just an old version of wireshark in your analysis that is the issue?

Also, what is the source of the 0 sized table? If it is your configuration, you could always try setting it to 1 or some other small number, which would have pretty much the same effect, but would avoid the zero case.

gregw added a commit that referenced this issue Feb 21, 2024
* Added test for #10805 Zero Dynamic Table

* fixed file header

* Added test for #10805 Zero Dynamic Table
@JohnNewComer
Copy link

Thanks for explanation.
Actually there is a real client has this problem, not just old wireshark. I took old wireshark as example cause I found it has similar behavior as the real client we using.
We found the real client couldn't handle the response after we using jetty to 9.4.54 with compression_error in GOAWAY.
As for the source of the 0 sized table, yes, it's our configuration.

@gregw
Copy link
Contributor

gregw commented Feb 22, 2024

@JohnNewComer We've gone over this again and we think we are doing something wrong in Jetty-9.4.54. We are incorrectly assuming the default table size is 0 (rather than 4096), so when you set zero we are not echoing the size back correctly. We believe we have this correct in jetty-10/11/12, but that something was broken in the back port to jetty-9.

We will fix this, but not sure of the time line as Jetty-9.4 is end-of-community life.... but as this might prevent upgrading to get CVE fixes, we can probably give this some priority (@lachlan-roberts can you help on this?)

@JohnNewComer to assist us fixing this precisely, it would be good if you could send us the full hex of each Header Block Fragment that you are seeing for 9.4.51 and 9.4.54; and for both the SETTINGS frame sent client->server and server->client.

@gregw
Copy link
Contributor

gregw commented Feb 22, 2024

I think we should also walk through the logic in 12 as well just to triple check it.

@JohnNewComer
Copy link

Thanks for checking. Below are the results for jetty 9.4.51 and 9.4.54.
Also, as you said there is correct in jetty 10/11/12, but I tried 11.0.20, same issue exists, would you please help confirm this?

Jetty 9.4.51:

SETTINGS frame:
client->server:

image
0000 00 00 1e 04 00 00 00 00 00 00 02 00 00 00 00 00
0010 01 00 00 00 00 00 08 00 00 00 00 00 03 7f ff ff
0020 ff 00 04 10 00 00 00

server->client:
image
0000 00 00 18 04 00 00 00 00 00 00 01 00 00 00 00 00
0010 03 00 00 00 80 00 04 00 08 00 00 00 06 00 00 20
0020 00

Header Blocker Fragment:
image
0000 20 48 82 68 01 61 96 d0 7a be 94 13 aa 69 3f 75
0010 04 01 32 a0 03 71 a0 5c 13 ca 62 d1 bf 0f 10 95
0020 1d 75 d0 62 0d 26 3d 4c 74 41 ea fb 24 e3 b1 05
0030 4c 16 a6 55 9e

Jetty 9.4.54:

SETTINGS frame:
client->server:

image
0000 00 00 1e 04 00 00 00 00 00 00 02 00 00 00 00 00
0010 01 00 00 00 00 00 08 00 00 00 00 00 03 7f ff ff
0020 ff 00 04 10 00 00 00

server->client:
image
0000 00 00 18 04 00 00 00 00 00 00 01 00 00 00 00 00
0010 03 00 00 00 80 00 04 00 08 00 00 00 06 00 00 20
0020 00

Header Blocker Fragment:
image
0000 48 82 68 01 61 97 dd 6d 5f 4a 05 e5 30 96 35 04
0010 01 34 a0 1c b8 cb b7 19 71 4c 5a 37 ff 0f 10 95
0020 1d 75 d0 62 0d 26 3d 4c 74 41 ea fb 24 e3 b1 05
0030 4c 16 a6 55 9e

@gregw
Copy link
Contributor

gregw commented Feb 23, 2024

@lachlan-roberts So in the above, the SETTINGS frames received and echoed by the server are identical for 9.4.51 and 9.4.54. The only difference is in the actual headings from sent for the response. In 9.4.51, the headings frame starts with 0x20, which is the encoding of the maxDynamic table size that is guarded by:

            if (tableCapacity != _context.getMaxDynamicTableSize())
                encodeMaxDynamicTableSize(buffer, tableCapacity);

So there is some difference in the initial table size, or the handling of the settings???

gregw added a commit that referenced this issue Feb 23, 2024
Set the correct default size for the table.
Always send the max table size on the first encode
@gregw gregw linked a pull request Feb 23, 2024 that will close this issue
@gregw
Copy link
Contributor

gregw commented Feb 23, 2024

@JohnNewComer I believe @lachlan-roberts has come up with a fix, and I've extended that with some defence in depth. See PR #11445
Is there any chance you can test that out?

gregw added a commit that referenced this issue Feb 26, 2024
* Added test for #10805 Zero Dynamic Table

* fixed file header

* Added test for #10805 Zero Dynamic Table

* Fix for #10805 Zero Dynamic Table

Set the correct default size for the table.
Always send the max table size on the first encode
@JohnNewComer
Copy link

Sorry for late reply, just got back to work. I have tested the fix just now, it works. Thanks for all the help.
By the way, how is the status of latest 11.0.x? Cause based on my test, it looks like the issue also exists in 11.0.20.

@gregw
Copy link
Contributor

gregw commented Feb 27, 2024

@JohnNewComer thanks for that confirmation. We will merge that fix forward through 10, 11 & 12 (slighty different in each). The joys of multiple versions/branches!

gregw added a commit that referenced this issue Feb 27, 2024
* Added test for #10805 Zero Dynamic Table

* fixed file header

* Added test for #10805 Zero Dynamic Table

* Fix for #10805 Zero Dynamic Table

Set the correct default size for the table.
Always send the max table size on the first encode
@JohnNewComer
Copy link

Cool, many thanks!

gregw added a commit that referenced this issue Feb 28, 2024
* Fix #10805 zero dynamic table (#11445)

* Added test for #10805 Zero Dynamic Table

* fixed file header

* Added test for #10805 Zero Dynamic Table

* Fix for #10805 Zero Dynamic Table

Set the correct default size for the table.
Always send the max table size on the first encode

* updated file header

Signed-off-by: gregw <gregw@webtide.com>

---------

Signed-off-by: gregw <gregw@webtide.com>
@gregw gregw closed this as completed in 686dd88 Feb 28, 2024
@terryyrliang
Copy link
Author

terryyrliang commented Apr 25, 2024

Hi @gregw , @sbordet and @lachlan-roberts , I use the the jetty 12.0.6 version to test, we still got error. And I finally find out an easy to reproduce the issue. Please have a look, because some rules, we cannot provide the log and tcpdump, I hope you can understand...

        <dependency>
            <groupId>io.vertx</groupId>
            <artifactId>vertx-web-client</artifactId>
            <version>4.5.7</version>
        </dependency>
        <dependency>
            <groupId>io.vertx</groupId>
            <artifactId>vertx-core</artifactId>
            <version>4.5.7</version>
        </dependency>
Http2Settings http2Settings = new Http2Settings();
        http2Settings.setHeaderTableSize(0);
        WebClientOptions options = new WebClientOptions()
                .setProtocolVersion(HttpVersion.HTTP_2)
                .setInitialSettings(http2Settings)
                .setHttp2ClearTextUpgrade(false)
                .setUserAgent("My-App/1.2.3");
        options.setKeepAlive(false);
        WebClient client = WebClient.create(Vertx.vertx(), options);
        client.get(8080, "fqdn", "/test")
                .send()
                .onSuccess(response -> {
                    System.out.println(response.body().toString());
                    response.headers().entries().forEach(i -> System.out.println(String.format("%s = %s", i.getKey(), i.getValue())));
                })
                .onFailure(Throwable::printStackTrace);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
Status: ✅ Done
Status: ✅ Done
6 participants