368763 - (CVE-2007-0994) Arbitrary code execution by using javascript: url with <img> tag

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Attachment: testcase

This is very similar to bug 363594.  This affects only 1.8/1.8.0 branches.

When javascript: url is set by script, the access checks work properly.

  <img id="i">
  i.src = "javascript:...";

But, when javascript: url is set by <img> (or <link>, <style>) tag, the access
checks don't work properly.

  <img src="javascript:...">

Thus, sandboxed script can access xbl.method's clone parent and xbl compilation
scope to run arbitrary code with chrome privileges.

Note:
Disabling JavaScript does *not* block this exploit.  Because, javascript: url
can be evaluated even when JavaScript is disabled. (this issue seems to be
fixed in bug 368655, according to a CVS Log.)

Comment 1

[Brian Crowder]

Boris:  Have you had a chance to look at this yet?  Do you want me to take it?  If so, can you give me a hint where to start looking (if this works along the same lines as the other bug, I assume it is in browser code, not spidermonkey)?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I didn't even realize this bug existed -- none of the changes made on it so far are things I get bugmail for.  So I haven't looked, no.

As for where to start looking... At this point, the javascript: URI on branch is run in a sandbox with a made-up principal.  So I'd suspect the bug is elsewhere.

I'd probably start with a breakpoint in EvalInSandboxObject, when I get there set a breakpoint in nsScriptSecurityManager::CheckPropertyAccessImpl, wait until the property in question comes through and see what's going on....  Of course if that property never comes through CheckPropertyAccessImpl that'll narrow down things right there.  ;)

Comment 3

[moz_bug_r_a4]

Attachment: testcase 2 - reduced testcase

This does not work when JavaScript is disabled.  And, this is almost the same
as the testcase in bug 363594.

Comment 4

[Daniel Veditz [:dveditz]]

FF2.0/1.5.0.8 are fine, FF2.0.0.1 is affected (I'll assume 1.5.0.9 as well for now, but will check in a bit).

-> crowder based purely on enthusiasm, but maybe should be part of Boris's fix for bug 363594.

Comment 5

[Brian Crowder]

I'm not having a lot of luck with this, it's in code that I have not looked at much before, if at all.  Might be better owned by bz or someone else?

Comment 6

[Brian Crowder]

Adding mrbkap and Brendan.  If we want this done today, I am going to need help.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So finally had time to sit down and read this bug and more importantly the testcase.  Sounds to me like we just need to land the patch for bug 363594 on the branches, no?

Unfortunately, it's been a while since I built branch, so I won't be able to test for a bit.  But I'll post a patch if someone is willing to test.

Comment 8

[Brian Crowder]

I'll gladly test a patch.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Actually, it's not quite so simple, and I'm not as convinced that the fix for bug 363594 is correct.

The reason bug 363594 is trunk-only is that in that case we were evaluating the JS async, so the caller cx was no longer on the context stack.

All well and good, but fixing that by pushing the _target_ cx on the context stack (as we did) only works as long as the target is not itself privileged in some way.  If push comes to shove, we should just land the patch I'll attach (port of bug 363594 to branches) because it's better than nothing, but it's not perfect.

Ideally, we would push on the stack the same JSContext that the sandbox used for evaluation.  Failing that, we should probably create a vanilla context with the sandbox object as its global, etc, just like EvalInSandboxObject() ends up doing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch port of bug 363594

This will fix this specific testcase.... At least with this, getting UniversalXPConnect involves getting your hands on a window whose global has UniversalXPConnect and then doing the load in some object connected with that window....

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I considered just doing a JS_NewContext here.  But then we'd either need a bunch of code to make sure that bug 368714 stays fixed or we'd allow infinite loops in toString/valueOf to hang the app.  :(

I suppose we could more or less copy/paste the ContextHolder class into this code...

Comment 12

[Brian Crowder]

With this patch applied, neither of these testcases yields an alert window for me.

Comment 13

[Brian Crowder]

(that was testing with the 1.8 branch, btw)

Comment 14

[Brian Crowder]

Applied and built on 1.8.0 cleanly.  If we need this fixed today, I recommend we land Boris' patch.

Comment 15

[Brian Crowder]

Comment on attachment 254063 [details] [diff] [review]
Branch port of bug 363594

Nominating for branches since I can't find bz at the moment.

Comment 16

[Brian Crowder]

Comment on attachment 254063 [details] [diff] [review]
Branch port of bug 363594

Who's a good sr?

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 254063 [details] [diff] [review]
Branch port of bug 363594

sr=dveditz

Comment 18

[Jay Patel [:jay]]

Comment on attachment 254063 [details] [diff] [review]
Branch port of bug 363594

Approved for both branches, a=jay (Thanks for the quick turnaround on the patch bz/crowder!)

Comment 19

[Brian Crowder]

Checking in dom/src/jsurl/nsJSProtocolHandler.cpp;
/cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v  <--  nsJSProtocolHandler.cpp
new revision: 1.113.2.9; previous revision: 1.113.2.8
done

brian-crowders-computer:~/mozcvs/mozilla crowder$ cvs ci dom/src/jsurl/nsJSProtocolHandler.cppChecking in dom/src/jsurl/nsJSProtocolHandler.cpp;
/cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v  <--  nsJSProtocolHandler.cpp
new revision: 1.113.2.2.2.4; previous revision: 1.113.2.2.2.3
done

Comment 20

[Martijn Wargers (dead)]

Verified fixed on branches, with a 2007-01-31 build of the 1.8.1 branch, I see the alerts in the testcases.
With a 2007-02-07 build of the 1.8.1 branch, I don't get any alerts anymore, instead I get these error messages in the js console:
Error: uncaught exception: Permission denied to get property Function.__proto__
Error: uncaught exception: Permission denied to get property String.__parent__

Also verified fixed with the latest 1.8.0.x branch build.

Comment 21

[Daniel Veditz [:dveditz]]

regression between 1.5.0.8/2.0 and 1.5.0.9/2.0.0.1. Although fixed by the patch for trunk bug 363594, the blame for that regression was laid on trunk-only bug 351633. Would be nice to be able to identify the branch regressor.

Comment 22

[Martijn Wargers (dead)]

On Firefox2, this seems to have regressed between 2006-12-01 and 2006-12-03:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-01+03&maxdate=2006-12-03+06&cvsroot=%2Fcvsroot
There is a check-in by jst that looks like the likely cause, it doesn't have a bug number associated with it, but I think it's bug 351370.
So I think the branch patch for bug 351370 was the branch regressor.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Oh, yeah.  Absolutely.

That said, did anyone file a bug to fix this correctly?  See comment 9.  The bandaid we checked in is sort of ok, but we should really fix this correctly.

Comment 24

[Daniel Veditz [:dveditz]]

Because of bug 368655 this one affects Thunderbird and SeaMonkey mail even with JavaScript disabled by default. This could be used to construct an email worm. Do not un-hide this bug until Thunderbird 1.5.0.10 has been released for at least a week or so.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I filed bug 372075 on comment 9.  I think we should fix it for the next security release, but I'm not sure I'll be able to work on it...  :(

Comment 27

[Daniel Veditz [:dveditz]]

(In reply to comment #24)
> Because of bug 368655 this one affects Thunderbird and SeaMonkey mail even with
> JavaScript disabled by default.

I was wrong: Thunderbird will not run a javascript: url in an img.src whether javascript is enabled or not. I suspected the remote content filter (since that whitelists only http(s) and ftp) but they're not run even if you load remote images.

I have not tested SeaMonkey mail.

Comment 28

[moz_bug_r_a4]

By the way, is this a bugzilla's bug?

I couldn't see comment #24 until after comment #27 has been added.  And now I
cannot see comment #25.

Comment 29

[moz_bug_r_a4]

Attachment: screenshot

Comment 30

[Boris Zbarsky [:bzbarsky]]

Does bugzilla keep track of private comment flags?  I see nothing about them in the bug history, but comment 24 used to be private.  Now comment 25 is.  If someone twiddled the flags, that would make sense...

Comment 31

[chris hofmann]

pvnick is doing a bit of research on XSS and also gathering up bugs with security related test cases to help add to the regression/certification test suites.  adding him to the cc list in these...