http://hg.mozilla.org/mozilla-central/diff/a2f468118868/dom/base/nsLocation.cpp (from bug 500328) causes the referer for location changes to always be the URI of the current (before change) location. This is a big departure from previous behavior, where the referrer used to be set to the URI of the principal for the initiating content script: generally, if the load happens cross-window (e.g. a parent frame sets the location on an inner frame or vice-versa), this URI is different from the current location. Furthermore, the URI set here gets passed later as the aRequestLocation to nsIContentPolicy::shouldLoad() implementations, which regard it as the origin of the load, and possibly cascades elsewhere (CSP?) where security or privacy checks are made. I hope HTML 5 does not dictate this new behavior, because it doesn't just cause empty frames, initialized with "about:blank", not to send out any Referer header when they're first loaded with a HTTP URL by setting their contentWindow.location, but, much worse, it can be exploited for arbitrary referrer spoofing. At any rate please let the chrome clients at least (content policies, mostly) keep receiving the old info (i.e. the URI of the window hosting the script which initiated the load) which is otherwise not easy to retrieve, especially for JavaScript implementers, and can have privacy and security implications. I'm setting the security bit because NoScript could be tricked by this bug into wrong ABE and XSS filtering decisions, and I don't know whether CSP, CORS or other security features which check the request origin are vulnerable. For sure I can tell this bug can be used to work-around the XPI installing whitelist, as exemplified here: http://evil.hackademix.net/test/fx4frames/ The same page shows how this bug can be exploited to cause cross-site information leakages (in the example, to know whether the user is logged or not in AMO) by forcing an informative redirected URL to be used as the referrer for a sniffing web page.

Er.. yes. That change was just wrong, looks like. Why was it made?

We want the referer to reflect the new URL after a pushState or a replaceState. Perhaps the right thing to do is to remember in the SHEntry whether it's the result of a push/replaceState. If so, we trust its URI; otherwise, we use the document's principal. Jonas pointed out that perhaps [1] should be comparing to the principal as well. [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9516

> We want the referer to reflect the new URL after a pushState or a replaceState. Which url? If page A calls pushState and then sets location on page B, should the referrer be page B, or the url page A pushed or something else?

The intent was that if page A calls pushState on itself and changes its url from x to x', and then if the user clicks a link on A, the referer should be x'. Apparently it's a bit more complicated than this, though. :)

I spoke to jst and mrbkap about this. Basically, our plan is: Where we used to return the URI of principal P, we now return the URI of the document associated with P. Of course, getting that document is a bit complicated.

Tested on http://evil.hackademix.net/test/fx4frames/ . I get a warning on the XPI bypass tests, and

... the AMO login sniff test informs me (incorrectly) that I'm logged into AMO. I assume this is the desired behavior, since that's what happens in FF 3.6. Giorgio, is this right?

(In reply to comment #8) > ... the AMO login sniff test informs me (incorrectly) that I'm logged into AMO. > I assume this is the desired behavior, since that's what happens in FF 3.6. > Giorgio, is this right? This seems correct. Do the referrer spoofing tests fail as well? Furthermore, could you try the following for me? 1. Install NoScript from http://noscript.net/getit 2. On browser restart, open a blank page (about:blank), click the NoScript status icon and select "Forbid about:blank" 3. Open the Error Console and check whether script evaluation works If it works (meaning that the origin for the javascript: URL to be opened in the console's inner frame is correctly reported to be a chrome URI, rather than about:blank), the content policies aspect of this bug should have been fixed as well.

(In reply to comment #9) > Do the referrer spoofing tests fail as well? Yes, FF prompts me before installing the extension in both tests.

(In reply to comment #10) > (In reply to comment #9) > > Do the referrer spoofing tests fail as well? > > Yes, FF prompts me before installing the extension in both tests. No, I meant what does happen if you click the "Spoof" button?

(In reply to comment #11) > No, I meant what does happen if you click the "Spoof" button? referer is correctly http://evil.hackademix.net/test/fx4frames/

(In reply to comment #12) > (In reply to comment #11) > > No, I meant what does happen if you click the "Spoof" button? > > referer is correctly http://evil.hackademix.net/test/fx4frames/ Fine, could you also check Error Console + NoScript with about:blank forbidden, per comment 9? Thanks

(In reply to comment #13) > Fine, could you also check Error Console + NoScript with about:blank forbidden, > per comment 9? Yep, was just doing that. Verified incorrect behavior without patch (scripts don't run from error console) on 9/5/10 nightly, and verified that scripts do run on patched build.

Any chance to have this in next beta?

I was going to land this afternoon, but the tree is currently closed to everything except b7 blockers, so I guess this won't make it until the next beta.

I wonder why this is not a blocker, since it affects both security and functionality. What does "blocking BetaN+" mean, exactly?

(In reply to comment #17) > What does "blocking BetaN+" mean, exactly? It means that this feature needs beta coverage, but isn't blocking any particular beta. In practice, this means it's blocking our final beta.

Not sure what to make of this test failure on Try: In chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_461743.js Assertion failure: cx->stack().contains(fp), at /builds/slave/tryserver-linux64-debug/build/js/src/jsdbgapi.cpp:1193 nsStringStats http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286601103.1286602488.13620.gz

Arbitrary referer spoofing defeats naive CSRF defenses that don't use a nonce/token. Also defeats deep-linking defenses sites may use.

Jlebar asked me to look at this. When the assertion is hit (100% reproducible for me), fp is null. Other callers guard on fp being null (e.g. nsContentUtils::CanAccessNativeAnon, AccessCheck::isSystemOnlyAccessPermitted) so this needs to do the same.

This is an unbitrotted patch with the guard. This passes the test. When we would have asserted, the callstack looks like xul.dll!GetURIFromFrameDocument(JSContext * cx, JSStackFrame * fp) Line 180 C++ xul.dll!nsLocation::CheckURL(nsIURI * aURI, nsIDocShellLoadInfo * * aLoadInfo) Line 264 + 0x14 bytes C++ xul.dll!nsLocation::SetURI(nsIURI * aURI, int aReplace) Line 362 + 0x24 bytes C++ xul.dll!nsLocation::SetHrefWithBase(const nsAString_internal & aHref, nsIURI * aBase, int aReplace) Line 646 + 0x3d bytes C++ xul.dll!nsLocation::SetHrefWithContext(JSContext * cx, const nsAString_internal & aHref, int aReplace) Line 593 + 0x19 bytes C++ > xul.dll!nsLocation::SetHref(const nsAString_internal & aHref) Line 562 + 0x15 bytes C++ xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 103 C++ xul.dll!CallMethodHelper::Invoke() Line 3065 + 0x1c bytes C++ xul.dll!CallMethodHelper::Call() Line 2332 + 0x8 bytes C++ xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode) Line 2296 + 0x16 bytes C++ xul.dll!XPCWrappedNative::SetAttribute(XPCCallContext & ccx) Line 2575 + 0xe bytes C++ xul.dll!XPC_WN_GetterSetter(JSContext * cx, unsigned int argc, jsval_layout * vp) Line 1670 + 0xc bytes C++ mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, js::Value *)* native, unsigned int argc, js::Value * vp) Line 652 + 0xf bytes C++ mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned int flags) Line 680 + 0x22 bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 855 + 0xf bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, JSObject * obj, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 955 + 0x2a bytes C++ mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * obj, jsval_layout fval, unsigned int argc, jsval_layout * argv, jsval_layout * rval) Line 4960 + 0x38 bytes C++ xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper, unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * nativeParams) Line 1694 + 0x35 bytes C++ xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * params) Line 572 C++ xul.dll!PrepareAndDispatch(nsXPTCStubBase * self, unsigned int methodIndex, unsigned int * args, unsigned int * stackBytesToPop) Line 114 + 0x21 bytes C++ xul.dll!SharedStub() Line 142 C++ xul.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct, nsIDOMEventListener * aListener, nsIDOMEvent * aDOMEvent, nsPIDOMEventTarget * aCurrentTarget, unsigned int aPhaseFlags, nsCxPusher * aPusher) Line 1112 + 0x12 bytes C++ xul.dll!nsEventListenerManager::HandleEventInternal(nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * * aDOMEvent, nsPIDOMEventTarget * aCurrentTarget, unsigned int aFlags, nsEventStatus * aEventStatus, nsCxPusher * aPusher) Line 1210 C++ xul.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * * aDOMEvent, nsPIDOMEventTarget * aCurrentTarget, unsigned int aFlags, nsEventStatus * aEventStatus, nsCxPusher * aPusher) Line 147 C++ xul.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor, unsigned int aFlags, int aMayHaveNewListenerManagers, nsCxPusher * aPusher) Line 213 C++ xul.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor, unsigned int aFlags, nsDispatchingCallback * aCallback, int aMayHaveNewListenerManagers, nsCxPusher * aPusher) Line 314 C++ xul.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget, nsPresContext * aPresContext, nsEvent * aEvent, nsIDOMEvent * aDOMEvent, nsEventStatus * aEventStatus, nsDispatchingCallback * aCallback, nsCOMArray<nsPIDOMEventTarget> * aTargets) Line 628 + 0x1e bytes C++ xul.dll!nsGenericElement::FireNodeInserted(nsIDocument * aDoc, nsINode * aParent, nsCOMArray<nsIContent> & aNodes) Line 3998 + 0x17 bytes C++ xul.dll!nsGenericHTMLElement::SetInnerHTML(const nsAString_internal & aInnerHTML) Line 767 + 0x11 bytes C++ xul.dll!nsGenericHTMLElementTearoff::SetInnerHTML(const nsAString_internal & aInnerHTML) Line 235 + 0x26 bytes C++ xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 103 C++ xul.dll!CallMethodHelper::Invoke() Line 3065 + 0x1c bytes C++ xul.dll!CallMethodHelper::Call() Line 2332 + 0x8 bytes C++ xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode) Line 2296 + 0x16 bytes C++ xul.dll!XPCWrappedNative::SetAttribute(XPCCallContext & ccx) Line 2575 + 0xe bytes C++ xul.dll!XPC_WN_GetterSetter(JSContext * cx, unsigned int argc, jsval_layout * vp) Line 1670 + 0xc bytes C++ mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, js::Value *)* native, unsigned int argc, js::Value * vp) Line 652 + 0xf bytes C++ mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned int flags) Line 680 + 0x22 bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 855 + 0xf bytes C++ mozjs.dll!js::JSProxyHandler::call(JSContext * cx, JSObject * proxy, unsigned int argc, js::Value * vp) Line 259 + 0x31 bytes C++ mozjs.dll!JSWrapper::call(JSContext * cx, JSObject * wrapper, unsigned int argc, js::Value * vp) Line 234 + 0x3e bytes C++ mozjs.dll!JSCrossCompartmentWrapper::call(JSContext * cx, JSObject * wrapper, unsigned int argc, js::Value * vp) Line 590 + 0x18 bytes C++ mozjs.dll!js::JSProxy::call(JSContext * cx, JSObject * proxy, unsigned int argc, js::Value * vp) Line 802 + 0x28 bytes C++ mozjs.dll!js::proxy_Call(JSContext * cx, unsigned int argc, js::Value * vp) Line 1027 + 0x15 bytes C++ mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, js::Value *)* native, unsigned int argc, js::Value * vp) Line 652 + 0xf bytes C++ mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned int flags) Line 673 + 0x22 bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 855 + 0xf bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, JSObject * obj, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 955 + 0x2a bytes C++ mozjs.dll!js::ExternalGetOrSet(JSContext * cx, JSObject * obj, jsid id, const js::Value & fval, JSAccessMode mode, unsigned int argc, js::Value * argv, js::Value * rval) Line 875 + 0x1d bytes C++ mozjs.dll!js::JSProxyHandler::set(JSContext * cx, JSObject * proxy, JSObject * receiver, jsid id, js::Value * vp) Line 171 + 0x2e bytes C++ xul.dll!xpc::XrayWrapper<JSCrossCompartmentWrapper,xpc::CrossCompartmentXray>::set(JSContext * cx, JSObject * wrapper, JSObject * receiver, jsid id, js::Value * vp) Line 733 C++ mozjs.dll!js::JSProxy::set(JSContext * cx, JSObject * proxy, JSObject * receiver, jsid id, js::Value * vp) Line 781 + 0x2c bytes C++ mozjs.dll!js::proxy_SetProperty(JSContext * cx, JSObject * obj, jsid id, js::Value * vp, int strict) Line 874 + 0x19 bytes C++ mozjs.dll!JSObject::setProperty(JSContext * cx, jsid id, js::Value * vp, int strict) Line 1072 + 0x2c bytes C++ mozjs.dll!js::Interpret(JSContext * cx, JSStackFrame * entryFrame, unsigned int inlineCallCount, JSInterpMode interpMode) Line 4417 + 0x2d bytes C++ mozjs.dll!js::RunScript(JSContext * cx, JSScript * script, JSStackFrame * fp) Line 637 + 0x11 bytes C++ mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned int flags) Line 740 + 0x11 bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 855 + 0xf bytes C++ mozjs.dll!js::ExternalInvoke(JSContext * cx, JSObject * obj, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval) Line 955 + 0x2a bytes C++ mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * obj, jsval_layout fval, unsigned int argc, jsval_layout * argv, jsval_layout * rval) Line 4960 + 0x38 bytes C++ xul.dll!nsJSContext::CallEventHandler(nsISupports * aTarget, void * aScope, void * aHandler, nsIArray * aargv, nsIVariant * * arv) Line 2164 + 0x2e bytes C++ xul.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout) Line 8916 + 0xb1 bytes C++ xul.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer, void * aClosure) Line 9264 C++ xul.dll!nsTimerImpl::Fire() Line 425 + 0xe bytes C++ xul.dll!nsTimerEvent::Run() Line 519 C++ xul.dll!nsThread::ProcessNextEvent(int mayWait, int * result) Line 547 + 0x19 bytes C++ xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, int mayWait) Line 250 + 0x16 bytes C++ xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 110 + 0xe bytes C++ xul.dll!MessageLoop::RunInternal() Line 220 C++ xul.dll!MessageLoop::RunHandler() Line 203 C++ xul.dll!MessageLoop::Run() Line 177 C++ xul.dll!nsBaseAppShell::Run() Line 186 C++ xul.dll!nsAppShell::Run() Line 243 + 0x9 bytes C++ xul.dll!nsAppStartup::Run() Line 191 + 0x1c bytes C++ xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData) Line 3682 + 0x25 bytes C++ firefox.exe!NS_internal_main(int argc, char * * argv) Line 158 + 0x12 bytes C++ firefox.exe!wmain(int argc, wchar_t * * argv) Line 129 + 0xd bytes C++ firefox.exe!__tmainCRTStartup() Line 552 + 0x19 bytes C firefox.exe!wmainCRTStartup() Line 371 C

So is the key part of that patch restoring the old behavior but adding the scheme check and using the new thing if the schemes match? Why is that scheme check reasonable?

Well, the key part is getting the document which owns the principal and using its URI as the referer. We then check to make sure that the scheme of that document matches the scheme of its principal --- if not, the assumption is that the document is something like a data: URI, so we don't want to use it as a referer.

Ah, I see. The scheme check could use a nice comment explaining why it's being done... For file:// URIs, the document URI and the principal URI both tend to be file:// but different URIs. Is that OK here?

Perhaps documents should simply know when their URI comes from pushState and what the URI would have been without pushState, and then compare the codebase URI for their principal to the URI they would have had. If they match, then use the pushstate uri... That seems like it would be less fragile. Also ok as a followup.

(In reply to comment #27) > Perhaps documents should simply know when their URI comes from pushState and > what the URI would have been without pushState, and then compare the codebase > URI for their principal to the URI they would have had. If they match, then > use the pushstate uri... I agree that seems much more straightforward.

Any news? It slipped unfixed in latest beta, didn't it?

Smaug, can you drive this one in? Justin's still in school and requested help.

fwiw, this slipped the last beta because the tree was in a perpetual state of closed to everything except b7 blockers. khuey's patch might be OK to land in its current state, modulo the comment requested in comment 26. I could fix it up as bz and I had been discussing as a followup; I should have time in two weeks (no school Nov 22 to Nov 26).

Nov 22 - 26 should be ok for this, I think.

Hm. Suppose document X (at URI x) creates a data: document Y (with URI y) in an iframe. Then X calls pushState on itself, taking it to URI x'. Now X navigates Y. The referrer URI should be x', right? But if we're using X's principal, we'll set the referrer to x. It seems like the right thing to do in general is to use the current URI of the document which owns the principal.

(In reply to comment 33) Oh, I understand. The code I wrote is doing just that, I think. I'll add a testcase for this behavior.

Just pushed to try.

You should probably null out mOriginalDocumentURI (which should be named that, imo) somewhere early enough in Reset() so that when we reset to the new document URI we set mOriginalDocumentURI too.

Not to pick nits, but how about mOriginalURI? mOriginalDocumentURI is ambiguously either m[OriginalDocument]URI or mOriginal[DocumentURI].

Hm. There's already an mOriginalDocument. But maybe it should be mOriginalDocumentURI, despite the ambiguity, because that has better parity with the other fields. This whole "document" prefix is weird. Why isn't it just mURI, mBaseURI, and mOriginalURI? The world may never know.

> Why isn't it just mURI, mBaseURI, and mOriginalURI? Fwiw, sold, as a followup. Check with jst/sicking, though? For now, mOriginalURI is good by me.

> JSObject* global = JS_GetGlobalForObject(cx, scope); was triggering a compartment mismatch assertion in the leaktests. I've changed how I'm getting the document to match some other code in nsLocation.cpp. It seems to work (and is green on try), but I admit that I don't fully understand the deep implications of the functions I'm calling. Patch in a moment.

Comment on attachment 492261 [details] [diff] [review] Patch v3 bz, does this look reasonable to you?

if this is going to make beta 8 seems like it needs to land soon. bz, can you check that last patch?

Your GetDocumentFromContext doesn't get the document corresponding to fp. It gets the document corresponding to the window we _started_ running script in; this matters in case of cross-window calls, right? The code in v2 seemed to get this right; what problems did you run into with that compartment-wise?

In trying to test comment 45, I ran into another problem. Suppose you: * Open page at location A. * Open a popup. * Call history.replaceState on the opener window and change the URI to B. * Navigate the popup from the opener by modifying popup.document.location. The referrer for the navigation should be B. But the principal for the opener has URI A, so by our logic here the referrer becomes A.

> But the principal for the opener has URI A, so by our logic here the referrer > becomes A. The principal is A, the original URI is A, so we should use the current uri, which is B, no?

(In reply to comment #47) > > But the principal for the opener has URI A, so by our logic here the referrer > > becomes A. > > The principal is A, the original URI is A, so we should use the current uri, > which is B, no? Oh yes; I managed to confuse myself with my own test. Comment 45 precisely describes the problem. I'll look into what was going wrong with my earlier patch.

Like v2, but with an added JSAutoEnterCompartment. Works on the leaktest which was previously failing. Will push to try.

Checking the return value of JSAutoEnterCompartment::enter.

This one looks good on try. bz, what do you think?

Using nsJSUtils::GetStaticScriptGlobal, per IRL conversation with bz.

Comment on attachment 497564 [details] [diff] [review] Patch v7 r=me