hpack

hpack copied to clipboard

This is to mitigate CVE-2019-9516, 0-Length Headers Leak. It will allow hpack users, such as hyper-h2, to close connections if this happens on the basis that the client is likely attempting a DoS attack.

(I'm happy to help maintain hpack if desired, I would release 3.1.0 with this fix at a minimum).

This patch isn’t actually necessary. If you try to mount the attack you’ll find it doesn’t work, because we don’t treat a name-value pair of “””,”” as having length 0 but instead as having length 32. We don’t need an extra defense here.

@Lukasa I see, this line is the key part then?

Whilst not necessary, is it still useful? Sending zero length headers isn't a legitimate thing for a client to do, with this hyper-h2 can respond like this if a client sends them.

@alexwlchan the CVE talks of both, but I've not seen an implementation that checks the value length. Maybe Lukasa can say more? My guess would be that it isn't infeasible to have a header field name without a value.

I wrote a patch to swift-nio-http2 that rejects zero length header field names on the grounds that RFC7230 forbids them in HTTP/1. It’s a reasonable patch; just not a security one. I have seen header fields without values in the wild before and expect to see them again.

I've reworded it to remove the security references and to rename as ZeroLengthHeaderNameError. With #170 I think the build will pass.

Current patch looks good.

Thinking some more, there's nothing to block encoding zero-length headers. This means we can encode headers that we'd promptly refuse to decode:

from hpack.hpack import Decoder, Encoder

e = Encoder()

encoded_bytes = e.encode({"": "nope"})
print(encoded_bytes)

d = Decoder()
decoded_bytes = d.decode(encoded_bytes)

How do we feel about that?

I can imagine there are use cases (e.g. mitmproxy) where being able to send zero-length header names is a useful feature – for example, to test that a server handles this attack correctly! But maybe it should be an error in regular usage, and you have to opt into allowing it?

Sounds sensible to me, could we split it into a second PR though (I think we'd need to discuss how to make it optional).