965273 - (CSP) app:// protocol allows { on the host part

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

The app:// protocol allows { as a valid character for the 'host' part of the URI, but at [1] (and on some more places over that file) that character isn't allowed for the host. So a CSP like

style-src: 'self' unsafe-inline 

gets translated into 
style-src: app:// unsafe-inline

Which then fails with 

E/GeckoConsole(  948): [JavaScript Warning: "Content Security Policy: Failed to parse unrecognized source app://"].

Dunno if it's related, but after that I also get:

E/GeckoConsole(  948): [JavaScript Error: "Content Security Policy: Can't use 'self' if self data is not provided"]
E/GeckoConsole(  948): [JavaScript Warning: "Content Security Policy: Failed to parse unrecognized source unsafe-inline"]

(those two errors come from parsing unsafe-inline on the same directive).

The {} characters are used for most of the installed apps (except for the privileged ones that also request a specific origin on their manifest). 

I don't know if the effect of that failure is that we're allowing to load some things we should not allow (because that rules aren't added) or the reverse.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Agh, I forgot the ref [1]: http://hg.mozilla.org/mozilla-central/annotate/7e79536aca0a/content/base/src/CSPUtils.jsm#l1596

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

The CSP specification does not allow { or }.  It says:
host-char = ALPHA / DIGIT / "-"
http://www.w3.org/TR/CSP/#source-list

In order to support that char, we'll have to make our implementation non-conforming, but I don't think that's the main problem here.

How are you producing this error?  The main problem you're seeing is that when CSP is created, there's no value for "self" (which means the thing that makes the CSP does not pass all the right arguments).

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> The CSP specification does not allow { or }.  It says:
> host-char = ALPHA / DIGIT / "-"
> http://www.w3.org/TR/CSP/#source-list

I know... and yet, it moves. We're using the {} chars on the app URIs from the start, and changing that now would be a problem (since we would have to create a update process to modify the installation of all the store apps on all the end user phones). I think it will be easier just to add the characters { and } to the character set if the protocol is app: 

> 
> In order to support that char, we'll have to make our implementation
> non-conforming, but I don't think that's the main problem here.
> 
> How are you producing this error?  The main problem you're seeing is that
> when CSP is created, there's no value for "self" (which means the thing that
> makes the CSP does not pass all the right arguments).

The thing is that I'm not sure how this is being thrown. I'm launching an app, which doesn't log any CSP errors on the child process. And it doesn't log any error because the CSP is appended as:

CSP debug: APPENDING POLICY: default-src *; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'
CSP debug:             SELF: app://{f0e0372a-6d51-4f5a-bab9-d38b99adbc3a}/index.html

Since there's no app:// on the policy, when it's parsed it doesn't give any error. Still it's processed to:

default-src *; script-src app://; object-src 'none'; style-src app:// unsafe-inline

which isn't correct.

Then at some point, the child process requests something from the parent one (I still don't know exactly at what point since the app I'm checking isn't mine), and the parent process starts giving the error I said at comment 0. Adding more logs, I see:

CSP debug: CSP CREATED
CSP debug: CSP object initialized, no policies to enforce yet
CSP debug: APPENDING POLICY: default-src *; script-src app://; object-src 'none'; style-src app:// unsafe-inline
CSP debug:             SELF:  null
CSP debug: CSP 1.0 COMPLIANT : false

Self is null there as you can see, and the policy is incorrect.

But even after adding the {} characters to the character set (I get then):

APPENDING POLICY: default-src *; script-src app://{f0e0372a-6d51-4f5a-bab9-d38b99adbc3a}; object-src 'none'; style-src app://{f0e0372a-6d51-4f5a-bab9-d38b99adbc3a} unsafe-inline
CSP debug:             SELF:  null

I still get the same error when parsing unsafe-inline. And BTW, I think that's a bug because the policy I'm parsing doesn't have any self and so it shouldn't matter if self was defined or not.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> How are you producing this error?  The main problem you're seeing is that
> when CSP is created, there's no value for "self" (which means the thing that
> makes the CSP does not pass all the right arguments).

Self is being passed as null intentionally from:

http://hg.mozilla.org/mozilla-central/diff/80c7fa20601d/content/base/src/contentSecurityPolicy.js#l1.164

As I thought, the CSP is being passed from the child process serialized and the parsing is failing.

As I see it, there are two separate but related problems here:

1. Either we remove the {} characters from the app URIS or we allow them on the CSP. Since at this point is much easier adding them, I'm going to do just that (for app URIs only) if you're ok with that.

2. We're checking if self is defined or not, even when it doesn't really matter since 'self' isn't used. I think we should only check that self is defined when 'self' is actually used.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. Add {} to the valid charset and fix the serialization/deserialization

This patch seems to fix the problem when deserializing a CSP on the parent.

To be honest, I'm still not sure when the serialization/deserialization that was failing is actually invoked. I have an app that produces the fail, but it's:

a) not mine
b) huge
c) has the code obfuscated

So... I haven't a clue where is it requesting something that needs the parent to apply the CSP. In any case, this patch takes care of the problem (no more spurious messages, and the CSP is created correctly at the parent process).

BTW, can I get access to bug 911547?

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8368178 [details] [diff] [review]
V1. Add {} to the valid charset and fix the serialization/deserialization

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

r=me after updating the invalid host character matching to use the regex R_HOSTCHAR in the top of CSPUtils.jsm.

::: content/base/src/CSPUtils.jsm
@@ +1334,5 @@
> +  // check for 'unsafe-eval' (case insensitive)
> +  if (aStr.toLowerCase() === "'unsafe-eval'"){
> +    sObj._allowUnsafeEval = true;
> +    return sObj;
> +  }

why did you move these up in the file?

@@ +1606,5 @@
>    if (!aStr) return null;
>  
>    // host string must be LDH with dots and stars.
> +  // We allow {} here also for app: 'hosts'
> +  var invalidChar = aStr.match(/[^{}a-zA-Z0-9\-\.\*]/);

I know you didn't write this code originally, but would you please change the regex here to build upon R_HOSTCHAR.source instead of hard-coding the character class an additional time?  (Same with the other seg.match call below around line 1631).

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Could you also add a quick test to test_csputils.js to verify that the app scheme and {} work?  Should be a quick one.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review. I'll add the test tomorrow, and make the other change you requested.

As for 

::: content/base/src/CSPUtils.jsm
@@ +1334,5 @@
> +  // check for 'unsafe-eval' (case insensitive)
> +  if (aStr.toLowerCase() === "'unsafe-eval'"){
> +    sObj._allowUnsafeEval = true;
> +    return sObj;
> +  }

Hmm I moved those because it was crashing at [1]. But come to think about it, I think that was also because the serialization was incorrect also (and it didn't had the quotes) and I only realized that after moving this and seeing that while not crashing I still didn't got the correct parsed CSP. 

In any case, I think it's better to leave it where it's now because we save on a bunch of matching on the cases when it's one of the reserved keywords (at the cost of doing two simple comparisons on the cases when it isn't). If you prefer it at the end, I can move it back down.

[1] http://hg.mozilla.org/mozilla-central/annotate/3a620ff8ac4a/content/base/src/CSPUtils.jsm#l1340

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

I'm not sure it matters a whole lot.  We'll be replacing the whole parser with a faster one soon-ish (see bug 925004), but so long as moving the if blocks doesn't break things, the move is fine.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2, including review comments

r=sstamm

Try run at https://tbpl.mozilla.org/?tree=Try&rev=f9170faa0808

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

Try run looks mostly good, requesting checkin.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/618319a4d21d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/618319a4d21d

Comment 14

[Preeti Raghunath(:Preeti)]

Minus for 1.3. Very late for 1.3