Closed Bug 1100181 Opened 5 years ago Closed 3 years ago

(CSP 1.1) look at connect-src directive when submitting pings

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: francois, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [CSP 1.1], [domsecurity-active])

Attachments

(2 files, 4 obsolete files)

As per the fetch spec (https://fetch.spec.whatwg.org/#requests), hyperlink auditing, the ping attribute, is covered by the connect-src directive.

Pings are currently not covered by any CSP directive in Firefox.

Note: pings are disabled by default and need to be enabled via the browser.send_pings pref.
Attached file Test page
Here's a test page that demonstrates that should and shouldn't be blocked.
Whiteboard: [CSP 1.1]
Attached patch CSP code and mochitest (obsolete) — Splinter Review
This is the code I removed from bug 529697.
Assignee: nobody → francois
Status: NEW → ASSIGNED
Attachment #8523643 - Flags: review?(mozilla)
Comment on attachment 8523643 [details] [diff] [review]
CSP code and mochitest

Review of attachment 8523643 [details] [diff] [review]:
-----------------------------------------------------------------

Even though the patch and tests looks fine to me I would like to wait till the WG has decided where PING is going to end up. Potentially your patch is correct and it will end up being mapped to 'connect-src', but might as well be mapped to 'form-action'. Let's just wait and land this patch once the WG has settled.
Clearing review-reqeust for now.
Attachment #8523643 - Flags: review?(mozilla)
Attached patch Code changes and mochitest (obsolete) — Splinter Review
Here's a rebased version of the patch.

The latest spec has moved PING into connect-src: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-connect-src

as per https://lists.w3.org/Archives/Public/public-webappsec/2015Jan/0271.html
Attachment #8523643 - Attachment is obsolete: true
Attachment #8556827 - Flags: review?(mozilla)
Comment on attachment 8556827 [details] [diff] [review]
Code changes and mochitest

Review of attachment 8556827 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Francois,

thanks for being on top and moving 'ping' into connect-src, that's where it belongs in my opinion anyway.
As discussed the other day I would rather not accept any tests that are not working in e10s. Any chance you could rewrite the test so it's not relying on http-on-opening-request?
Attachment #8556827 - Flags: review?(mozilla)
Chris, I removed the "skip if e10s" bit from the test and also ran it successfully under e10s using:

  ./mach mochitest --e10s dom/base/test/csp/test_ping.html

How can I reproduce the e10s failure you're seeing?
Flags: needinfo?(mozilla)
(In reply to François Marier [:francois] from comment #6)
> Chris, I removed the "skip if e10s" bit from the test and also ran it
> successfully under e10s using:
> 
>   ./mach mochitest --e10s dom/base/test/csp/test_ping.html
> 
> How can I reproduce the e10s failure you're seeing?

I haven't tried to run the test, I just saw that the test was skipped
> [test_ping.html]
> skip-if = buildapp == 'b2g' # http-on-opening-request observers are not available in child processes

If I remember it's causing problems on e10s as well, but if you can run the test successfully on TBPL not skipping any platforms or build configurations it should be ok.

Anyway, I would still rather have a test not relying on http-on-opening-request. (I know we still have CSP tests relying on it, but I would really like to get away from that practice).
Flags: needinfo?(mozilla)
Francois, we are triaging at the moment. I see you have a patch, what's missing here to get this landed?
Flags: needinfo?(francois)
The only thing missing here is to rewrite the test in an e10s-compliant way.
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #9)
> The only thing missing here is to rewrite the test in an e10s-compliant way.

What you could do instead of using http-notify-request is an approach where you send the beacon request to an *.sjs server and fetch up the result using an xhr request. Similar do what we do here [1]. Make sure to process the request on the sever async, so we don't end up having any kind of race condition.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/beacon/test_beaconOriginHeader.html?force=1#37
Priority: -- → P3
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to François Marier [:francois] from comment #9)
> > The only thing missing here is to rewrite the test in an e10s-compliant way.
> 
> What you could do instead of using http-notify-request is an approach where
> you send the beacon request to an *.sjs server and fetch up the result using
> an xhr request. Similar do what we do here [1]. Make sure to process the
> request on the sever async, so we don't end up having any kind of race
> condition.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/beacon/
> test_beaconOriginHeader.html?force=1#37


PING is governed by connect-src in CSP Level 3, see:
https://w3c.github.io/webappsec-csp/#directive-connect-src

Hey Francois, as discussed in the triage meeting we can land that patch now. Feel free to flag me for review once the test works on all platforms.
Flags: needinfo?(francois)
Blocks: csp-w3c-3
No longer blocks: csp-w3c-2
Flags: needinfo?(francois)
Hey Francois, any chance you wanna land this? I suppose we can just remove the B2G specific flag from the mochitest.ini and land as is, right?
Flags: needinfo?(francois)
Whiteboard: [CSP 1.1] → [CSP 1.1], [domsecurity-backlog]
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Hey Francois, any chance you wanna land this? I suppose we can just remove
> the B2G specific flag from the mochitest.ini and land as is, right?

I've rebased my patch and I'll test it to see if the tests still pass.

Any reason why we shouldn't just like the "disable if B2G" thing in there? I mean we may as well not break the B2G tests on purpose...
Flags: needinfo?(francois) → needinfo?(mozilla)
(In reply to François Marier [:francois] from comment #13)
> Any reason why we shouldn't just like the "disable if B2G" thing in there? I
> mean we may as well not break the B2G tests on purpose...

I thought that flag only applies to Treeherder. But yeah, sure, leave it in (I suppose). You could ping Paul to make sure!
Flags: needinfo?(mozilla)
Attached patch bug1100181.patch (obsolete) — Splinter Review
Rebased patch. Tests don't seem to pass on e10s :(
Attachment #8556827 - Attachment is obsolete: true
I'm going to unassign myself. I've been sitting on this for far too long and it could be a good first bug for a contributor given that this is just missing a working test.
Assignee: francois → nobody
Status: ASSIGNED → NEW
(In reply to François Marier [:francois] from comment #16)
> I'm going to unassign myself. I've been sitting on this for far too long and
> it could be a good first bug for a contributor given that this is just
> missing a working test.

Fair enough. Adding tpe-seceng to the whiteboard.
Whiteboard: [CSP 1.1], [domsecurity-backlog] → [CSP 1.1], [domsecurity-backlog], tpe-seceng
Assignee: nobody → ckerschb
Priority: P3 → P2
Whiteboard: [CSP 1.1], [domsecurity-backlog], tpe-seceng → [CSP 1.1], [domsecurity-active]
Stephanie, pings should be governed by connect-src according to the latest spec:
https://www.w3.org/TR/CSP3/#directive-connect-src
Attachment #8733983 - Attachment is obsolete: true
Attachment #8763680 - Flags: review?(stephouillon)
Status: NEW → ASSIGNED
Comment on attachment 8763680 [details] [diff] [review]
bug_1100181_csp_ping_connect_src.patch

Hi, 

putting r+, I've just have two small comments:

>diff --git a/dom/security/test/csp/file_ping.html b/dom/security/test/csp/file_ping.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/security/test/csp/file_ping.html
>@@ -0,0 +1,19 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Bug 1100181 - CSP: Enforce connect-src when submitting pings</title>
>+</head>
>+<body>
>+<!-- we are using an image for the test, but can be anything -->
>+<a id="testlink"
>+   href="http://mochi.test:8888/tests/image/test/mochitest/blue.png"
>+   ping="http://mochi.test:8888/tests/image/test/mochitest/blue.png?send-ping"
>+>Send ping</a>
>+
>+<script type="text/javascript">
>+  var link = document.getElementById("testlink");
>+  link.click();
>+</script>
>+
>+</body>
>+</html>

The code could be indented inside the <html> and <body> tags; and, the ">" before "Send ping" could be put on the line before.

>diff --git a/dom/security/test/csp/test_ping.html b/dom/security/test/csp/test_ping.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/security/test/csp/test_ping.html
>@@ -0,0 +1,103 @@
>+
>+var tests = [
>+  {
>+    result : "allowed",
>+    policy : "connect-src 'self'"
>+  },
>+  {
>+    result : "blocked",
>+    policy : "connect-src 'none'"
>+  }
>+];
>+
>+// initializing to -1 so we start at index 0 when we start the test
>+var counter = -1;
>+
>+function checkResult(aResult) {
>+  is(aResult, tests[counter].result, "should be " + tests[counter].result + " in test " + counter + "!");

Isn't there a space missing before "should be " ?
Attachment #8763680 - Flags: review?(stephouillon) → review+
(In reply to Stephanie Ouillon [:arroway] from comment #19)
> Isn't there a space missing before "should be " ?

Forget this, I hit the publish button too quickly.
Attachment #8763680 - Attachment is obsolete: true
Attachment #8764912 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74982b5ca182
CSP: Enforce connect-src when submitting pings. r=arroway
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/74982b5ca182
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.