Closed Bug 1057376 Opened 5 years ago Closed 5 years ago

Fix handling of multiple CSP policies

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: vlatko.markovic, Assigned: vlatko.markovic)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8477407 - Flags: review?(fabrice)
(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.
Attachment #8477407 - Flags: review?(fabrice) → review?(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.
flagging needinfo for my questions in comment 3.
Flags: needinfo?(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.
Flags: needinfo?(vlatko.markovic)
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"?
Flags: needinfo?(vlatko.markovic)
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.
Attachment #8477407 - Flags: review?(sstamm) → review+
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).
Flags: needinfo?(vlatko.markovic)
Blocks: CSP
Severity: normal → minor
Status: UNCONFIRMED → NEW
Component: DOM: Apps → DOM: Security
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Blocks: 1059202
Update patch summary.
Attachment #8477407 - Attachment is obsolete: true
Attachment #8491722 - Flags: review?(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).
Attachment #8491722 - Flags: review?(sstamm) → review+
https://hg.mozilla.org/mozilla-central/rev/b70adcc6738f
Assignee: nobody → vlatko.markovic
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.