If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Investigate add-on carve outs for frame-ancestors (and subjectToCSP infrastructure in general)

ASSIGNED
Assigned to

Status

()

Core
DOM: Security
P2
normal
ASSIGNED
7 months ago
4 months ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

7 months ago
It's not possible to frame a page from an add-on that has a CSP with a restrictive frame-ancestors directive.
Given how we have a carve out for other things, it's odd that add-on pages can not bypass frame-ancestors.

What I have learned so far:

Ancestry checking starts in nsDocument, calling PermitsAncestry on the CSP:
https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/dom/base/nsDocument.cpp#2750

PermitsAncetry in nsCSPContext collects the ancestry chain and hands the chain to permitsInternal: https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/dom/security/nsCSPContext.cpp#1209

permitsInternal (also in nsCSPContext) goes through the policies and calls permits() on all policies:
https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/dom/security/nsCSPContext.cpp#228

This is where I'm not entirely shure bit AFAIU nsCSPPolicy::permits goes through the directives and for frame-ancestor directives it uses the typical srclist style matching (e.g., SchemeSrc).


** Next step is how other directives are checked and when their subjectToCSP call happens. It's best we to align those two callsites. **
Priority: -- → P2
Whiteboard: [domsecurity-active]
Frame ancestors is somehow special in my opinion; since it stops a whole document from loading. If frame-ancestors should not apply to certain loads, then we should guard the entry point into CSP, which is here [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2745
Blocks: 1231788
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 months ago
Created attachment 8856412 [details] [diff] [review]
0001-Bug-1346774-move-subjectToCSP-into-static-helper-fun.patch
Attachment #8856412 - Flags: feedback?(ckerschb)
(Assignee)

Comment 3

6 months ago
Created attachment 8856413 [details] [diff] [review]
0002-Bug-1346774-do-not-check-frame-ancestors-for-non-CSP.patch
Attachment #8856413 - Flags: feedback?(ckerschb)
(Assignee)

Comment 4

6 months ago
Created attachment 8856415 [details] [diff] [review]
0003-Bug-1346774-test-frame-ancestors-and-chrome-pages-r-.patch

Please take a first look at the patches.

The test patch is only green-ish:
I get an intermittent timeout for the 2nd part (the csp-on-violate-policy observer) and have yet to find out why.
Attachment #8856415 - Flags: feedback?(ckerschb)
(Assignee)

Updated

6 months ago
Attachment #8856415 - Attachment is obsolete: true
Attachment #8856415 - Flags: feedback?(ckerschb)
(Assignee)

Updated

6 months ago
Attachment #8856413 - Attachment is obsolete: true
Attachment #8856413 - Flags: feedback?(ckerschb)
(Assignee)

Updated

6 months ago
Attachment #8856412 - Attachment is obsolete: true
Attachment #8856412 - Flags: feedback?(ckerschb)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

6 months ago
(In reply to Frederik Braun [:freddyb] from comment #4)
> I get an intermittent timeout for the 2nd part (the csp-on-violate-policy
> observer) and have yet to find out why.

Nevermind :)

Comment 9

6 months ago
mozreview-review
Comment on attachment 8856447 [details]
Bug 1346774: move subjectToCSP into static helper function

https://reviewboard.mozilla.org/r/128404/#review131262

::: dom/security/nsCSPService.h:15
(Diff revision 1)
>  #include "nsXPCOM.h"
>  #include "nsIContentPolicy.h"
>  #include "nsIChannel.h"
>  #include "nsIChannelEventSink.h"
> +#include "nsNetUtil.h"
> +#include "nsIProtocolHandler.h"

I don't think you need those inclusions, do you?

::: dom/security/nsCSPUtils.h:15
(Diff revision 1)
>  #include "nsCOMPtr.h"
>  #include "nsIContentPolicy.h"
>  #include "nsIContentSecurityPolicy.h"
>  #include "nsIURI.h"
> +#include "nsNetUtil.h"
> +#include "nsIProtocolHandler.h"

I don't think you need those inclusions here either
Attachment #8856447 - Flags: review?(ckerschb) → review+

Comment 10

6 months ago
mozreview-review
Comment on attachment 8856448 [details]
Bug 1346774: do not check frame-ancestors for non-CSP eligible URLs

https://reviewboard.mozilla.org/r/128406/#review131268

::: dom/security/nsCSPContext.cpp:1277
(Diff revision 1)
>  
>      currentURI = doc->GetDocumentURI();
>  
>      if (currentURI) {
> +
> +      // skip items in ancestor tree if the URI is not CSP eligible

that looks good but please expand that comment, in particular add something about the carveout for addons that should be allowed to load CSP protected frames.
Attachment #8856448 - Flags: review?(ckerschb) → review+

Comment 11

6 months ago
mozreview-review
Comment on attachment 8856449 [details]
Bug 1346774: test frame-ancestors and chrome pages

https://reviewboard.mozilla.org/r/128408/#review131270

::: dom/security/test/csp/test_frame_ancestor_chain.html:12
(Diff revision 1)
> +<head>
> +    <meta charset="utf-8">
> +    <title>Test for Bug 1346774</title>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> +    <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
> +    <script type="application/javascript">

I generally think it's better to have the JS in the body and not in the head, but I can live with that, up to you. But I think we generally use 2 space indendation instead of 4, right?

::: dom/security/test/csp/test_frame_ancestor_chain.html:75
(Diff revision 1)
> +            // Instead we test that the title is not from file_frame_ancestors_deny (i.e., 'Unframeable')
> +            const frameDoc = contentWindow.document.getElementById("iframe").contentDocument;
> +            isnot(frameDoc.title, "Unframeable", 'A content window can\'t bypass frame ancestors');
> +            contentWindow.close()
> +            testResult();
> +        }*/

if that is not needed, please remove

::: dom/security/test/csp/test_frame_ancestor_chain.html:94
(Diff revision 1)
> +            }
> +        }
> +
> +
> +
> +

please remove empty lines

::: dom/security/test/csp/test_frame_ancestor_chain.html:105
(Diff revision 1)
> +    <p id="display"></p>
> +    <div id="content" style="display: none">
> +
> +    </div>
> +    <pre id="test">
> +</pre>

nit: have opening and closing tags on the same line

::: dom/security/test/csp/test_frame_ancestor_chain.html:108
(Diff revision 1)
> +    </div>
> +    <pre id="test">
> +</pre>
> +</body>
> +
> +</html>

remove empty lines
Attachment #8856449 - Flags: review?(ckerschb) → review+
Is there a reason this never landed?
Flags: needinfo?(fbraun)
(Assignee)

Comment 13

4 months ago
Apologies. priorities and other things got in the way.
The patch still applies cleanly, so I just took your nits and pushed to try again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a104f4917510adc338ad3ff5bdde738bf7d4e9f3
Flags: needinfo?(fbraun)
You need to log in before you can comment on or make changes to this bug.