fix: Bound decode_compressed to the length zlib actually wrote - #1623
fix: Bound decode_compressed to the length zlib actually wrote#1623Mounika2456 wants to merge 2 commits into
Conversation
✅ Deploy Preview for dpp-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
github-actions
Bot
added
documentation
|
thanks for the pr but how are you triggering this in asan? discord doesn't ever send ett_compressed term values? |
| #include <dpp/unicode_emoji.h> | ||
| #include <dpp/restrequest.h> | ||
| #include <dpp/json.h> | ||
| #include <zlib.h> |
There was a problem hiding this comment.
unit tests should not directly import zlib
There was a problem hiding this comment.
Dropped it. The test now carries the deflate stream as a fixed byte array instead of compressing at runtime, so it only needs etf_parser.
| throw dpp::parse_exception(err_etf, "ETF compressed value: decompresson error"); | ||
| } | ||
| /* uncompress() was handed the rest of the buffer as its input, so the term ends there */ | ||
| offset = size; |
There was a problem hiding this comment.
this changes outer parsing behaviour and is not required, can you please justify why this is here?
There was a problem hiding this comment.
Fair point, that wasn't needed for the overread and I've put it back to offset += destinationSize, so outer parsing is untouched. What's left is just the bound on the inner parse: the vector is sized rather than reserved so its elements actually exist, and size comes from what uncompress() reported writing instead of the declared header value.
|
this seems to have come verbatim from discord/erlpack depository we based our erl parser on. https://github.com/discord/erlpack/blob/2a4c0e832f3cd4e07c92d4baec326a631ed50f59/js/decoder.h#L318 as such discord themselves have the same bug in their code and also never noticed it because their erl does not send compressed types (wouldn't make sense, because the entire stream can be zlib compressed) |
|
Not off a live gateway, no. I built the term by hand and passed it straight to That's an ett_compressed declaring 4096 bytes over a stream that inflates to five. On dev it comes back as a 4080 character string built out of the vector's uninitialized capacity; under ASAN it trips container-overflow in read_8_bits first, since reserve() leaves size() at 0 so every read is outside the container. And you're right about the lineage, the reserve() and the declared-size bound both came over from erlpack's decoder.h verbatim. Agreed it isn't reachable from the gateway if Discord never emits compressed terms, so this only matters for anything running untrusted ETF through the public parser. No objection if you'd rather close it on that basis. |
| const int ret = uncompress((Bytef*)outBuffer.data(), &destinationSize, (const unsigned char*)(data + offset), (uLong)(size - offset)); | ||
|
|
||
| offset += sourceSize; | ||
| offset += destinationSize; |
There was a problem hiding this comment.
shouldn't this be source size? destination size is changed by zlib. we are supposed to increment offset by the compressed content size yes?
3 participants
decode_compressed trusts the uncompressed size declared in the term header instead of the length uncompress() reports back, so a term that declares more than its zlib stream produces has the remainder decoded out of heap that was never written.
An ett_compressed term declaring 4096 bytes whose stream deflates to five (an ett_binary header announcing 4080 bytes of payload) comes back as a 4080 character string assembled from that memory; under ASAN the same input trips container-overflow in read_8_bits, since reserve() leaves the vector empty. zlibcontext::decompress already sizes its buffer and counts what inflate wrote, so this brings the term decoder in line with it. Covered by a new offline test that fails on master.
Code change checklist