Closed Bug 999656 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ckerschb, Assigned: geekboy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [CSP 1.1])

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 994782
Yep, we should clean this up before calling frame-ancestors "done".
Assignee: nobody → sstamm
Blocks: csp-w3c-2
Severity: normal → minor
Depends on: 846978
Whiteboard: [CSP 1.1]
Attached patch first whack (obsolete) — Splinter Review
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.
Attachment #8531735 - Flags: feedback?(mozilla)
Attachment #8531735 - Attachment is patch: true
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;
Attachment #8531735 - Flags: feedback?(mozilla) → feedback+
Attached patch bug999656-refactor-mappings (obsolete) — Splinter Review
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?
Attachment #8531735 - Attachment is obsolete: true
Attachment #8534417 - Flags: review?(mozilla)
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.
Attachment #8534417 - Flags: review?(mozilla) → review+
Thanks, Chris.  Carrying over r=ckerschb.
Attachment #8534417 - Attachment is obsolete: true
Attachment #8534600 - Flags: review+
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12380b98f9d8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.