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}

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.

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.

Sounds like this could be a contentsink or parser issue.

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.

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.

xulifr3 is Bug 373911

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).

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.

Moving to A6 as we're outta time.

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.

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.

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

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.

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...

Oh, and I think that since this is trunk-only we should open it...

(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.

unload4e is Bug 384014