Closed Bug 123383 Opened 18 years ago Closed 18 years ago

JS URLs in many dialogs run as chrome

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: security-bugs, Assigned: security-bugs)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 88143, 108399
Is there a way for a js dialog to be able to run with the privilages of a
particular url?
It is also bad that this works with javascript disabled.
Critical Moz1.0 fixes
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0
Keywords: patch
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?
> 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.
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?)
I agree with mstoltz that at the moment any patch which prevents the exploits is
much better than nothing.
This is the same patch plus a call to CheckLoadURI, which we should have been
doing here anyway.
Attachment #76135 - Attachment is obsolete: true
Attachment #76652 - Flags: review+
Comment on attachment 76652 [details] [diff] [review]
Patch v2 - includes CheckLoadURI call

r=jrgm, but fix the indentation (no tabs)
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 on attachment 76652 [details] [diff] [review]
Patch v2 - includes CheckLoadURI call

sr=jst if you fix the bad indentation :-)
Attachment #76652 - Flags: superreview+
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
Attachment #76652 - Flags: approval+
The fix is in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Marking verified as per above developer comments.
Status: RESOLVED → VERIFIED
Group: security?
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.