1057376 - Fix handling of multiple CSP policies

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Vlatko Markovic]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715215003

Steps to reproduce:

Create an app that has value for the CSP field in the manifest.


Actual results:

The CSP policy from the manifest will be applied as second after the default csp policy. When loading app documents and the policies are applied a warning is shown that one of the policies can't be parsed correctly. The policy that causes the warning contains concatenated string of both default policy and the policy of the manifest.


Expected results:

There should be no warnning and both of the policies should be applied correctly.

Comment 1

[Vlatko Markovic]

Attachment: Bug_1057376-Fix-multiple-CSP-policies-handling.patch

Comment 2

[Vlatko Markovic]

(In reply to Vlatko Markovic from comment #1)
> Created attachment 8477407 [details] [diff] [review]
> Bug_1057376-Fix-multiple-CSP-policies-handling.patch

The toString() which is used inside the for loop in the nsCSPContext::Write() function [ http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPContext.cpp#1202 ] does not clear the string contents so it puts the concatenated strings in the output stream. Later when we read from the stream we read merged policies from there.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8477407 [details] [diff] [review]
Bug_1057376-Fix-multiple-CSP-policies-handling.patch

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

Does this fix the problem stated in the bug (would be nice to have an automated test that catches this)?  ::Write and ::Read are mainly used for serialization of the CSP for session restore and caching (nsISerializable).  The app CSP (from manifest) is applied inside nsDocument::InitCSP() the first time.

Either way, good catch.  This lack of truncation is a bug and we should fix it.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

flagging needinfo for my questions in comment 3.

Comment 5

[Vlatko Markovic]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Comment on attachment 8477407 [details] [diff] [review]
> Bug_1057376-Fix-multiple-CSP-policies-handling.patch
> 
> Review of attachment 8477407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this fix the problem stated in the bug (would be nice to have an
> automated test that catches this)?  ::Write and ::Read are mainly used for
> serialization of the CSP for session restore and caching (nsISerializable). 
> The app CSP (from manifest) is applied inside nsDocument::InitCSP() the
> first time.
> 
> Either way, good catch.  This lack of truncation is a bug and we should fix
> it.

The problem that occured was that the second (and probably each following policy) that is applied is becoming incorrect and applied like that.

If we want for example to apply:
Policy 0: default-src *; script-src *; object-src 'none'
Policy 1: style-src http://some.domain.com; script-src http://some.domain.com

What we get is:
Policy 0: default-src *; script-src *; object-src 'none'
Policy 1: default-src *; script-src *; object-src 'none'style-src http://some.domain.com; script-src http://some.domain.com

In this case for the second policy for object-src we will get only "http://some.domain.com" since the merged strings are creating the invalid source "'none'style-src" for which there is warning and is ignored, and we get duplicate script-src directive "script-src http://some.domain.com" which is also ignored according to the warnings.

If we have the truncation the warnings are not there so I assume that the problem described here is solved.

The truncation is needed here since neither nsCSPPolicy::toString() [http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#853] or nsCSPDirective::toString [http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#635] that are used in this context does not clear the output string content.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

I think maybe my question was not clear.  Does this change actually fix the problem described in the beginning?  read/write are used for nsISerializable operations in session restore, I don't think they're used for app loading (though my understanding of how that works is not very good).

Do you have a test that I can run that reproduces this problem without the patch and seems to be solved with this patch applied?  Or can you point to where in mozilla-central's code the app loading triggers a "read"?

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8477407 [details] [diff] [review]
Bug_1057376-Fix-multiple-CSP-policies-handling.patch

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

Someone mentioned to me you're using appcache to pre-load the apps, which would cause the "deserialization" behavior you're seeing.  This is the right fix for that case, so while I'd love to see a test for this, I think we should fix it quickly.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Before checkin, please update the patch summary to indicate *how* the fix works.  (e.g., "clear read buffer between policy deserializations in CSP" or something).

Comment 9

[Vlatko Markovic]

Attachment: Bug_1057376-Fix-multiple-CSP-policies-handling.patch

Update patch summary.

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/b70adcc6738f

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8491722 [details] [diff] [review]
Bug_1057376-Fix-multiple-CSP-policies-handling.patch

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

I think I already r+'ed this... anyhow, here's another r=me (good commit message, thanks).

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/b70adcc6738f

Comment 13

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/d614ae398c03