Unify and clean up initialization of CSP

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Since we are adding Meta CSP we now are dealing with multiple entry points to set and initalize a CSP. We should unify those entry points and clean that up.

Clean up includes:
* Setting the CSP on the principal (e.g. principal::GetCSP() could already return a CSP instead of making the caller responsbile to create one).
* CSP affects specific things on the document (e.g. referrerPolicy, flags for upgrade insecure requets and upgrade insecure preloads) - we should unify that behavior and create a nice API for that.
* Probably a few more things within nsDocument::InitCSP() can be factored out and cleand up.
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 968586
Status: NEW → ASSIGNED
Depends on: 663570
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 1

2 years ago
Created attachment 8699294 [details] [diff] [review]
bug_1224694_unify_csp_init.patch

Hey Jonas, this patch unifies the csp initialization within the principal. However, I don't know what the best way is to handle the serialization of the CSP within:
* nsPrincipal::Read() and
* nsPrincipal::Write().

Any suggestions? Also, I think we are missing the serialization of preloadCSP, right?
Flags: needinfo?(jonas)
Attachment #8699294 - Flags: feedback?(jonas)
(Assignee)

Comment 2

2 years ago
Created attachment 8707660 [details] [diff] [review]
bug_1224694_unify_csp_init.patch

As discussed in person, we can directly access mCSP since it's a member of the Principal class. So this is really a review request and not a feedback request. Thanks Jonas!
Attachment #8699294 - Attachment is obsolete: true
Attachment #8699294 - Flags: feedback?(jonas)
Flags: needinfo?(jonas)
Attachment #8707660 - Flags: review?(jonas)
Comment on attachment 8707660 [details] [diff] [review]
bug_1224694_unify_csp_init.patch

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

r=me with that fixed

::: caps/nsIPrincipal.idl
@@ +145,5 @@
> +     *
> +     * Please note if aDocument is null, then setRequestContext on the
> +     * CSP object is called using the current principal.
> +     */
> +    [noscript] nsIContentSecurityPolicy initAndSetCSP(in nsIDOMDocument aDocument);

Name this ensureCSP instead. That's the naming convention we use elsewhere for similar things.

@@ +166,5 @@
> +     *
> +     * Please note if aDocument is null, then setRequestContext on the
> +     * speculative CSP object is called using the current principal.
> +     */
> +    [noscript] nsIContentSecurityPolicy initAndSetPreloadCSP(in nsIDOMDocument aDocument);

ensurePreloadCSP

::: caps/nsPrincipal.cpp
@@ -409,5 @@
> -
> -  // need to link in the CSP context here (link in the URI of the protected
> -  // resource).
> -  if (csp) {
> -    csp->SetRequestContext(nullptr, this);

Don't you need to do this still?
Attachment #8707660 - Flags: review?(jonas) → review+
(Assignee)

Comment 4

2 years ago
Created attachment 8707674 [details] [diff] [review]
bug_1224694_unify_csp_init.patch

(In reply to Jonas Sicking (:sicking) from comment #3)
> Name this ensureCSP instead. That's the naming convention we use elsewhere
> for similar things.

Makes sense.

> ::: caps/nsPrincipal.cpp
> > -  if (csp) {
> > -    csp->SetRequestContext(nullptr, this);
> 
> Don't you need to do this still?

Yes, indeed - good catch - thanks!
Attachment #8707660 - Attachment is obsolete: true
Attachment #8707674 - Flags: review+

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f001a01c85d7

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bba3baa2ce769798c5e1d9196c8c443bd97c43b
Backed out changeset f001a01c85d7 (bug 1224694) for browser-chrome bustage on a CLOSED TREE

Comment 7

2 years ago
Backed out for bustages like https://treeherder.mozilla.org/logviewer.html#?job_id=19782871&repo=mozilla-inbound

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/febf0e69c996

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/febf0e69c996
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1028724
You need to log in before you can comment on or make changes to this bug.