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

NEW
Unassigned

Status

()

P2
normal
2 years ago
11 months ago

People

(Reporter: freddyb, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years 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
Status: NEW → ASSIGNED
(Reporter)

Comment 2

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

Comment 3

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

Comment 4

2 years 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)
(Reporter)

Updated

2 years ago
Attachment #8856415 - Attachment is obsolete: true
Attachment #8856415 - Flags: feedback?(ckerschb)
(Reporter)

Updated

2 years ago
Attachment #8856413 - Attachment is obsolete: true
Attachment #8856413 - Flags: feedback?(ckerschb)
(Reporter)

Updated

2 years ago
Attachment #8856412 - Attachment is obsolete: true
Attachment #8856412 - Flags: feedback?(ckerschb)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 8

2 years 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

2 years 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

2 years 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

2 years 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)
(Reporter)

Comment 13

2 years 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)
(Reporter)

Updated

11 months ago
Assignee: fbraun → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.