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

x/net/http2: sendWindowUpdate may send invalid window size increment #31170

Open
anmonteiro opened this issue Mar 31, 2019 · 1 comment
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@anmonteiro
Copy link

Disclaimer: I didn't run into an issue here, but I'm working on an HTTP/2 implementation and saw something that could possibly not be according to the spec.

The code linked below contains a loop that sends multiple WINDOW_UPDATE frames if the window size increment is higher than the allowed by the HTTP/2 spec. It then sends a last WINDOW_UPDATE frame after the loop has exited.

https://github.com/golang/net/blob/74de082e2cca95839e88aa0aeee5aadf6ce7710f/http2/server.go#L2189-L2193

However, because the check reads: for n >= maxUint31, the last frame sent could contain a window size increment of 0 if the initial n is a multiple of 2^31 - 1. That is not allowed by the HTTP/2 spec:

From: https://httpwg.org/specs/rfc7540.html#rfc.section.6.9

A receiver MUST treat the receipt of a WINDOW_UPDATE frame with an flow-control window increment of 0 as a stream error (Section 5.4.2) of type PROTOCOL_ERROR; errors on the connection flow-control window MUST be treated as a connection error (Section 5.4.1).

I think the fix here would be just to change the >= check to >. I suspect that the reason nobody ran into this before is that it's already extremely unlikely that all the conditions for this bug to manifest are actually met in the wild.

P.S.: The questions in the issue template didn't really apply to this issue. Apologies in advance if this is not very helpful.

@gopherbot gopherbot added this to the Unreleased milestone Mar 31, 2019
@anmonteiro anmonteiro changed the title x/net: HTTP/2 sendWindowUpdate may send invalid window size increment x/net/http2: sendWindowUpdate may send invalid window size increment Mar 31, 2019
@andybons
Copy link
Member

andybons commented Apr 1, 2019

@bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 1, 2019
@andybons andybons changed the title x/net/http2: sendWindowUpdate may send invalid window size increment x/net/http2: sendWindowUpdate may send invalid window size increment Apr 1, 2019
leighmcculloch added a commit to stellar/go that referenced this issue Sep 5, 2019
Use the default `net/http` transport which supports http2, instead of using `x/net/http2`.

At some point early on we adopted using `x/net/http2` as the http2 transport in our servers. It was probably necessary at the time, I'm not sure, but it's not necessary today unless we need full access to configure the transport. In our case we're initializing the http2 package with an empty config, so that alone suggests we are unnecessarily using that package. @bartekn noticed this while reviewing #1688 (comment) a dependency upgrade required for the shift from Dep to Modules.

The `net/http` package uses a vendored version of `x/net/http2` for http2. We only need to use `x/net/http2` directly if we want to customize it in ways that `net/http` doesn't expose, or if we want to use a later version of the http2 transport that has yet to be included in a release of Go. A huge downside of using the package directly though is that if critical fixes are made to it and included in a minor Go release we won't get them unless we manually update this package.

The godocs for the [net/http](https://golang.org/pkg/net/http/) package contain more details about the relationship between `net/http` and `x/net/http2`.

Close #1695

Summary of changes:

- Remove configuration of the http2 transport for Horizon.
- Remove configuration of the http2 transport for code shared by Federation, Bifrost, and Bridge.

Testing:

No tests that we have in the repository will identify any issues with this change, and any tests we could write would be complex and not helpful longterm, so I took a manual approach:

1. I ran Horizon with TLS enabled, with and without this change, and used curl to verify that the HTTP2 upgrade occurs successfully and results in a `HTTP/2 200` response, using this `curl` command:
   ```
   curl -vk --http2 https://localhost:8000
   ```

   This test was successful.

2. I ran Horizon with TLS enabled, with and without this change, and used [h2spec](https://github.com/summerwind/h2spec) to verify that the server before and after the change has the same spec passes and failures, using this `h2spec` command:
   ```
   h2spec http2 -t -k -h=localhost -p=8000
   ```

   This test was successful on most runs. There were definitely spec failures, but they were consistent before and after the change. In one run I noticed spec 6.9.3 part 3 (window updates) failed on `net/http`, however on re-runs it didn't fail. Investigating, I found a report in golang/go#31170 that `x/net/http2` may not be handling window updates correctly in some scenarios, and so it is likely that this issue is intermittent and present when using either and I just witnessed it only on one.

An identical change was also made to code shared by the federation, bifrost, and bridge servers. I ran the same tests above against the federation server with TLS enabled to verify that both code paths were unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants