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?
There was a problem hiding this comment.
zlib's second parameter is destLen, not a source length. On entry it's the capacity of dest, on exit it's "the actual size of the uncompressed data". sourceLen is a plain uLong and input only, so uncompress() never reports how many compressed bytes it consumed.
So on dev that variable is already holding the uncompressed length in spite of its name, and offset += sourceSize is already advancing an offset into the compressed buffer by the decompressed size. The rename is the only thing that changed on that line, the arithmetic is identical to dev either way.
Your read of what it should be is right though. Getting the consumed input count needs uncompress2(), where sourceLen is in/out and comes back as the number of source bytes consumed. It only shows up for a compressed term nested inside another term, since a top level one has nothing after it to parse. I left the line alone because it's pre-existing and you'd flagged the outer parsing change as out of scope, but I can switch it to uncompress2() here if you'd rather have it fixed in one go.
Hi Mounika, just popping in as an independent researcher, I’ve taken the liberty to review the PR and disclosed vulnerability. During a triage with Brain, privately, it’s believed that this is no more than an informative report that doesn’t require a patch (as mentioned early that terms are not used by Discord nor D++ -- they're provided solely for completeness of the ETF API for Erlpack). In order for this vulnerability to be actually exploit this is issue, the API user would need to intentionally expose the ETF functions for the use of an external parser for WebSockets. As this would be outside of the scope of the project, the overall exploitability of the issue would be low due to it’s need for an intentional and targeted usage of the API outside of the bounds of the scope. To add additional clarity: if a user was to take this route, it would be an issue within the design control of the user's project and not the design control of D++. I will note, that this does contain the potential to spray the heap for secrets in the API if it was changed with improper access controls on the user’s behalf. Although the chance for obtaining said secrets remains low due to how ASLR operates for all programs and systems. I’ve provided a calculated CVSS 3.1 Vector for the reported vulnerability. If you have any questions, do feel free to ask and I can provide further clarification. CVSS 3.1 Vector: CVSS Base Score: 5.3 Edit: added additional clarity |
|
@Ashthetik that matches how I'd scope it. Reaching this needs the application to feed untrusted ETF into the parser itself, which the library never does on its own path, so informative rather than something worth an advisory. No argument on the scoring. @braindigitalis I answered the offset question in the thread above. Short version is that the |
|
Hi, we arent able to merge this until youve done the CLA and ticked the boxes in the PR description |
4 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