123383 - JS URLs in many dialogs run as chrome

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

See also bugs 108399 and 88143. Here's yet another one: viewing Page Info on a
page containing <object data="javascript:....."></object> can run chrome.

Comment 1

[Bradley Baetz (:bbaetz)]

Is there a way for a js dialog to be able to run with the privilages of a
particular url?

Comment 2

[georgi - hopefully not receiving bugspam]

It is also bad that this works with javascript disabled.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Critical Moz1.0 fixes

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch for Element Properties dialog

Comment 5

[Bradley Baetz (:bbaetz)]

This is ugly. Is there an API for dialogs to drop privs for tthe current stack
frame + descendants?

This patch won't help with bug 127702, for example. Also, its not extendable to
other url schemes. Also, do you need to call checkloaduri or relatives?

Comment 6

[Mitchell Stoltz (not reading bugmail)]

> This is ugly. Is there an API for dialogs to drop privs for tthe current stack
> frame + descendants?
No, there isn't such an API, and to attempt that at this point would be both
uglier and more regression-prone. I'm trying to tread lightly at this stage in
the game. This patch fixes all versions of the Element Properties dialog
(images, links, etc.).

> This patch won't help with bug 127702, for example. 
This patch was never intended to fix bug 127702. The fix for that bug will be
handled in nsImapURL.

> Also, its not extendable to other url schemes.
Sure it is. All we have to do is add those schemes to the regexp. In any case,
it's much safer to list the schemes we allow than the schemes we deny. I really
don't think it matters if links to MyWeirdNewProtocolHandler: URLs aren't live
in the properties dialog - that's really not a big deal.

> Also, do you need to call checkloaduri or relatives?
Yes, you're probably right about that. I'll see about adding CheckLoadURI.

We can always revisit this after Moz1.0, but for now I want a quick, safe,
uncomplicated fix.

Comment 7

[John Morrison]

Mitch, r=jrgm for the patch given (but convert the tabs in the patch to 
spaces).

But, [a] from your last comment it sounds like you may have another patch 
pending, and [b] I can't read the other bugs noted on this page (restricted
access), so maybe an r= from someone with more knowledge of the security apis
might be better (bbaetz?)

Comment 8

[georgi - hopefully not receiving bugspam]

I agree with mstoltz that at the moment any patch which prevents the exploits is
much better than nothing.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch v2 - includes CheckLoadURI call

This is the same patch plus a call to CheckLoadURI, which we should have been
doing here anyway.

Comment 10

[John Morrison]

Comment on attachment 76652 [details] [diff] [review]
Patch v2 - includes CheckLoadURI call

r=jrgm, but fix the indentation (no tabs)

Comment 11

[Bradley Baetz (:bbaetz)]

Yeah, this is fine, but please do file a bug on the alternative cleaner
solution, unless you don't think that such a method would be useful in teh
general case.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 76652 [details] [diff] [review]
Patch v2 - includes CheckLoadURI call

sr=jst if you fix the bad indentation :-)

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 76652 [details] [diff] [review]
Patch v2 - includes CheckLoadURI call

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 14

[Mitchell Stoltz (not reading bugmail)]

The fix is in.

Comment 15

[bsharma]

Marking verified as per above developer comments.