[FIX]use after free with window.unload and javascript uri

RESOLVED FIXED in mozilla1.9alpha6

Status

()

defect
P1
normal
RESOLVED FIXED
13 years ago
4 months ago

People

(Reporter: guninski, Assigned: bzbarsky)

Tracking

({qawanted})

Trunk
mozilla1.9alpha6
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] 1.9-only?)

Attachments

(11 attachments, 1 obsolete attachment)

use after free with window.unload and javascript uri

window.onunload=...location.replace("javascript:twisted") causes use
after on exit if source is viewed.

in "view source" there is some garbage in the beginning.

seems trunk only.

classified as use after because 0xdadadada is dereferenced and
src/jsarena.h:170:#define JS_FREE_PATTERN         0xDA

WARNING: NS_ENSURE_TRUE(aParser) failed: file /opt/joro/firefox/mozilla/parser/htmlparser/src/nsDTDUtils.cpp, line 1062
###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file /opt/joro/firefox/mozilla/content/base/src/nsDocument.cpp, line 5445
###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file /opt/joro/firefox/mozilla/content/base/src/nsDocument.cpp, line 5445

#5  <signal handler called>
#6  0xb7d6e3bd in NS_LogCOMPtrRelease_P (aCOMPtr=0x8260afc, aObject=0x81a81c8)
    at /opt/joro/firefox/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1205
#7  0xb498a807 in ~nsCOMPtr (this=0x8260afc)
    at ../../dist/include/xpcom/nsCOMPtr.h:581
#8  0xb4daad1a in ~MapItem (this=0x8260af8)
    at /opt/joro/firefox/mozilla/content/xslt/src/base/txExpandedNameMap.h:129
#9  0xb4daad3d in nsTArrayElementTraits<txExpandedNameMap_base::MapItem>::Destruct (e=0x8260af8) at ../../../../dist/include/xpcom/nsTArray.h:198
#10 0xb4daad94 in nsTArray<txExpandedNameMap_base::MapItem>::DestructRange (
    this=0x8260ae4, start=0, count=2)
    at ../../../../dist/include/xpcom/nsTArray.h:699
#11 0xb4daae7a in nsTArray<txExpandedNameMap_base::MapItem>::RemoveElementsAt (
    this=0x8260ae4, start=0, count=2)
    at ../../../../dist/include/xpcom/nsTArray.h:550
#12 0xb4dd88b8 in nsTArray<txExpandedNameMap_base::MapItem>::Clear (

#6  0xb7d6e3bd in NS_LogCOMPtrRelease_P (aCOMPtr=0x8260afc, aObject=0x81a81c8)
    at /opt/joro/firefox/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1205
1205      void *object = dynamic_cast<void *>(aObject);
(gdb) p *aObject
$1 = {_vptr.nsISupports = 0xdadadada}
Whiteboard: [sg:critical] 1.9-only?
can't reproduce the crash.
(In reply to comment #1)
> can't reproduce the crash.
> 

debug trunk crash, optimized trunk doesn't crash.

in both cases "view source" shows garbage in the beginning - do you see garbage in "view source"?
Critical security bugs need to have an owner.  If you are not the correct person for this bug, please help us find someone else.  Thanks.
Assignee: nobody → Olli.Pettay
Haven't seen any garbage or crashes, but strange thing is that the next
loaded page gets vvvv from the document.write().
That looks *very* scary.
1.8 works quite differently. First document.write() updates the
page and an alert is show, location bar has then value
bugzilla.mozilla.org, though it never seems to load that.
So 1.8 doesn't do the right thing either.

Before bug 351633 trunk works almost like 1.8, though
after showing alert browser doesn't look like it is still loading something.
Blocks: 351633
Sounds like this could be a contentsink or parser issue.
Posted file testcase 2
Comment on attachment 257874 [details]
testcase 2

This testcase shows the regression from bug 351633.
Fun.  I'll look into it.  We should be running that in a sandbox, imo, and apparently aren't for some reason... Or something.

In any case, not a view-source bug for sure.
Assignee: Olli.Pettay → general
Component: View Source → DOM
Product: Firefox → Core
QA Contact: view.source → ian
Assignee: general → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha3
this has significant window spoofing potential, but can't make it so far. probably someone familiar with exploits should try to spoof windows via modification of this.
hm, "testcase 2" does at least window spoofing on trunk, but not on 2.0.0.2
(In reply to comment #4)
> Haven't seen any garbage or crashes, but strange thing is that the next
> loaded page gets vvvv from the document.write().
> That looks *very* scary.

this has signs of race condition.

when tested on localhost or 100Mb lan view source shows garbage in the beginning and and *does not show* the loaded page.

when tested on bugzilla, there is no garbage but the html is injected in the loaded page.

hm, testcase2a doesn't inject html with my network connectivity but produces error "document is not defined"
The 1.8 behavior in comment 4 is basically bug 371548.  So sounds like this bug is a trunk-only regression from the async javascript: changes?
the assertions in the description are not very romantic
So "testcase 2" is running against the same principal as the original page, so it gets to write stuff.  Not completely surprising.  I think we should disallow all location sets in unload -- see bug 371360 comment 13.
(In reply to comment #17)
>I think we should disallow all location sets in unload
That is something Safari and Konqueror and newer versions (apparently 
9.1+) of Opera seem to do, but IIRC not IE.

http://www.cs.helsinki.fi/u/pettay/moztests/unload_change_location.html
Click Google. If location can be set in unload, www.mozilla.org is loaded.
(In reply to comment #17)
> So "testcase 2" is running against the same principal as the original page, so
> it gets to write stuff.  Not completely surprising.  I think we should disallow
> all location sets in unload -- see bug 371360 comment 13.
> 

not quite sure "testcase 2" causes consistent behaviour. something is writing to |self| then the rest of |self| is an https html document. may be missing something.
(In reply to comment #17)
> 371360 comment 13.
>>>is to disallow all docshell operations and document.open() from inside the unload handler

it is conjectured that the less active content can do the less exploits ;)
but:
1. there are at least 2 unload handlers - body.unload and window.unload.
2. are you sure you can tell you are in unload handler - unload handler defining function, script, eval(), load xbl binding (some may be false) ?

> 1. there are at least 2 unload handlers - body.unload and window.unload.

Not important.

> 2. are you sure you can tell you are in unload handler

That's more of a problem.  You'd have to catch all async stuff started from unload....  We could do that with a content policy, probably.
on trunk unload is not the only problem.

xbl destructors changing location produce similar symtoms - crashes with scary register, almost sure use after free - check xulifr3.html.

new bug about xulifr3 ?
Yes, please.
For what it's worth, a simple fix for this particular regression is to just not flag the javascript: URI as allowed to execute if it starts loading during unload.  But there are the other issues Georgi points out (some of which are present before the async javascript: changes).
Depends on: CVE-2007-1095
the async bug is causing troubles and doesn't seem very urgent imho...
I'm not sure what comment 29 is saying.  Clarify, please?  Which "async bug"?
mean Bug 351633 – [FIX]Make javascript: URI execution async

the description of Bug 351633 is not scary, so consider it non urgent :)
Ah.  Well, the bugs it blocks are pretty scary.  It's not "urgent" like "must fix on branches right now" but it's necessary to have a sane security story in general.
(In reply to comment #32)
> Ah.  Well, the bugs it blocks are pretty scary. 

now branches seems safer than trunk w.r.t events imho

How so?  And note that trunk is a work in progress.
(In reply to comment #34)
> How so?  And note that trunk is a work in progress.
> 

from my point of view more xbl/onload event related testcases cause crash on trunk than on branches, may be wrong though.
Lets mark this as a blocker, if someone disagrees please speak up (especially you boris since it's assigned to you).

If you do object that please add a summary of what the problem is since the bug has gotten hard to follow at this point.
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9alpha3 → mozilla1.9alpha5
Moving to A6 as we're outta time.
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
I can't reproduce the crash, but in any case the patch in bug 371360 should help with it....  Please test with that patch?  It does fix smaug's testcase for sure.
Summary: use after free with window.unload and javascript uri → [FIX]use after free with window.unload and javascript uri
attachment "testcase 2" puts "...scary..." in the middle of bugzilla content.
hm, there are signs of race condition.

when loaded from localhost "unload4b" shows garbage in viewsource which is a bad sign.
view source on better unload4b shows garbage:

ÿþ<�h�1�>�v�i�e�
> attachment "testcase 2" puts "...scary..." in the middle of bugzilla content.

I'm well aware.  My patch fixes it.

Looking at unload4b.
I can't reproduce the problem with unload4b either... but in any case, the patch in bug 371360 would stop it.

That said, I'm going to post a belt-and-braces patch here to make sure that javascript: can't seriously misbehave.
Posted patch Said patchSplinter Review
Attachment #267296 - Flags: superreview?(cbiesinger)
Attachment #267296 - Flags: review?(jst)
Really need to do some testing of various javascript: loads while pages are loading at the same time... I did test that using a javascript: URI as the URI for a frame/iframe works.
bz, is |addBinding| expected to work?

trying to abuse it for cross domain stuff with no luck.
Expected to work in what sense?  In general, it works when used properly.
(In reply to comment #49)
> Expected to work in what sense?  In general, it works when used properly.
> 

thanks. i tried to inject anonymous content in other window for spoofing - didn't work. but inserting anonymous content in the same document also didn't work, so thought |addBinding| may not be fully implemented.
Comment on attachment 267296 [details] [diff] [review]
Said patch

r=jst
Attachment #267296 - Flags: review?(jst) → review+
window.open("","_self") in unload handler has similar side effects to location=""
Should also be blocked by the patch in bug 371360.  But feel free to post a testcase so I can test...  It's really hard to get anywhere without reproducible testcases.  :(
(In reply to comment #53)
> Should also be blocked by the patch in bug 371360.  But feel free to post a
> testcase so I can test...  It's really hard to get anywhere without
> reproducible testcases.  :(
> 

unload4d is a testcase. there is no error when started from bugzilla. when started  from another host js console gives error "document is not defined". managed to inject html in localhost (when started from localhost and loading a sleeping cgi) - similar to the bugzilla injection.
unload4e - |window| is not defined on reloading + assertions
if loaded in iframe unload4e kind of messes with chrome - |reload| button throws js exception while location bar shows http uri.
bz, let me know if this bug needs forking - imo the testcases are not very scary, but are not very romantic either.
4d worksforme in my build.  I have no idea what 4e is even testing.
Attachment #267296 - Flags: superreview?(cbiesinger) → superreview+
Checked in.  This should fix the trunk regression.  If there is a further problem on the branch, that's bug 371360.

Not really sure how to test this...
Flags: in-testsuite?
Oh, and I think that since this is trunk-only we should open it...
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #59)
> 4d worksforme in my build.  I have no idea what 4e is even testing.
> 

confirming 4d is fixed.

when 4e is opened, the link is clicked and then |reload| is clicked there is a strange error in js console:

Error: window is not defined
Source File: {window.open(location,"_self")}
Line: 1

just pointing the strange error |window is not defined|
Yeah, not really sure what's up there, to be honest.  I'd file a separate bug on it.
(In reply to comment #63)
> Yeah, not really sure what's up there, to be honest.  I'd file a separate bug
> on it.
> 

4e looks like a race - the |alert| seems important.
Depends on: 385092
Group: security
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Keywords: qawanted
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.