1263286 - CSP report sent for base-uri with no cause

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Aidan Woods]

Attachment: firefox-base-uri-report-1

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36

Steps to reproduce:

On this webpage: https://www.housingstudents.org.uk/
Firefox reports a violation of base-uri for the violating the url 'about:blank', when no base-uri is declared in the DOM.
Even when declaring a valid base-uri (i.e. the one specified in the CSP) the violation is still reported, see test page here:
https://www.housingstudents.org.uk/declare_base

First link, base tag absent. Second link, base tag declared in head of document.


Actual results:

Firefox reports a violation of base-uri  for the violating the url 'about:blank', when no base-uri is declared in the DOM.
Firefox reports a violation of base-uri  for the violating the url 'about:blank', when the correct base-uri is declared in the DOM.


Expected results:

No violation should be reported, as far as I can tell.

Comment 1

[Matthew N. [:MattN]]

Can you also check if this is a regression with the instructions from Bug 1262638 comment 4?

Comment 2

[Aidan Woods]

Attachment: firefox-base-uri-regression-bisection

You'll see there's only one 'bad' report, the earlier versions the tool presented didn't seem to understand the modern CSP spec – I skipped over a few, with nothing changing.

It would appear this issue dates back as far as 2014-10-21, if it is indeed a bug.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

I can reproduce the console error when visiting https://www.housingstudents.org.uk/; whether this is really a bug or not, I am not sure.

The site uses a report-only CSP and the following CSP in enforcement mode:
> report-uri https://housingstudents.report-uri.io/r/default/csp/enforce;
> default-src 'self';
> script-src 'self' 'unsafe-inline' https://ajax.googleapis.com/ https://www.google.com/ https://www.gstatic.com/;
> style-src 'self' 'unsafe-inline' https://fonts.googleapis.com/;
> img-src 'self' https://www.google.com/ data: blob:;
> font-src 'self' https://fonts.googleapis.com/ https://fonts.gstatic.com/;
> frame-src 'self' https://www.google.com/;
> base-uri 'self';
> form-action 'self';
> frame-ancestors 'none';
> block-all-mixed-content;
> upgrade-insecure-requests;

Boris, the site enforces a CSP of *base-uri 'self'* and also includes jquery which creates a new HTMLDocument (see stacktrace underneath, and also JS stack at the very end). When creating a new HTMLDocument we call nsDocument::SetBaseURI() with 'about:blank' which is then blocked by CSP. Should about:blank be exempt from CSP in that case? Please note that Chrome does not report an error to the console in that case. What do you think?

#0  0x00007fffe745c7dd in nsDocument::SetBaseURI (this=0x7fffd2a07000, aURI=0x7fffbcffa0d0) at /home/ckerschb/moz/mc/dom/base/nsDocument.cpp:3584
#1  0x00007fffe9086c3a in NS_NewDOMDocument (aInstancePtrResult=0x7fffffffa950, aNamespaceURI=..., aQualifiedName=..., aDoctype=0x7fffb94fea00, 
    aDocumentURI=0x7fffbcffa0d0, aBaseURI=0x7fffbcffa0d0, aPrincipal=0x7fffba81e5e0, aLoadedAsData=true, aEventObject=0x7fffbeb8c900, aFlavor=DocumentFlavorLegacyGuess)
    at /home/ckerschb/moz/mc/dom/xml/XMLDocument.cpp:148
#2  0x00007fffe7327e18 in mozilla::dom::DOMImplementation::CreateHTMLDocument (this=0x7fffbdceb1a0, aTitle=..., aDocument=0x7fffffffaa70, aDOMDocument=0x7fffffffaa80)
    at /home/ckerschb/moz/mc/dom/base/DOMImplementation.cpp:209
#3  0x00007fffe7328828 in mozilla::dom::DOMImplementation::CreateHTMLDocument (this=0x7fffbdceb1a0, aTitle=..., aRv=...)
    at /home/ckerschb/moz/mc/dom/base/DOMImplementation.cpp:258
#4  0x00007fffe80ac7c3 in mozilla::dom::DOMImplementationBinding::createHTMLDocument (cx=0x7fffc5038800, obj=..., self=0x7fffbdceb1a0, args=...)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/DOMImplementationBinding.cpp:185
#5  0x00007fffe838efc7 in mozilla::dom::GenericBindingMethod (cx=0x7fffc5038800, argc=1, vp=0x7fffda11d2b8) at /home/ckerschb/moz/mc/dom/bindings/BindingUtils.cpp:2778
#6  0x00007fffeb7c4a72 in js::CallJSNative (cx=0x7fffc5038800, native=0x7fffe838ed97 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/moz/mc/js/src/jscntxtinlines.h:235
#7  0x00007fffeb7a24f9 in js::InternalCallOrConstruct (cx=0x7fffc5038800, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:480
#8  0x00007fffeb7a2817 in InternalCall (cx=0x7fffc5038800, args=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:525
#9  0x00007fffeb7a2841 in js::CallFromStack (cx=0x7fffc5038800, args=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:531
#10 0x00007fffeb7b1b29 in Interpret (cx=0x7fffc5038800, state=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:2831
#11 0x00007fffeb7a2177 in js::RunScript (cx=0x7fffc5038800, state=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:426
#12 0x00007fffeb7a348b in js::ExecuteKernel (cx=0x7fffc5038800, script=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0)
    at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:704
#13 0x00007fffeb7a3755 in js::Execute (cx=0x7fffc5038800, script=..., scopeChainArg=..., rval=0x0) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:737
#14 0x00007fffeb562741 in Evaluate (cx=0x7fffc5038800, scope=..., staticScope=..., optionsArg=..., srcBuf=..., rval=...) at /home/ckerschb/moz/mc/js/src/jsapi.cpp:4487
#15 0x00007fffeb562947 in Evaluate (cx=0x7fffc5038800, scopeChain=..., optionsArg=..., srcBuf=..., rval=...) at /home/ckerschb/moz/mc/js/src/jsapi.cpp:4513
#16 0x00007fffeb563030 in JS::Evaluate (cx=0x7fffc5038800, scopeChain=..., optionsArg=..., srcBuf=..., rval=...) at /home/ckerschb/moz/mc/js/src/jsapi.cpp:4574
#17 0x00007fffe7504cb2 in nsJSUtils::EvaluateString (aCx=0x7fffc5038800, aSrcBuf=..., aEvaluationGlobal=..., aCompileOptions=..., aEvaluateOptions=..., aRetValue=..., 
    aOffThreadToken=0x0) at /home/ckerschb/moz/mc/dom/base/nsJSUtils.cpp:212
#18 0x00007fffe75050fa in nsJSUtils::EvaluateString (aCx=0x7fffc5038800, aSrcBuf=..., aEvaluationGlobal=..., aCompileOptions=..., aOffThreadToken=0x0)
    at /home/ckerschb/moz/mc/dom/base/nsJSUtils.cpp:280
#19 0x00007fffe7545ee0 in nsScriptLoader::EvaluateScript (this=0x7fffbdf6ebe0, aRequest=0x7fffb94d9f60, aSrcBuf=...)
    at /home/ckerschb/moz/mc/dom/base/nsScriptLoader.cpp:1142
#20 0x00007fffe7545237 in nsScriptLoader::ProcessRequest (this=0x7fffbdf6ebe0, aRequest=0x7fffb94d9f60) at /home/ckerschb/moz/mc/dom/base/nsScriptLoader.cpp:961
#21 0x00007fffe7543dad in nsScriptLoader::ProcessScriptElement (this=0x7fffbdf6ebe0, aElement=0x7fffbd062e78) at /home/ckerschb/moz/mc/dom/base/nsScriptLoader.cpp:648
#22 0x00007fffe75410a4 in nsScriptElement::MaybeProcessScript (this=0x7fffbd062e78) at /home/ckerschb/moz/mc/dom/base/nsScriptElement.cpp:142
#23 0x00007fffe6da0695 in nsIScriptElement::AttemptToExecute (this=0x7fffbd062e78) at /home/ckerschb/moz/mc/dom/base/nsIScriptElement.h:221
#24 0x00007fffe6d9d86f in nsHtml5TreeOpExecutor::RunScript (this=0x7fffbeb87c00, aScriptElement=0x7fffbd062de0)
    at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:666
---Type <return> to continue, or q <return> to quit---
#25 0x00007fffe6d9d052 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7fffbeb87c00) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:491
#26 0x00007fffe6da1898 in nsHtml5ExecutorReflusher::Run (this=0x7fffba272340) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:58
#27 0x00007fffe5b3fc30 in nsThread::ProcessNextEvent (this=0x7ffff6b7b500, aMayWait=false, aResult=0x7fffffffc5bf) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:994
#28 0x00007fffe5ba6439 in NS_ProcessNextEvent (aThread=0x7ffff6b7b500, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290
#29 0x00007fffe62fc910 in mozilla::ipc::MessagePump::Run (this=0x7fffe2789a00, aDelegate=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:98
#30 0x00007fffe625f74f in MessageLoop::RunInternal (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:230
#31 0x00007fffe625f6e4 in MessageLoop::RunHandler (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:223
#32 0x00007fffe625f6bd in MessageLoop::Run (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:203
#33 0x00007fffe9215c0c in nsBaseAppShell::Run (this=0x7fffda1b8080) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156
#34 0x00007fffea16cf45 in nsAppStartup::Run (this=0x7fffda17cb50) at /home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:281
#35 0x00007fffea205c52 in XREMain::XRE_mainRun (this=0x7fffffffc990) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4340
#36 0x00007fffea206019 in XREMain::XRE_main (this=0x7fffffffc990, argc=1, argv=0x7fffffffdea8, aAppData=0x7fffffffcbb0)
    at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4437
#37 0x00007fffea2062f6 in XRE_main (argc=1, argv=0x7fffffffdea8, aAppData=0x7fffffffcbb0, aFlags=0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4543
#38 0x000000000040586a in do_main (argc=1, argv=0x7fffffffdea8, envp=0x7fffffffdeb8, xreDirectory=0x7ffff6b59780) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:220
#39 0x0000000000405c74 in main (argc=1, argv=0x7fffffffdea8, envp=0x7fffffffdeb8) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:360
(gdb) call DumpJSStack()
[9786] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/ckerschb/moz/mc/netwerk/cache2/CacheFileMetadata.cpp, line 308
[Thread 0x7fffbcaff700 (LWP 9846) exited]
0 l.createHTMLDocument<() ["https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js":4]
    this = [object Window]
1 anonymous(a = [object Window]) ["https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js":4]
    this = [object Window]
2 anonymous(a = [object Window], b = [function]) ["https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js":2]
    this = [object Window]
3 <TOP LEVEL> ["https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js":2]

Comment 4

[Boris Zbarsky [:bzbarsky]]

So in spec terms I assume the relevant thing here is https://w3c.github.io/webappsec-csp/2/#directive-base-uri which says:

  Step 4 of the algorithm defined in HTML5 to obtain a document’s base URL (resolution of the
  href attribute of the base element) MUST be changed to:

    If the previous step was not successful, or the result of the previous step does not match
    the allowed base URLs for the protected resource, then the document base URL is fallback 
    base URL. Otherwise, it is the result of the previous step. 

So this is monkeypatching (why?  :() the algorithm at ... well, the thing it's monkeypatching does not exist.  The perils of monkeypatching.

Mike, can you and Anne figure out what the CSP spec is actually trying to do here and how it should do it?  Because as things stand I can't quite tell whether our behavior is correct or not...

But on the assumption that the intent is that the base URI from <base> is ignored in certain cases, why are we doing this in SetBaseURI as opposed to the <base> code?  Seems like doing the latter would be the right thing and not have the problem reported in this bug.

Comment 5

[Aidan Woods]

Per some testing I did beforehand, I've changed the Report-Only CSP on the provided link to the following for the base-uri directive: "base-uri 'self' about:blank 'about:blank';"

Neither of these variations on about:blank succeed in whitelisting it as a an accepted base-uri, and the report-only policy still sends a report.

I've left the enforced policy as is regarding the base-uri directive for comparison.

My point here being that even if about:blank is not an exception by default, there should be a way to whitelist it.

Comment 6

[Mike West]

No excuse, sorry. I should have upstreamed this years ago.

Clarifying in https://github.com/whatwg/html/pull/1053.

Comment 7

[Aidan Woods]

(In reply to Aidan Woods from comment #5)
> Per some testing I did beforehand, I've changed the Report-Only CSP on the
> provided link to the following for the base-uri directive: "base-uri 'self'
> about:blank 'about:blank';"
> 
> Neither of these variations on about:blank succeed in whitelisting it as a
> an accepted base-uri, and the report-only policy still sends a report.
> 
> I've left the enforced policy as is regarding the base-uri directive for
> comparison.
> 
> My point here being that even if about:blank is not an exception by default,
> there should be a way to whitelist it.

Dedicated test pages for declaring in CSP,
about:blank
https://www.housingstudents.org.uk/csp_declare_about:blank
and,
'about:blank'
https://www.housingstudents.org.uk/csp_declare_about:blank_quotes

each in addition to 'self' in the base-uri directive. As mentioned, neither appear to be a recognised syntax – so a report is still sent.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Mike, thank you.

Christoph, what's needed here is just implementing the spec Mike updated.  Specifically, you want to do https://html.spec.whatwg.org/multipage/semantics.html#set-the-frozen-base-url step 3, the bits that call into <https://w3c.github.io/webappsec-csp/document/#allow-base-for-document>.

In terms of our code, that means doing the CSP check in SetBaseURIUsingFirstBaseWithHref.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1263286_base_uri_csp_bug.patch

Thanks Boris, Mike et al.

I manually verified that there are no more CSP violations for about:blank on https://www.housingstudents.org.uk/. Further we have 2 tests within our codebase:
* test_base-uri.html
* test_null_baseuri.html
where the second one was testing that the actual base uri is non-null. Since we moved the CSP check from SetBaseURI() to SetBaseURIUsingFirstBaseWithHref() we could update:
> if (csp && newBaseURI) {
to
> if (csp) {

I don't think that newBaseURI can ever be null anymore, can it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8742153 [details] [diff] [review]
bug_1263286_base_uri_csp_bug.patch

newBaseURI can totally be null here.  If not having that null-check doesn't fail try, we should add a corresponding test.  Something like:

  <base href="http://foo:foo/">

should do the trick.

>+        if (!permitsBaseURI) {
>+          return;

That's not correct per spec, as far as I can tell.

Specifically, consider a document like so:

  <base href="some-URI-allowed-by-CSP">
  <base href="some-URI-not-allowed-by-CSP">

and then script does removeAttribute("href") no that first <base>.  Per spec, the base URI should become the fallback base URI, because the first <base> is ignored (no "href" attribute) and for the second <base> the "frozen base URL" is the fallback base URL, because it fails the CSP check.

But with your patch, we'll take the early return here, never make the SetBaseURI(nullptr) call, and keep our base URI as "some-URI-allowed-by-CSP".

Please add tests, ideally web platform tests, that check the behavior here.

Comment 11

[Romain Neutron]

Hi guys, I just stumbled upon this bug today and spotted the origin to a piece of code in jQuery that triggers this error:

```
( function() {
	var body = document.implementation.createHTMLDocument( "" ).body;
	body.innerHTML = "<form></form><form></form>";
	return body.childNodes.length === 2;
} )()
```

Here's a simple reproducer

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1263286_base_uri_csp_bug.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> But with your patch, we'll take the early return here, never make the
> SetBaseURI(nullptr) call, and keep our base URI as "some-URI-allowed-by-CSP".

Boris, it seems that currently nsDocument::SetBaseURI() even returned NS_OK if the csp check blocked the load. So, I suppose we never entered the if-branch within HTMLSharedElement.cpp to call SetBaseURI(nullptr) here [1]. With this patch we would call SetBaseURI(nullptr) in case the content policy check fails.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLSharedElement.cpp#182

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1263286_update_base_uri_tests.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> Specifically, consider a document like so:
> 
>   <base href="some-URI-allowed-by-CSP">
>   <base href="some-URI-not-allowed-by-CSP">
> 
> and then script does removeAttribute("href") no that first <base>.  Per
> spec, the base URI should become the fallback base URI, because the first
> <base> is ignored (no "href" attribute) and for the second <base> the
> "frozen base URL" is the fallback base URL, because it fails the CSP check.

I updated and extended our tests to include such a test where we call removeAttribute("href").

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=254db4ca3f9c

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Boris, it seems that currently nsDocument::SetBaseURI() even returned NS_OK if the csp
> check blocked the load. So, I suppose we never entered the if-branch within
> HTMLSharedElement.cpp to call SetBaseURI(nullptr) here 

Indeed.  So we were not following the spec to start with...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8744878 [details] [diff] [review]
bug_1263286_base_uri_csp_bug.patch

>+++ b/dom/base/nsDocument.cpp

With these changes, nsDocument::SetBaseURI always returns NS_OK.  Can we just change it, and nsIDocument::SetBaseURI, to return void?

>+      nsresult rv = aDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));
>+      NS_ENSURE_SUCCESS_VOID(rv);

Can rv actually fail?  Seems to me that it can't and we should assert that.  Failing that, we should SetBaseURI(nullptr) if it fails.

>+        rv = csp->Permits(newBaseURI, nsIContentSecurityPolicy::BASE_URI_DIRECTIVE,
>+                          true, &cspPermitsBaseURI);
>+        NS_ENSURE_SUCCESS_VOID(rv);

Again, on failure we should SetBaseURI(nullptr).

Maybe the simplest thing to do is to simply null out newBaseURI on these various failures and press on?   You could also set newBaseURI to null if !cspPermitsBaseURI, and then just have a single shared aDocument->SetBaseURI(newBaseURI) call...

r=me with the above fixed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8744879 [details] [diff] [review]
bug_1263286_update_base_uri_tests.patch

Thank you for updatin these!  It would be awesome if we could make these into web platform tests, but that's followup fodder.

Could you please also add a test like this:

  { csp: "",
    base1: "http://mochi.test",
    base2: "http://test1.example.com",
    action: "remove-base1",
    result: "http://test1.example.com",
    desc: "Removing first base should result in the second base"
  },

just to ensure that the CSP is really what's making the difference in the other remove-base1 test?

>+  ok(result.startsWith(tests[counter].result), tests[counter].desc);

It would be a bit easier to sort out what causes the test to fail if this line looked something like this:

  ok(result.startsWith(tests[counter].result), 
     `${tests[counter].desc}: Expected a base URI that starts with ${tests[counter].result} but got ${result}`);

or so.

r=me with the above fixed.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1263286_base_uri_csp_bug.patch

(In reply to Boris Zbarsky [:bz] from comment #16)
> just change it, and nsIDocument::SetBaseURI, to return void?

Yes, that makes sense.

> Maybe the simplest thing to do is to simply null out newBaseURI on these
> various failures and press on?   You could also set newBaseURI to null if
> !cspPermitsBaseURI, and then just have a single shared
> aDocument->SetBaseURI(newBaseURI) call...

Sure thing - incorporated those changes.

Carrying over r+ from bz.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1263286_update_base_uri_tests.patch

Thanks Boris, carrying over r+ from bz;

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b65e6c80cadb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8700157750

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b65e6c80cadb
https://hg.mozilla.org/mozilla-central/rev/6e8700157750

Comment 22

[Valentina Virlics [:vvalentina], Ecosystem QA]

Verified as fixed on FF 49(Build ID: 20160907153016) using Windows 7-x64.
Firefox browser (49) no longer reports a violation of base-uri  for the violating the url 'about:blank'.

Tested in addons-dev.allizom.org pages where this issue can be seen on Firefox Release.

Postfix screenshot:http://screencast.com/t/jHWk5ien1