Fix handling of multiple CSP policies

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

Core
DOM: Security
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Vlatko Markovic, Assigned: Vlatko Markovic)

Tracking

(Blocks: 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8477407 [details] [diff] [review]
Bug_1057376-Fix-multiple-CSP-policies-handling.patch
Attachment #8477407 - Flags: review?(fabrice)
(Assignee)

Comment 2

4 years ago
(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)
(Assignee)

Comment 5

4 years ago
(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: 493857
Severity: normal → minor
Status: UNCONFIRMED → NEW
Component: DOM: Apps → DOM: Security
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk

Updated

4 years ago
Blocks: 1059202
(Assignee)

Comment 9

4 years ago
Created attachment 8491722 [details] [diff] [review]
Bug_1057376-Fix-multiple-CSP-policies-handling.patch

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
You need to log in before you can comment on or make changes to this bug.