Closed Bug 515458 Opened 15 years ago Closed 14 years ago

(CSP) Implement frame ancestor check

Categories

(Core :: DOM: Core & HTML, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2

People

(Reporter: geekboy, Assigned: geekboy)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch CSP Frame Ancestor Check (v7) (obsolete) — Splinter Review
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
Attached patch CSP Frame Ancestor Check (v7.1) (obsolete) — Splinter Review
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
Attached patch CSP Frame Ancestor Check (v7.2) (obsolete) — Splinter Review
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
Attached patch CSP Frame Ancestor Check (v7.4) (obsolete) — Splinter Review
Includes some bug fixes and modifications based on brief, informal reviews. (Also had pretty nasty bit rot.)
Attachment #403672 - Attachment is obsolete: true
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)
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.
Attached patch CSP Frame Ancestor Check (v7.5) (obsolete) — Splinter Review
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-
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+
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
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Target Milestone: mozilla1.9.3 → mozilla1.9.3a2
Blocks: 550865
Depends on: 553180
Depends on: 553993
Depends on: 561916
You need to log in before you can comment on or make changes to this bug.