(CSP) Implement frame ancestor check

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Core
DOM: Core & HTML
P1
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: geekboy, Assigned: geekboy)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 399553 [details] [diff] [review]
Checks a page's CSP before loading it as a subdocument 

When a page has CSP enabled and it is requested by a site that will put it in a frame, the policy's "frame-ancestors" directive must be satisfied before it is loaded as a sub-document.

Initial patch is attached.
(Assignee)

Comment 1

9 years ago
Created attachment 401119 [details] [diff] [review]
CSP Frame Ancestor Check (v7)

This version is updated to check for the CSP instance in the document's principal (not in nsDocument) as the rest of the v7 patches are written.
Attachment #399553 - Attachment is obsolete: true
(Assignee)

Comment 2

8 years ago
Created attachment 402491 [details] [diff] [review]
CSP Frame Ancestor Check (v7.1)

Added mochitests (10) for checking efficacy of the frame-ancestors restriction.  Also tweaked contentSecurityPolicy.js component to properly report violations via the observer service.
Attachment #401119 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Created attachment 403672 [details] [diff] [review]
CSP Frame Ancestor Check (v7.2)

Fixed mochitests to play nice with the main test harness (subframes were using window.top instead of finding the appropriate intermediate frame to alert when they're loaded).
Attachment #402491 - Attachment is obsolete: true
(Assignee)

Comment 4

8 years ago
Created attachment 420199 [details] [diff] [review]
CSP Frame Ancestor Check (v7.4)

Includes some bug fixes and modifications based on brief, informal reviews. (Also had pretty nasty bit rot.)
Attachment #403672 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Comment on attachment 420199 [details] [diff] [review]
CSP Frame Ancestor Check (v7.4)

This is a pretty short patch -- most of it is unit tests.  jst, would you take a peek?
Attachment #420199 - Flags: review?(jst)

Updated

8 years ago
Attachment #420199 - Flags: review?(jst) → review+
Comment on attachment 420199 [details] [diff] [review]
CSP Frame Ancestor Check (v7.4)

- In nsDocShell::DisplayLoadError():

+    else if (NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION == aError) {
+        // CSP error
+        error.AssignLiteral("forbiddenFrameAncestor"); 
+        cssClass.AssignLiteral("neterror");
+        // TODO: localize this, or at least put the message into a resource
+        messageStr.AssignLiteral("CSP Blocked loading of this page because it doesn't trust the page embedding it.");

Is there a bug on file to localize this?

r=jst assuming that bug exists and is dependent on this bug.
(Assignee)

Comment 7

8 years ago
Created attachment 423083 [details] [diff] [review]
CSP Frame Ancestor Check (v7.5)

Moved the strings into localizable .properties and .dtd files.  Carrying over jst's review, since that's all I changed and johnath wanted to peek at it anyway.

johnath: would you do a UI review on the strings and the use of neterror?
Attachment #420199 - Attachment is obsolete: true
Attachment #423083 - Flags: ui-review?(johnath)
Attachment #423083 - Flags: review+
Comment on attachment 423083 [details] [diff] [review]
CSP Frame Ancestor Check (v7.5)

>diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/appstrings.properties
>+cspFrameAncestorBlocked=This page did not load because it doesn't trust the page embedding it.

We generally don't anthropomorphize web pages like this.  :) I think we want to talk about the policy error, not the trust of the page.  Maybe something like:

This page cannot be loaded because it has a security policy which prevents embedding in this way.

or

This page has a content security policy which prevents it from being embedded in this way.


Still not something a user is going to understand, but maybe more explicit about the cause?

(Ditto for the other .properties file with identical text)

>diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/netError.dtd
>+<!ENTITY cspFrameAncestorBlocked.title "CSP Blocked This Load">

We shouldn't use unspecified acronyms in our error messages. Something like:

"Blocked by Security Policy"

or

"Blocked by Content Security Policy"

>+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>CSP blocked this load because the protected page is embedded on another page in a way not allowed by this page's policy.</p>">

Likewise, let's avoid the acronym. When we need to refer to an agent doing a thing in these messages, we typically use "Firefox", so maybe:

"&brandShortName; prevented this page from loading in this way because the page has a content security policy which prevents it."


(And again, ditto with the other versions of these strings).

ui-r- but it should be a quick + with the changes - I mostly just want another crack at them. :)

Thanks Sid - really excited to see this stuff landing soon.
Attachment #423083 - Flags: ui-review?(johnath) → ui-review-
(Assignee)

Comment 9

8 years ago
Created attachment 423094 [details] [diff] [review]
CSP Frame Ancestor Check (v7.5 with revised strings)

here's another whack, johnath.
Attachment #423083 - Attachment is obsolete: true
Attachment #423094 - Flags: ui-review?(johnath)
Attachment #423094 - Flags: review+
Comment on attachment 423094 [details] [diff] [review]
CSP Frame Ancestor Check (v7.5 with revised strings)

>diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/appstrings.properties
>+cspFrameAncestorBlocked=This page has a content security policy that prevents it from being embedded in this way.
>diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/netError.dtd
>+<!ENTITY cspFrameAncestorBlocked.title "Blocked by Content Security Policy">
>+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>&brandShortName; prevented this page from loading in this way because the page has a content security policy that disallows it.</p>">

These look good - thanks, Sid. However:

>diff -r d660f235e5ad dom/locales/en-US/chrome/netError.dtd
>--- a/dom/locales/en-US/chrome/netError.dtd	Tue Jan 05 13:08:54 2010 -0800
>+++ b/dom/locales/en-US/chrome/netError.dtd	Fri Jan 22 16:01:12 2010 -0800
>@@ -75,13 +75,16 @@
> ">
> 
> <!ENTITY phishingBlocked.title "Suspected Web Forgery!">
> <!ENTITY phishingBlocked.longDesc "
> <p>Entering any personal information on this page may result in identity theft or other fraud.</p>
> <p>These types of web forgeries are used in scams known as phishing attacks, in which fraudulent web pages and emails are used to imitate sources you may trust.</p>
> ">
> 
>+<!ENTITY cspFrameAncestorBlocked.title "Blocked by Content Security Policy">
>+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>&brandShortName; prevented this page from loading in this way because the page has a content security policy that disallows it.</p>">

I think that the dom/ version of this DTD doesn't source brand.dtd, and hence can't use &brandShortName; which is a bit insidious - this would pass all tests in Firefox, but give xhtml parse errors on other gecko consumers. IIRC, the dom/ version just uses "The browser" everywhere instead.

ui-r=me with that fixed to match whatever the prevailing style is in that file.
Attachment #423094 - Flags: ui-review?(johnath) → ui-review+
(Assignee)

Comment 11

8 years ago
Created attachment 423411 [details] [diff] [review]
CSP Frame Ancestor Check (v7.5 with revised strings)

Thanks johnath, updated and ready to land.
Attachment #423094 - Attachment is obsolete: true
Attachment #423411 - Flags: ui-review+
Attachment #423411 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/0b3cd1b80f3c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3

Updated

8 years ago
Target Milestone: mozilla1.9.3 → mozilla1.9.3a2

Updated

8 years ago
Blocks: 550865

Updated

8 years ago
Depends on: 553180
Depends on: 553993

Updated

8 years ago
Depends on: 561916
You need to log in before you can comment on or make changes to this bug.