Closed
Bug 368655
Opened 18 years ago
Closed 18 years ago
[FIX]Easy DoS by <img src="javascript:for(;;);"> even if javascript disabled
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: amotohiko_mozillafirebird, Assigned: bzbarsky)
References
()
Details
(Keywords: regression, verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:low dos])
Attachments
(2 files)
1.92 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
jst
:
review+
dveditz
:
approval1.8.1.2+
dveditz
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•18 years ago
|
||
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 | |
Updated•18 years ago
|
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
![]() |
Assignee | |
Comment 2•18 years ago
|
||
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 3•18 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•18 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 5•18 years ago
|
||
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?
![]() |
Assignee | |
Comment 6•18 years ago
|
||
I filed bug 368714 on the lack of a slow script dialog.
Updated•18 years ago
|
Keywords: regression
Whiteboard: [sg:low dos]
Updated•18 years ago
|
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Comment 7•18 years ago
|
||
Comment on attachment 253284 [details] [diff] [review]
Fix
We'll need a different patch for the branches, right? No nullprincipal back then.
Updated•18 years ago
|
Whiteboard: [sg:low dos] → [sg:low dos] need branch patch?
Comment 8•18 years ago
|
||
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?
![]() |
Assignee | |
Comment 9•18 years ago
|
||
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.
![]() |
Assignee | |
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
Comment on attachment 253559 [details] [diff] [review]
Branch patch
r=jst
Attachment #253559 -
Flags: review?(jst) → review+
Comment 12•18 years ago
|
||
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+
Updated•18 years ago
|
Whiteboard: [sg:low dos] need branch patch? → [sg:low dos] needs landing
Reporter | ||
Comment 14•18 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
![]() |
Assignee | |
Comment 15•18 years ago
|
||
> 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...
Comment 16•18 years ago
|
||
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.
![]() |
Assignee | |
Comment 17•18 years ago
|
||
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. :(
Comment 18•18 years ago
|
||
I should have noted in comment #16 that I also tried disabling Javascript in Options/Content. The script was not executed after disabling Javascript.
Updated•18 years ago
|
Whiteboard: [sg:low dos] needs landing → [sg:low dos] don't reveal until tbird ships
Comment 19•18 years ago
|
||
Thunderbird was not affected by this
Whiteboard: [sg:low dos] don't reveal until tbird ships → [sg:low dos]
Updated•18 years ago
|
Group: security
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•