[FIX]Easy DoS by <img src="javascript:for(;;);"> even if javascript disabled

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Anbo Motohiko, Assigned: bz)

Tracking

({regression, verified1.8.0.10, verified1.8.1.2})

Trunk
mozilla1.9alpha1
regression, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low dos], URL)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8.1.2pre) Gecko/20070128 BonEcho/2.0.0.2pre
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8.1.2pre) Gecko/20070128 BonEcho/2.0.0.2pre

Firefox allows javascript scheme in src attribute of img element.
Current branch nightlies can nothing because of Bug 344890 (there are no window object), but the script runs even if I disable JavaScript.
(sorry, I can't test with trunk)

And if there is an infinite loop in this script, the "script runs slowly" dialog does not shown.


Tested and occurs on Firefox 2.0.0.2pre/20070128/WinMe and 1.5.0.10pre/20070129/WinMe.

Reproducible: Always

Steps to Reproduce:
1.Try URL above.
Actual Results:  
Firefox hangs.

Expected Results:  
At least, the "script runs slowly" dialog appears.

I want to ask Mozilla folks: why do you allow javascript scheme in src attribute of img element? According to Bug 179793 comment #0, this feature is for the compatibility with Netscape4 and Windows IE 6. Opera 9.10 allow this (but Opera doesn't hangs whole applicaiton), but I've heard of that Windows IE7 doesn't allow this. I think this /feature/ is not needed at all.
Oooh, yeah.  I bet this is a regression from bug 351370.  Patch coming up in a sec, I hope.
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Assignee: general → bzbarsky
Blocks: 351370
Flags: blocking1.9?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
OS: Windows ME → All
Hardware: PC → All
Summary: Easy DoS by <img src="javascript:for(;;);"> even if javascript disabled → [FIX]Easy DoS by <img src="javascript:for(;;);"> even if javascript disabled
Target Milestone: --- → mozilla1.9alpha
Version: 1.8 Branch → Trunk
Created attachment 253284 [details] [diff] [review]
Fix

The other option is to put that check in the sandbox code, but I'm not sure we want that...
Attachment #253284 - Flags: superreview?(jst)
Attachment #253284 - Flags: review?(jst)
Comment on attachment 253284 [details] [diff] [review]
Fix

This seems good to me. r+sr=jst
Attachment #253284 - Flags: superreview?(jst)
Attachment #253284 - Flags: superreview+
Attachment #253284 - Flags: review?(jst)
Attachment #253284 - Flags: review+
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Comment on attachment 253284 [details] [diff] [review]
Fix

This makes sure to not execute javascript: URIs if script is disabled for the page, even if we were going to execute them in a sandbox.
Attachment #253284 - Flags: approval1.8.1.2?
Attachment #253284 - Flags: approval1.8.0.10?
I filed bug 368714 on the lack of a slow script dialog.
Keywords: regression
Whiteboard: [sg:low dos]
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Comment on attachment 253284 [details] [diff] [review]
Fix

We'll need a different patch for the branches, right? No nullprincipal back then.
Whiteboard: [sg:low dos] → [sg:low dos] need branch patch?
Comment on attachment 253284 [details] [diff] [review]
Fix

Clearing flags as this patch relies on trunk-only components.
Attachment #253284 - Flags: approval1.8.1.2?
Attachment #253284 - Flags: approval1.8.0.10?
Sorry about the lag here.  I was kinda busy for a bit, and the answer for branches is not exactly trivial.

So all that this method wants out of a principal is to check whether it's chrome, and if not to check whether its URI is in a whitelist (about:, about:neterror, that sort of thing).

So I think it should be safe enough to use the about:blank principal.  I'll comment accordingly in the patch coming up.
Created attachment 253559 [details] [diff] [review]
Branch patch

This works for the branches.
Attachment #253559 - Flags: review?(jst)
Attachment #253559 - Flags: approval1.8.1.2?
Attachment #253559 - Flags: approval1.8.0.10?
Comment on attachment 253559 [details] [diff] [review]
Branch patch

r=jst
Attachment #253559 - Flags: review?(jst) → review+
Comment on attachment 253559 [details] [diff] [review]
Branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #253559 - Flags: approval1.8.1.2?
Attachment #253559 - Flags: approval1.8.1.2+
Attachment #253559 - Flags: approval1.8.0.10?
Attachment #253559 - Flags: approval1.8.0.10+
Whiteboard: [sg:low dos] need branch patch? → [sg:low dos] needs landing
Fixed on branches.
Keywords: fixed1.8.0.10, fixed1.8.1.2
(Reporter)

Comment 14

11 years ago
(In reply to comment #9)
> Sorry about the lag here.  I was kinda busy for a bit, and the answer for
> branches is not exactly trivial.
> 
> So all that this method wants out of a principal is to check whether it's
> chrome, and if not to check whether its URI is in a whitelist (about:,
> about:neterror, that sort of thing).
> 
> So I think it should be safe enough to use the about:blank principal.  I'll
> comment accordingly in the patch coming up.
> 

On 2.0.0.2pre 2007020204, when I open URI above, the "script runs slowly"
dialog appears (thank you for fixing Bug 368714).

When I disable JS on a page by using CAPS, the script still runs. Is this intended? I made a simple test for this.
http://www1.ttcn.ne.jp/amotohiko/temp/bug368655.html
> When I disable JS on a page by using CAPS

You mean the "javascript" policy for the page?

It's somewhat "intended" in that we don't really know who started the load.  So we don't know which CAPS policy is relevant.  I suppose we could check whether script is enabled in general (for the about:blank principal) and if it is also check for the principal of the target context.  I'm not sure whether that gets us very far (e.g. I think you can work around that by getting an image from a subframe, then loading a different page in the subframe, so the target context is no longer from your domain), and I'm not sure how commonly the CAPS thing is even used (unlike the general "disable javascript" option, which does work now).

The real way to fix that is to track principals for all javascript: loads and not execute the script at all if we don't have an origin principal.  That's what I'd like to aim to do for 1.9...
Verified on branches and trunk. 1509 and 2001 would hang on the url on top. Now after a few seconds you get: "A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete", and you get the option to Stop or Continue.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070206 BonEcho/2.0.0.2pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.10pre) Gecko/20070206 Firefox/1.5.0.10pre

Also tested on Mac, Linux 1.8.1.2 builds.
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Juan, that's the verification for bug 368714, not the patch that landed in this bug.  To verify this bug you need to disable JavaScript in your browser and then verify that this script doesn't execute at all....  Sorry about the way this got split up into two bugs.  :(
Keywords: verified1.8.0.10, verified1.8.1.2 → fixed1.8.0.10, fixed1.8.1.2
I should have noted in comment #16 that I also tried disabling Javascript in Options/Content. The script was not executed after disabling Javascript.
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Whiteboard: [sg:low dos] needs landing → [sg:low dos] don't reveal until tbird ships
Thunderbird was not affected by this
Whiteboard: [sg:low dos] don't reveal until tbird ships → [sg:low dos]
Group: security
Duplicate of this bug: 362049
You need to log in before you can comment on or make changes to this bug.