Closed Bug 339785 Opened 14 years ago Closed 14 years ago

scanner: memory exposure to scripts

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

Details

(Keywords: verified1.8.0.5, verified1.8.1, Whiteboard: [sg:critical?])

Attachments

(3 files, 2 obsolete files)

The code of GetXMLEntity from jsscan.c contains the following code:

    bp = ts->tokenbuf.base + offset;
....
    FastAppendChar(&ts->tokenbuf, ';');
    bytes = js_DeflateString(cx, bp + 1,
                             PTRDIFF(ts->tokenbuf.ptr, bp, jschar) - 2);
 
This does not take into account that FastAppendChar may grow and reallocate the buffer. When such reallocation happens, ts->tokenbuf.ptr - bp can be an arbitrary number. If js_DeflateString can allocate and access the corresponding region of memory, bytes would be C string corresponding to this arbitrary number of characters. Then this string would be embedded in the throw error object that the script can catch and read.

Since in principle this could allow to read various bytes that travels through arena area via catching from script the thrown error object, I mark this as security problem.
Attached file Test case for JS shell (obsolete) —
Note that depending on memory layout the test case may actually. If this is the case, try to change N in test() function to 127, 511 or other 2^p - 1 values.
Attached file Better test (for JS shell) (obsolete) —
This is a better version as in the previous case that "must_be_good" string could be bad in fact.
Attachment #223899 - Attachment is obsolete: true
Attached patch FixSplinter Review
The fix just remove appending of ';' to the buffer as it was not included in the error message in any case.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #223907 - Flags: review?(mrbkap)
Attachment #223907 - Flags: approval1.8.0.5?
Attachment #223907 - Flags: approval-branch-1.8.1?(brendan)
Attachment #223907 - Flags: review?(mrbkap) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I committed the fix
Attached file Even better test case
The test case now contains a loop to try various N itself.
Attachment #223901 - Attachment is obsolete: true
Comment on attachment 223907 [details] [diff] [review]
Fix

Wow, what was I thinking?  Thanks,

/be
Attachment #223907 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
I committed the patch to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Comment on attachment 223907 [details] [diff] [review]
Fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #223907 - Flags: approval1.8.0.5? → approval1.8.0.5+
Flags: blocking1.8.0.5+
I committed the patch from comment 3 to MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.5
Flags: in-testsuite+
verified fixed 1.8.0.5, 1.8.1, 1.9a1 20060622 builds on all platforms.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical?]
note to self: e4x/GC/regress-339785.js: TIMED OUT browser firefox-trunk-dbg-1.9a1_2006072314 windows, not seen on any other platform.
'Even better testcase' appeaers to be okay aviary. So I guess this is not present there. Igor?
(In reply to comment #13)
> 'Even better testcase' appeaers to be okay aviary. So I guess this is not
> present there. Igor?
> 

The bug was introduces during 1.8 development. So assuming "aviary" means branches with version < 1.8, then the answer is that the bug does not exist there.
Group: security
/cvsroot/mozilla/js/tests/e4x/GC/regress-339785.js,v  <--  regress-339785.js
You need to log in before you can comment on or make changes to this bug.