470804 - crash [@ NS_GetInnermostURI - nsScriptSecurityManager::CheckLoadURIWithPrincipal]

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect)

Description

[Wayne Mery (:wsmwk)]

topcrash 10 crasher for Thunderbird 3.0b1

security:caps?

Doesn't appear much in nightlies, but does in the a1-3 and b1 releases. none have comments check the graph at http://crash-stats.mozilla.com/report/list?product=Thunderbird&version=Thunderbird%3A3.0a1&version=Thunderbird%3A3.0a2&version=Thunderbird%3A3.0a3&version=Thunderbird%3A3.0b1&platform=windows&query_search=signature&query_type=contains&query=NS_GetInnermostURI&date=&range_value=4&range_unit=weeks&do_query=1&signature=NS_GetInnermostURI%28nsIURI*%29

bp-d298ebd7-630a-46bb-9cce-0487b2081220

NS_GetInnermostURI	nsNetUtil.h:1412
nsScriptSecurityManager::CheckLoadURIWithPrincipal	caps/src/nsScriptSecurityManager.cpp:1321
NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2263
XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1477
js_Invoke	js/src/jsinterp.cpp:1313

Comment 1

[David :Bienvenu]

I'm wondering if there are any extensions installed - from what I can tell, no one calls CheckLoadURIWithPrincipal from js.

Comment 2

[Phil Ringnalda (:philor)]

Content context menu does, http://mxr.mozilla.org/comm-central/source/mail/base/content/nsContextMenu.js#291 (as does contentAreaUtils.js which viewSourceUtils.js includes, but I don't see it hitting that).

It's the sort of thing I often miss, but I don't quite see what you could pass to CheckLoadURIWithPrincipal to make it crash getting the innermost URI.

Comment 3

[David :Bienvenu]

ah, I was just looking in the comm-central mxr, which doesn't include toolkit, I guess.

I think passing null would make it crash. I think NS_PRECONDITION does nothing in release builds.

Comment 4

[timeless]

this is a bug in caps. the method is scriptable it needs to handle null

Comment 5

[timeless]

Attachment: throw early

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 354912 [details] [diff] [review]
throw early

sr=dveditz

leaving r?bz on in case he wants to look, but this is pretty self-obvious. But maybe handling null in NS_GetInnermostURI more gracefully would be worth the extra compare
  if (!nestedURI && uri)
before addref-ing.

You'd still need this change, of course, because if you survived the NS_GetInnermostURI call you'd crash on targetBaseURI->GetScheme(). This patch is both necessary and sufficient to fix this bug, additional changes to NS_GetInnermostURI can be a separate issue.

As long as you're in nsScriptSecurityManager, are there any other scriptable APIs that aren't sanity checking? checkLoadURI() doesn't check target, but falls through to this one. checkLoadURIStr/checkLoadURIStrWithPrincipal look OK, as does getCodebasePrincipal.

getChannelPrincipal() is scriptable and will crash on a null channel. "Don't do that" springs to mind, how robust do we want to be in the face of stupid XUL code?

isSystemPrincipal() can't handle a null return param, but that can't happen from script.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354912 [details] [diff] [review]
throw early

Looks good.

As far as NS_GetInnermostURI, I think we want to leave it as-is.  It's used in some performance-critical cases (and _does_ assert if passed unexpected input).

Comment 8

[Phil Ringnalda (:philor)]

Attachment: Crashtest

This does a nice job of crashing without the patch and not with, but I'm not really sure I'm not making bad assumptions about things like any exception being fine, or what principal I should pass to get as close as possible without crashing.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355011 [details] [diff] [review]
Crashtest

Any non-system principal will do, so just do a check that the principal is not the system principal?  r=bzbarsky with that change.

Comment 10

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/eb870a41e5cb
http://hg.mozilla.org/mozilla-central/rev/59c1cdb2cf7e

Comment 11

[timeless]

Comment on attachment 354912 [details] [diff] [review]
throw early

wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct, could we push to the branch?

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to comment #11)
> (From update of attachment 354912 [details] [diff] [review])
> wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct,
> could we push to the branch?

indeed, this needs to go out with the next beta, 3.0b3.
#9 crasher for 3.0b2+3.0b3pre

Comment 13

[Ludovic Hirlimann [:Usul]]

(In reply to comment #11)
> (From update of attachment 354912 [details] [diff] [review])
> wsm says this is a top crash on 1.9.1, it has a test and is tiny and correct,
> could we push to the branch?

Yes.

Comment 15

[Benjamin Smedberg]

Comment on attachment 354912 [details] [diff] [review]
throw early

a=me ... please land the test on the branch as well

Comment 16

[Mark Banner (:standard8)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/79b969c3d4fa
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b6c1a8033a4

Comment 17

[Wayne Mery (:wsmwk)]

v.fixed - don't see any crashes after 3.0b2 20090408014204