1179060 - Apply a default CSP for signed packaged content

Status: RESOLVED WONTFIX

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Same as for privileged apps, we need to ensure that signed packages have a securityt policy applied which prevents loading scripts from outside the package.

Comment 1

[Ethan Tseng [:ethan]]

AFAIK, the default CSP policy for signed packages should be equal to privilege apps.

Comment 2

[Ethan Tseng [:ethan]]

=== Work Note ===
1. The default CSP policy for privileged apps is defined in all.js.
   https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#1959

   pref("security.apps.privileged.CSP.default", 
        "default-src * data: blob:; script-src 'self'; object-src 'none'; 
         style-src 'self' 'unsafe-inline'");

2. Principal's app status constants are defined in nsIPrincipal.idl.
   const short APP_STATUS_NOT_INSTALLED = 0;
   const short APP_STATUS_INSTALLED     = 1;
   const short APP_STATUS_PRIVILEGED    = 2;
   const short APP_STATUS_CERTIFIED     = 3;


My question:
If signed packages have the same default policy as privileged apps, do we have to add a new preference, say, "security.apps.signedpackage.CSP.default"? Or we can directly apply the pref for privilege apps?

Do we want to add a new appStatus for signed packaged apps?

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Ethan Tseng [:ethan] from comment #2)
> === Work Note ===
> 1. The default CSP policy for privileged apps is defined in all.js.
>   
> https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#1959
> 
>    pref("security.apps.privileged.CSP.default", 
>         "default-src * data: blob:; script-src 'self'; object-src 'none'; 
>          style-src 'self' 'unsafe-inline'");
> 
> 2. Principal's app status constants are defined in nsIPrincipal.idl.
>    const short APP_STATUS_NOT_INSTALLED = 0;
>    const short APP_STATUS_INSTALLED     = 1;
>    const short APP_STATUS_PRIVILEGED    = 2;
>    const short APP_STATUS_CERTIFIED     = 3;
> 
> 
> My question:
> If signed packages have the same default policy as privileged apps, do we
> have to add a new preference, say,
> "security.apps.signedpackage.CSP.default"? Or we can directly apply the pref
> for privilege apps?
> 
> Do we want to add a new appStatus for signed packaged apps?

Lets create a new one in case we want to vary it.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

And by that I mean

security.signedpackage.CSP.default

Comment 5

[Ethan Tseng [:ethan]]

=== Implementation Plan ===

1. Add a new pref "security.signedpackage.CSP.default" in modules/libpref/init/all.js.
2. Add a new flag |applySignedPackageCSP| in nsDocument::InitCSP().
3. Add code to determine this flag, such as GetDefaultCSPBySignedPackageId().
4. Add csp->AppendPolicy(signedPkgDefaultCSP, false) in nsDocument::InitCSP().
5. Add code in nsCSPContext or nsCSPSerivce to implement policy check for signed package content.


Hi Chris and Paul,
Please provide your feedback on my plan. If it is rational, I'll start to write patches based on this plan.

Comment 6

[Ethan Tseng [:ethan]]

In order to determine whether the load context is signed packaged content, we need to query |mSignedPkg| attribute in OriginAttributes. If the string is not empty, it is signed package.

|mSignedPkg| will be set appropriately when bug 1178526 is done. So set dependency on this bug.

Comment 7

[Ethan Tseng [:ethan]]

Attachment: Part 1: Apply a default CSP for signed packaged content.

This is a WIP patch, which uses URL path to determine the document is signed packaged content or not. The actual way for determination is to use OriginAttributes::mSignedPkg attribute.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8668683 [details] [diff] [review]
Part 1: Apply a default CSP for signed packaged content.

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

Looks good, but definitely needs cleanup and review from a dom-peer.

::: dom/base/nsDocument.cpp
@@ +2794,5 @@
> +            ("Failed to get preference value for %s",
> +             PREF_SIGNED_PACKAGE_DEFAULT_CSP));
> +    return false;
> +  }
> +  return true;

Actually I think you can simplify and inline the whole function underneath.

@@ +2859,5 @@
> +  if (loadContext) {
> +    mozilla::OriginAttributes attrs;
> +    loadContext->GetOriginAttributes(attrs);
> +    if (!attrs.mSignedPkg.IsEmpty()) {
> +      applySignedPackageCSP = true;

yes, please query the originAttributes from the loadInfo.

@@ +2970,5 @@
>  
> +  // ----- if the doc is part of signed package, apply its default CSP.
> +  if (applySignedPackageCSP) {
> +    NS_ConvertUTF8toUTF16 policyString(signedPackageDefaultCSP);
> +    csp->AppendPolicy(policyString, false);

I suppose this is the right place to do that, looks good.

::: dom/base/nsDocument.h
@@ +1735,5 @@
>  
>    nsresult CheckFrameOptions();
>    bool IsLoopDocument(nsIChannel* aChannel);
>    nsresult InitCSP(nsIChannel* aChannel);
> +  bool GetDefaultCSPForSignedPackage(nsCString &aCspString);

no need to make that function part of the class, it's only called from within nsDocument.cpp, right?

::: modules/libpref/init/all.js
@@ +1962,5 @@
>  // Default Content Security Policy to apply to privileged apps.
>  pref("security.apps.privileged.CSP.default", "default-src * data: blob:; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'");
>  
> +// Default Content Security Policy to apply to signed packages.
> +pref("security.signedpackage.CSP.default", "default-src * data: blob:; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'");

Obviously I would prefer if we could remove 'unsafe-inline', any chance we can make that happen?

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> >  
> > +// Default Content Security Policy to apply to signed packages.
> > +pref("security.signedpackage.CSP.default", "default-src * data: blob:; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'");
> 
> Obviously I would prefer if we could remove 'unsafe-inline', any chance we
> can make that happen?

No, unless scoped styles can now be used somehow with non-inline styles. See bug 968907 and also https://bugzilla.mozilla.org/show_bug.cgi?id=1022996#c5

That is unless our web components dont need scoped styles any more. I should check that.

Comment 10

[Mahendra Potharaju [:mahe]]

[Tracking Requested - why for this release]:

This bug being part of New Security Model shouldn’t be a 2.5 blocker as New Sec is not part of 2.5 now.

Removing 2.5 blocker flag.

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Comment on attachment 8668683 [details] [diff] [review]
> Part 1: Apply a default CSP for signed packaged content.
> Review of attachment 8668683 [details] [diff] [review]:
> -----------------------------------------------------------------
> Looks good, but definitely needs cleanup and review from a dom-peer.

Chris, thanks a lot for your feedback!

It's been a while I don't have progress on this bug.
I think I should update some status.

I will enhance the part 1 patch, which depends on bug 1178526, which is going to be landed soon.
After that, I'll request a DOM peer to review the patch (maybe Bolly Holley).

Secondly, according to Chris' advice (offline discussion at MTV), we should split CSP logic somewhere and add handle and security check for signed packaged content. I am still working on this part.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Paul, seems B2G specific as well, what should we do with this?

Comment 13

[Ethan Tseng [:ethan]]

Even if we plan to continue NSEC v2, signed package would no longer be an option.
I think there is no reason so work on this bug.

Comment 14

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #13)
> Even if we plan to continue NSEC v2, signed package would no longer be an
> option.
> I think there is no reason so work on this bug.
                             ^^
typo.
There is no reason to work on this bug.