Closed Bug 311403 Opened 19 years ago Closed 19 years ago

chrome XBL method.eval + onerror handler allows arbitary code execution

Categories

(Core :: Security, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: sync2d, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8, verified1.7.13, Whiteboard: [sg:critical] privilege escalation)

Attachments

(5 files, 1 obsolete file)

You can use onerror event handler to circumvent eval()'s principal checks and can execute arbitary code with elevated privilege. See the testcase.
Attached file testcase (1.5+ only) (deleted) —
Works on: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1 Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051006 Firefox/1.6a1 Firefox 1.0.x is not exploitable because onerror handler on 1.0.x versions receive parameters different from exploitable versions.
Confirming. Might be fixed by the patch for bug 311025 -- Blake's testing
Blocks: sbb?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8rc1+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:high] privilege escalation
Whiteboard: [sg:high] privilege escalation → [sg:critical] privilege escalation
(In reply to comment #2) > Might be fixed by the patch for bug 311025 It isn't.
Attached patch maybe wrong patch (obsolete) (deleted) — Splinter Review
So the basic issue here is that when we compile the outer eval, we don't have a scripted caller, so we compile it with NULL principals. From then on, the scripted caller of the second eval also has no principals, so any security checks that we would do succeed trivially. This patch gives the outer eval the principals of the |this| object, which in the case of onerror is the content window.
Attachment #198747 - Flags: review?(brendan)
Comment on attachment 198747 [details] [diff] [review] maybe wrong patch better patch coming.
Attachment #198747 - Attachment is obsolete: true
Attachment #198747 - Flags: review?(brendan)
Attached patch better, maybe incomplete, patch (deleted) — Splinter Review
This has r=brendan in person. There are more places that need to be patched to deal correctly with null principals.
Assignee: dveditz → mrbkap
Status: NEW → ASSIGNED
Attachment #198762 - Flags: review+
I forgot to mention, I checked this in yesterday. I'll come up with a more complete patch soon.
Blocks: 311455
Attached patch fix script too (deleted) — Splinter Review
If it was possible to get one's hands on a new script object, compiled without any principals, this refuses to execute it. Note that this is a diff -w.
Attachment #198818 - Flags: review?(brendan)
Comment on attachment 198728 [details] testcase (1.5+ only) Note this testcase does not work in 1.0.x as mentioned in comment 1, but we still want this principal mishandling fixed on the 1.0 branch because there may be other ways to force the same issue.
Attachment #198728 - Attachment description: testcase → testcase (1.5+ only)
Attached patch follow-on trunk cleanup patch (deleted) — Splinter Review
This should get into the 1.8 branch along with substantive fixes, so we can diff trunk and branch without noise. /be
Attachment #198977 - Flags: review?(mrbkap)
Comment on attachment 198818 [details] [diff] [review] fix script too Yet another rigtheous fix. We should not have tolerated null principals in a setting (rt->findObjectPrincipals onn-null, e.g.) where principals are needed generally, this long. /be
Attachment #198818 - Flags: review?(brendan)
Attachment #198818 - Flags: review+
Attachment #198818 - Flags: approval1.8rc1?
Comment on attachment 198977 [details] [diff] [review] follow-on trunk cleanup patch r=mrbkap
Attachment #198977 - Flags: review?(mrbkap) → review+
I checked in the additional patch. Marking this bug as FIXED. Brendan, I wasn't sure if you wanted me to check your cleanup patch (attachment 198977 [details] [diff] [review]) in, so I left it alone.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(In reply to comment #13) > I checked in the additional patch. Marking this bug as FIXED. > > Brendan, I wasn't sure if you wanted me to check your cleanup patch (attachment > 198977 [edit]) in, so I left it alone. I checked that into the trunk yesterday. Just want to emphasize that we should sync trunk and 1.8 branch when this goes in for rc1. /be
Attachment #198818 - Flags: approval1.8rc1? → approval1.8rc1+
Depends on: 311952
(In reply to comment #9) > Note this testcase does not work in 1.0.x as mentioned in comment 1, but we > still want this principal mishandling fixed on the 1.0 branch because there may > be other ways to force the same issue. setTimeout can be used to force the same issue on 1.0.x. setTimeout(eval, 0, code, obj); When eval is called from timeout, scripted caller is null.
(In reply to comment #15) > setTimeout can be used to force the same issue on 1.0.x. that is bug 311455.
Checked in on MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Flags: testcase+
Attachment #198762 - Flags: approval1.7.13+
Attachment #198762 - Flags: approval-aviary1.0.8+
Comment on attachment 198818 [details] [diff] [review] fix script too aviary101/moz17 landing approval: a=dveditz for drivers. Please add the fixed1.7.13 and fixed-aviary1.0.8 keywords when landed.
Attachment #198818 - Flags: approval1.7.13+
Attachment #198818 - Flags: approval-aviary1.0.8+
Attachment #198977 - Flags: approval1.7.13+
Attachment #198977 - Flags: approval-aviary1.0.8+
This should be fixed on the 1.7 branches from some earlier commits I did today.
Status: RESOLVED → VERIFIED
verified with: Windows: Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215 Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8 Macintosh: Moz - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8 Fx - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8 Linux Moz - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060215
Group: security
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: