802833 - Deprecate TYPE_OTHER, create new content types for SVG Animations and Effects

Status: NEW

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Spawning this bug off from bug 799346.  Replace TYPE_OTHER with more specific Content Types.  The uses of TYPE_OTHER are for CSP Reports and SVG Animations and Effects.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)

Work by Brian Smith.  Requesting review from Sid Stamm.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 672539 [details] [diff] [review]
Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)

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

CSP stuff looks pretty good modulo a few little things.  dholbert should look at some of the other stuff.

::: content/base/src/contentSecurityPolicy.js
@@ -49,5 @@
>    let cp = Ci.nsIContentPolicy;
>    let csp = ContentSecurityPolicy;
>    let cspr_sd = CSPRep.SRC_DIRECTIVES;
>  
> -  csp._MAPPINGS=[];

If you don't have this, you might get an error when you try to reference _MAPPINGS[something]...

@@ +327,5 @@
>              var contentPolicy = Cc["@mozilla.org/layout/content-policy;1"]
>                                    .getService(Ci.nsIContentPolicy);
> +            if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_CSP_REPORT,
> +                                         chan.URI, this._request,
> +                                         null, null, null) // XXX: should pass principal here

You might want to store a reference to the principal during scanRequestData(), then use it here.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 672539 [details] [diff] [review]
Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)


>diff -r 8817fd2e5045 content/base/public/nsIContentPolicy.idl

Side note: This ^^^ header on each chunk makes bugzilla's diff viewer display "8817fd2e5045" instead of the actual filename.  This makes patches hard to read/review the patch on bugzilla.  I'm not sure what tool / command you're using to generate your patches, but you might want to look into doing it differently so that you get an actual filename displayed there as the first "diff" argument, to make Bugzilla's diff-viewer happy.

>-  /* Please update nsContentBlocker when adding new content types. */
>+  /**
>+   * Inicates a Content Security Policy report.
>+   */
>+  const nsContentPolicyType TYPE_CSP_REPORT = 17;

s/Inicates/Indicates/

>+  /**
>+   * Inicates an external HTML/SVG/SMIL animation or effect.
>+   */
>+  const nsContentPolicyType TYPE_ANIMATION_OR_EFFECT = 18;

s/Inicates/Indicates/

Also, RE the actual substance of the comment -- it's not quite right, for animations at least.  For animations, the targeted resource is the *target* of an animation (the thing to be animated).

And "external" doesn't seem quite right, either. IIRC, SVG animations are only allowed to target elements in the same document, and SVG filters & <use> could be same-document or different-document -- but we appear to pass this content-type regardless of whether the resource is external or internal

>+
>+  /* When adding new content types, please update nsContentBlocker,
>+   * NS_CP_ContentTypeName, contentScurityPolicy.js, all nsIContentPolicy

s/contentScurityPolicy/contentSecurityPolicy/

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 672539 [details] [diff] [review]
Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)

>@@ -1068,17 +1070,17 @@ nsExternalResourceMap::PendingLoad::Star
>   // Allow data URIs and other URI's that inherit their principal by passing
>   // true as the 3rd argument of CheckMayLoad, since we want
>   // to allow external resources from data URIs regardless of the difference
>   // in URI scheme.
>   rv = requestingPrincipal->CheckMayLoad(aURI, true, true);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>-  rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_OTHER,
>+  rv = NS_CheckContentLoadPolicy(aCPType,


Do we really need to pass aCPType down so far?

AFAICT from the patch, every single entry point to this ^^^ code will have aCPType == TYPE_ANIMATION_OR_EFFECT, so perhaps (for now) it'd be simpler to just hardcode that in here, and avoid having to modify like 6 levels of callers...?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Do we really need to pass aCPType down so far?
> 
> AFAICT from the patch, every single entry point to this ^^^ code will have
> aCPType == TYPE_ANIMATION_OR_EFFECT, so perhaps (for now) it'd be simpler to
> just hardcode that in here, and avoid having to modify like 6 levels of
> callers...?

My concern was/is that we may have new kinds of external resources in the future, and that if we made the aCPType implicit, then those new kinds of external resources may get accidentally classified as TYPE_ANIMATION_OR_EFFECT. The patch is already written and doesn't seem too disruptive and doesn't seem likely to cause a performance or memory hit (no need to add new data members to any classes, for example), so I hope it will be OK to do it this way.

That said...

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Also, RE the actual substance of the comment -- it's not quite right, for
> animations at least.  For animations, the targeted resource is the *target*
> of an animation (the thing to be animated). And "external" doesn't seem 
> quite right, either. IIRC, SVG animations are only allowed to target
> elements in the same document,  and SVG filters &
> <use> could be same-document or different-document -- but we appear to pass
> this content-type regardless of whether the resource is external or internal

It seems quite unclear how animations/filters/effects should be handled regarding nsIContentPolicy. Definitely I was mistaken when I wrote this patch. The mixed content blocker, if not many other (all?) nsIContentPolicy implementations, is written based on the assumption that the target is the thing for which we are being about.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Brian Smith (:bsmith) from comment #5)
> (In reply to Daniel Holbert [:dholbert] from comment #4)
> > Do we really need to pass aCPType down so far?
> > 
> > AFAICT from the patch, every single entry point to this ^^^ code will have
> > aCPType == TYPE_ANIMATION_OR_EFFECT, so perhaps (for now) it'd be simpler to
> > just hardcode that in here, and avoid having to modify like 6 levels of
> > callers...?
> 
> My concern was/is that we may have new kinds of external resources in the
> future, [...] I hope it will be OK to do it this way.

OK, seems reasonable to me.

> (In reply to Daniel Holbert [:dholbert] from comment #3)
> It seems quite unclear how animations/filters/effects should be handled
> regarding nsIContentPolicy.

OK -- let's chat on IRC about this. Also, I think bz should ultimately review this -- he understands our external-resource-restriction policies & assumptions better than I do.

Comment 7

[Tanvi Vyas[:tanvi]]

Split out the CSP Report type into bug 802905.  So this bug no longer blocks bug 782654.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 672539 [details] [diff] [review]
Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)

(canceling review-request, as there's been some re-thinking of the patch, and per comment 6 I shouldn't be the ultimate reviewer here anyway)