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? |
|
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. |
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 |
5 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