Buffer: fix use-after-free on detach during argument coercion - #1106
Open
basavaraj-sm05 wants to merge 1 commit into
Open
Buffer: fix use-after-free on detach during argument coercion#1106basavaraj-sm05 wants to merge 1 commit into
basavaraj-sm05 wants to merge 1 commit into
Conversation
Several Buffer.prototype methods fetched the underlying ArrayBuffer's
data pointer before finishing the coercion of their own offset,
length, or value arguments. In QuickJS (qjs_buffer.c) this affected
the read/write integer and float accessors, write(), copy(),
compare(), indexOf(), and toString(); in njs (njs_buffer.c) it
affected the shared range helper behind compare()/equals() and the
source side of copy().
Any of those arguments can be an object with a valueOf, and the
callback it triggers can detach the very buffer whose pointer was
already captured (via ArrayBuffer.prototype.transfer() or
$262.detachArrayBuffer()). The following bounds check and memory
access then ran against that stale pointer: a heap-use-after-free in
QuickJS, since transfer() frees the backing store, and a read through
a NULL-based address in njs, since its detach only clears the data
pointer. Both are reachable from ordinary script, e.g.
buf.compare(other, {valueOf(){other.buffer.transfer(); return 0}}).
The fix resolves every user-controllable argument first and only
looks up the buffer pointer immediately before the bounds check and
the actual read or write, so a detach mid-conversion now surfaces as
the ordinary "detached buffer" error. Added a regression suite that
detaches the buffer from a valueOf during each affected call.
|
🎉 Thank you for your contribution! It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 Open Source Software (OSS) project. Please kindly read the F5 CLA and reply on a new comment with the following text to agree: I have hereby read the F5 CLA and agree to its terms You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Several Buffer.prototype methods, in both engines, fetch the underlying ArrayBuffer's raw data pointer before finishing the coercion of their own offset/length/value arguments. In QuickJS (qjs_buffer.c) this affects the read/write integer and float accessors, write(), copy(), compare(), indexOf(), and toString(); in njs (njs_buffer.c) it affects the shared range helper behind compare()/equals() and the source side of copy(). Any of those arguments can be an object with a valueOf, and the callback it triggers can detach the very buffer whose pointer was already captured, via ArrayBuffer.prototype.transfer() or, in a test context, $262.detachArrayBuffer(). The subsequent bounds check and memory access then run against that stale pointer: a straightforward heap-use-after-free in QuickJS, since transfer() frees the backing store, and a read through a NULL-based address in njs, since its own detach only clears the data pointer. Both are reachable from ordinary script, for example
buf.compare(other, {valueOf(){other.buffer.transfer(); return 0}}). I found the QuickJS side first, and since njs's own read/write-int functions already re-fetch the buffer after coercion, went looking for the same gap in njs's remaining Buffer methods and found it in the compare/copy range helper too. The fix in both files is the same: resolve every user-controllable argument first, and only look up the buffer pointer immediately before the bounds check and the actual read or write, so a detach mid-conversion now surfaces as the ordinary "detached buffer" error instead of touching freed or invalid memory. I added a regression suite to buffer.t.js that detaches the buffer from inside a valueOf during each affected call and asserts a TypeError is thrown.Checklist
Before creating a PR, run through this checklist and mark each as complete:
CONTRIBUTINGdocument