1224225 - International Domain Name in connect-src of Content Security Policy header not working

Status: RESOLVED FIXED

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

Description

[rychlikm]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

My server is at https://2π.net/ otherwise known as https://xn--2-umb.net in punny code. It delivers Content Security Policy header like so:

Content-Security-Policy:connect-src 'self' wss://xn--2-umb.net; default-src 'self'; font-src 'self'; frame-src; img-src 'self'; media-src; object-src; script-src 'self'; style-src 'self' 'unsafe-inline'

Browser then makes web socket connection back to:

window.location.origin + '/ws/terminal'

Firefox 42.0 on Debian Jessie.



Actual results:

Firefox blocks my web socket connection with the following console message:

    Content Security Policy: The page's settings blocked the loading of a resource at wss://2π.net/socket.io/?EIO=3&transport=websocket&sid=iv135fhmiskr9vnJAAAE ("connect-src https://2π.net wss://xn--2-umb.net").


Expected results:

Expect wss connection is made successfully as happens with Chrome.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Thank you for the bug report!

> window.location.origin + '/ws/terminal'

That actually produces different strings in Chrome and Firefox.  In particular, in Chrome that's "https://xn--2-umb.net/ws/terminal" while in Firefox it's "https://2π.net/ws/terminal".

Does this work in Firefox if you pass the same string to websocket there that you're passing in Chrome?

In any case, the relevant bit of the spec is at <http://www.w3.org/TR/CSP2/#match-source-expression>.

It starts off with taking the given URL and running it through the URL parser algorithm defined at https://url.spec.whatwg.org/#url-parsing which will end up in https://url.spec.whatwg.org/#host-state which calls https://url.spec.whatwg.org/#concept-host-parser with the "Unicode flag" not set.  Looks like this should set the host to the punycode version.  So having the punycode in the policy should work correctly no matter what's passed to the websocket API.

In terms of our implementation, nsCSPHostSrc::permits uses aUri->GetHost.  It should presumably use aUri0->GetAsciiHost, since it's meant to be matching against punycoded hostnames.  Christoph, you seem like you might know about this code, or at least have blame for it... 

I assume CSP_CreateHostSrcFromURI should also use GetAsciiHost then to make 'self' work.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

That is very likely (In reply to Boris Zbarsky [:bz] from comment #1)
> I assume CSP_CreateHostSrcFromURI should also use GetAsciiHost then to make
> 'self' work.

Thanks Boris, that is very likely the problem. I'll get that fixed!

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

I don't have time to fix that right now - let's put it in the backlog for now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is breaking CSP when non-ASCII hostnames are involved.  We really do need to fix this if we want people hosting sites on such domains to not just tell people to use some other browser.

Tanvi, who might have time to fix this?  Comment 1 has the analysis; this mostly needs verification of the fact that the analysis is correct and tests..

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Steph, would you have time to take a look at this bug? If so, please assign this to yourself.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

The CSP Parser itself should be fine (see inlined testcase underneath); this is a problem with the matching algorithm. I suppose we should be replacing uri->GetHost() with uri->GetAsciiHost() wherever we do a string matching within nsCSPUtils.cpp, e.g. here [1] and obviously write a testcase for it.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#448



%-----snip------

diff --git a/dom/security/test/TestCSPParser.cpp b/dom/security/test/TestCSPParser.cpp
--- a/dom/security/test/TestCSPParser.cpp
+++ b/dom/security/test/TestCSPParser.cpp
@@ -172,16 +172,18 @@ nsresult runTestSuite(const PolicyTest* 
 }
 
 // ============================= TestDirectives ========================
 
 nsresult TestDirectives() {
 
   static const PolicyTest policies[] =
   {
+    { "connect-src xn--mnchen-3ya.de",
+      "connect-src http://xn--mnchen-3ya.de"},
     { "default-src http://www.example.com",
       "default-src http://www.example.com" },
     { "script-src http://www.example.com",
       "script-src http://www.example.com" },
     { "object-src http://www.example.com",
       "object-src http://www.example.com" },
     { "style-src http://www.example.com",
       "style-src http://www.example.com" },

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Freddy, any chance you could take that on? I think comment 6 should be the solution here. Please note that in fact I would imagine we should use GetAsciiHost pretty much anywhere within the CSP code.

Comment 8

[Frederik Braun [:freddy]]

Relevant try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97549f3400bcdb03f6fb70f470acd55fcca7d45c

Comment 9

[Frederik Braun [:freddy]]

Attachment: Bug 1224225: Use GetAsciiHost in CSP source matching code

Review commit: https://reviewboard.mozilla.org/r/121010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121010/

Comment 10

[Frederik Braun [:freddy]]

Attachment: Bug 1224225: Tests for punycode/unicode in CSP source matching code

Review commit: https://reviewboard.mozilla.org/r/121012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121012/

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8848042 [details]
Bug 1224225: Tests for punycode/unicode in CSP source matching code

https://reviewboard.mozilla.org/r/121012/#review122956

::: dom/security/test/csp/test_punycode_host_src.html:38
(Diff revision 1)
> +  },
> +  { // test 2
> +    description: "loads script as sub2.xn--lt-uia.example.org, and whitelist in CSP as sub2.xn--lt-uia.example.org",
> +    action: "script-punycode-csp-punycode",
> +    csp: "script-src http://sub2.xn--lt-uia.example.org;",
> +    expected: "script-allowed",

Can you add a third test replacing http:// with a leading *.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8848041 [details]
Bug 1224225: Use GetAsciiHost in CSP source matching code

https://reviewboard.mozilla.org/r/121010/#review122958

r=me.

Comment 13

[Frederik Braun [:freddy]]

Comment on attachment 8848041 [details]
Bug 1224225: Use GetAsciiHost in CSP source matching code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121010/diff/1-2/

Comment 14

[Frederik Braun [:freddy]]

Comment on attachment 8848042 [details]
Bug 1224225: Tests for punycode/unicode in CSP source matching code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121012/diff/1-2/

Comment 15

[Frederik Braun [:freddy]]

TIL: Mozreview/Reviewboard Autoland doesn't work without L3 access. D'uh.
Please check in! :)

Comment 16

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

Mozreview is saying the second patch needs reviewed, too, before landing.

Comment 17

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

Comment on attachment 8848042 [details]
Bug 1224225: Tests for punycode/unicode in CSP source matching code

https://reviewboard.mozilla.org/r/121012/#review123202

This was already r+d, but mozreview got confused. Maybe this will unconfuse it.

Comment 18

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

Comment on attachment 8848041 [details]
Bug 1224225: Use GetAsciiHost in CSP source matching code

https://reviewboard.mozilla.org/r/121010/#review123204

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cf7e1e3d993
Use GetAsciiHost in CSP source matching code r=ckerschb,KWierso
https://hg.mozilla.org/integration/autoland/rev/6e360ca72271
Tests for punycode/unicode in CSP source matching code r=ckerschb,KWierso

Comment 20

[Frederik Braun [:freddy]]

Thank you for unconfusing mozreview, KWierso!

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cf7e1e3d993
https://hg.mozilla.org/mozilla-central/rev/6e360ca72271