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 frames are serialised one per TLS record #32924

Closed
mnot opened this issue Apr 19, 2020 · 7 comments
Closed

HTTP/2 frames are serialised one per TLS record #32924

mnot opened this issue Apr 19, 2020 · 7 comments
Labels
http2 Issues or PRs related to the http2 subsystem. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.

Comments

@mnot
Copy link
Contributor

mnot commented Apr 19, 2020

[ previously reported on HackerOne and judged to be low enough severity to open here ]

Summary: NodeJS's HTTP/2 client serialises each HTTP/2 request in a separate TLS record, exposing information to attackers performing traffic analysis.

Description: One of the design goals of HTTP/2 is to make traffic analysis more difficult, by multiplexing multiple requests on the same connection. However, NodeJS's HTTP/2 client side appears to send each request (HEADERS frame) in a separate TLS record, which makes this delineation -- and importantly, the sizes of the requests) -- apparent to observers on the network.

Steps To Reproduce:

  1. Run server. js in included tarball
  2. Configure Wireshark to use the included localhost-privkey.pem for the TLS private key, and ssl-keys.log for the keylog.
  3. Start Wireshark capturing on "host localhost and port 8443"
  4. Run client.js.
  5. Observe packet capture (annotated screenshot attached).

Impact: makes the lives of traffic analysis attackers easier. While NodeJS isn't a browser, it's used by a broad variety of applications, and I'd be a bit surprised if someone somewhere wasn't using it as a client in a situation where this wasn't a risk.

In particular, this information could be used to observe the differences in the sizes of requests, thereby allowing an attacker to "fingerprint" the activity more accurately.

This information could also be used by an attacker to more effectively perform attacks like CRIME, BEAST, etc.

Supporting Material/References:

See attached tarball containing a demo client and server - nodetest.zip

Annotated screenshot from Wireshark:

wireshark

@mnot
Copy link
Contributor Author

mnot commented Apr 19, 2020

Ping @jasnell

@devsnek devsnek added http2 Issues or PRs related to the http2 subsystem. https Issues or PRs related to the https subsystem. security Issues and PRs related to security. labels Apr 19, 2020
@jasnell
Copy link
Member

jasnell commented Apr 19, 2020

Thank you @mnot.

@nodejs/crypto @indutny ... The underlying challenge here is that the tls impl does not adequately coalesce writes on the same tls socket or provide a way to easily do so. At the http2 layer, these are just writes to the tls.Socket.

@mildsunrise
Copy link
Member

This is weird, #27861 solved this exact problem... This seems like a regression.

the tls impl does not adequately coalesce writes on the same tls socket or provide a way to easily do so

TLS sockets should support cork() / uncork() to force grouping buffers into a single segment 🤔

@jasnell
Copy link
Member

jasnell commented Apr 19, 2020

Interesting. Worth looking at again. It's possible that this ended up being resolved since the issue was reported. Likely worth retesting and/or backporting if it hasn't been. (I don't know offhand if it has or hasn't)

@mildsunrise
Copy link
Member

mildsunrise commented Apr 19, 2020

I did a quick test with the supplied code and was unable to reproduce the problem with the latest release, everything appears as one segment:

TCP packet showing one TLS segment with multiple HTTP/2 frames

So yes, it seems to be as you said. The PR landed at v12 and got backported to v10.

@mnot
Copy link
Contributor Author

mnot commented Apr 19, 2020

Great!

@mildsunrise mildsunrise added tls Issues and PRs related to the tls subsystem. and removed https Issues or PRs related to the https subsystem. labels Apr 20, 2020
@mildsunrise
Copy link
Member

mildsunrise commented Apr 20, 2020

#27861 was released on v12.4.0 at June and this bug was reported on September. I think it's safe to assume the bug is fixed, is there anything left to do before we can close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants