999656 - CSP in CPP: Refactor mapping of frame-ancestors to TYPE-DOCUMENT

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In the new CSP implementation we currently map frame-ancestors to TYPE-Document. We should refactor that part and come up with a better mapping.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Yep, we should clean this up before calling frame-ancestors "done".

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: first whack

Hey Chris: how do you like this direction?  I still have to clean it up a bit and see if I can't factor more out, but the CSP tests pass with this patch.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8531735 [details] [diff] [review]
first whack

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

Wohoo - we should have used constants in the idl from the beginning instead of introducing enums. This really cleans things up quite a bit. Also, I like the way you cleaned up the permits-functions. Great work! Thanks!

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +23,1 @@
>  [scriptable, uuid(dc86b261-5e41-4cab-ace3-a0278f5a7ec7)]

Don't forget to update the uuid since function signatures changed.

::: dom/security/nsCSPContext.cpp
@@ +179,5 @@
>                                 nonce,
>                                 // aExtra is only non-null if
>                                 // the channel got redirected.
>                                 (aExtra != nullptr),
> +                               false,

nit: maybe add a comment after false
// fall back to default-dir or something!

::: dom/security/nsCSPUtils.cpp
@@ +146,5 @@
> +
> +    case nsIContentPolicy::TYPE_MEDIA:
> +      return nsIContentSecurityPolicy::MEDIA_SRC_DIRECTIVE;
> +
> +    case nsIContentPolicy::TYPE_DOCUMENT:

Even though we can/should leave TYPE_DOCUMENT here, that case statement should never get triggered for TYPE_DOCUMENT. See: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#142
I think we should keep it here anyway though.

@@ +152,5 @@
> +      return nsIContentSecurityPolicy::FRAME_SRC_DIRECTIVE;
> +
> +    // BLock XSLT as script, see bug 910139
> +    case nsIContentPolicy::TYPE_XSLT:
> +      return nsIContentSecurityPolicy::SCRIPT_SRC_DIRECTIVE;

You could move the case statement up right after TYPE_SCRPT;

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: bug999656-refactor-mappings

I pulled out the common code from the various places in nsCSPContext that loop through all the policies, and put that into a permitsInternal function.  That way if we have bugs in policy-checks, it will be easy to track down a single place where it could happen.

I think this is ready.  Chris: what do you think?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6e6d7b5961fa

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8534417 [details] [diff] [review]
bug999656-refactor-mappings

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

Thanks a lot for cleaning that up. Really like to see those changes happening, especially moving all the directive constants into the *.idl.

::: dom/security/nsCSPUtils.cpp
@@ +148,5 @@
> +
> +    case nsIContentPolicy::TYPE_MEDIA:
> +      return nsIContentSecurityPolicy::MEDIA_SRC_DIRECTIVE;
> +
> +    // TYPE_DOCUMENT shouldn't be used since it's specifically whitelistedby

Nit: missing whitespace between whitelisted by

@@ +175,5 @@
> +    // CSP can not block csp reports, fall through to error
> +    case nsIContentPolicy::TYPE_CSP_REPORT:
> +    // Fall through to error for all other directives
> +    default:
> +      NS_ASSERTION(false, "Can not map nsContentPolicyType to CSPDirective");

I think we should use a MOZ_ASSERT(false) in the default case. That should seriously not be allowed to work.

::: dom/security/nsCSPUtils.h
@@ +54,5 @@
>  #define STYLE_NONCE_VIOLATION_OBSERVER_TOPIC    "Inline Style had invalid nonce"
>  #define SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC    "Inline Script had invalid hash"
>  #define STYLE_HASH_VIOLATION_OBSERVER_TOPIC     "Inline Style had invalid hash"
>  
> +// these strings map to the CSPDirectives in nsIContentSecurityPolicy

You have that nice "Note" comment in the *.idl, please also add a similar comment here tha t you have to add a new CSPDirective in the *.idl whenever you add a new directive to that array.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Attachment: bug999656-refactor-mappings (r=ckerschb)

Thanks, Chris.  Carrying over r=ckerschb.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Inbound is open and closed and I just can't time it right today so I'm going to add the checkin-needed keyword here.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12380b98f9d8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/12380b98f9d8