deflate bug with windowBits == 8?

[vinniefalco]

What's all this about?

    if (windowBits == 8) windowBits = 9;  /* until 256-byte window bug fixed */

https://github.com/madler/zlib/blob/master/deflate.c#L276

Does this mean that 256-byte windows are broken?

[madler]

Yes.

[vinniefalco]

Is this something that can be fixed? Does this mean that all existing implementations of WebSocket permessage-deflate are also broken (since they all use ZLib, and they can negotiate an 8-bit window)?

[madler]

Perhaps it can be fixed, but I have not tried. Nothing should be broken, since an 8-bit window zlib stream is never generated by zlib. If 8 is requested, a 9-bit window is used instead.

[vinniefalco]

Here's the possible issue, a WebSocket client requests an 8-bit window. The server (using ZLib, and unaware of line 276 in deflate.c) agrees, and responds that it will use an 8-bit window.

At this point the server will compress data, and that data may contain match offsets greater than 256 (since its really using a 9 bit window). The client will receive that data and get this error:
https://github.com/madler/zlib/blob/master/inflate.c#L1130

[madler]

In addition to compressing with a 512-byte window, 9 bits is put in the zlib header when 8 bits is requested. So zlib, or any compliant decompressor reading that header, will use at least a 512-byte window and there will be no error on decompression.

[vinniefalco]

That is certainly true if you are using a zlib header, but the WebSocket protocol specifies raw deflate streams and negotiates the window sizes separately in the HTTP request/response sequence which initiates the session:
https://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-28#section-7.1.2

I believe all existing WebSocket implementations that use ZLib are vulnerable to this bug. Take websocketpp for example:
https://github.com/zaphoyd/websocketpp/blob/master/websocketpp/extensions/permessage_deflate/enabled.hpp#L272

If a client requests an 8-bit window size, the server will call deflateInit2 with -8 for the window size (meaning, 256 byte window and raw deflate stream). But under the hood, ZLib will really be using a 512 byte window and cause emitted deflate blocks to break the client's window invariant.

[madler]

You are correct. I have committed 049578f to reject requests for a window size of 256 bytes when using raw or gzip encoding.

[vinniefalco]

Should this be reported in a security advisory?

[aytey]

Should this be reported in a security advisory?

What's the "exploit" here? Is it simply that things "fail to work correct" if 8 (rather than 9) is passed? Or can this lead to a crash on the server-side if the wrong window is used (therefore there's a potential DoS)?

I'd also say that this wasn't a security issue for zlib, but a security issue for, e.g., websocketpp.

[vinniefalco]

I don't know that it leads to a crash for a well written server, but for an application which uses raw deflate, if the application calls inflateInit with an 8 bit window size, and the other side sends deflate blocks with match offsets greater than 256, the application will get this error:
https://github.com/madler/zlib/blob/master/inflate.c#L1130

Whether or not its a denial of service depends on if the application handles errors correctly.

[aytey]

Quite, so this seems like an "API usage error".

There's no security vulnerability with zlib itself, but there are potential usage scenarios when something depending on zlib could call it in an incorrect way and get "unexpected" results.

For example, for websocketpp, what does the server do if it hits that error? I assume that it checks that return code for inflate and then "does something sensible" if this isn't as expected?

Honestly, I don't really understand if there's a wider implication; my assumption is though that there's nothing serious here (e.g., like Heartbleed) -- if zlib simply processed the mis-matched window and then returned non-controlled data, then there might be an issue. As it is, zlib appears to do do some sanity checking, and returns an error on "bad" data. Nothing untoward seems like it has happening there.

Without trying it, I presume that:

https://github.com/madler/zlib/blob/master/inflate.c#L1249

gets hit and returns Z_BUF_ERROR. It is up to the caller to check the return code and handle that sensibly.

If websocketpp doesn't handle that sensibly, and does something else, then there could be a security implication there.

[vinniefalco]

Your analysis is sound

[peter-gribanov]

I think it’s wrong to leave it as it is.
I understand that this breaks backward compatibility, but now it works inconsistently.
We can use a 256 byte window (in fact 512 byte) for compression, but we cannot use the same 256 byte window for decompression.

It's necessary to denide the use of a 256 byte window for both compression and decompression. This will be more correct since compression with this window is not supported.
Or, we need to change the window size from 256 bytes to 512 bytes in decompression, as in compression.