1224694 - Unify and clean up initialization of CSP

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

Comment 5

[Pulsebot]

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

Comment 6

[Nigel Babu [:nigelb]]

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

[Nigel Babu [:nigelb]]

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

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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