Closed
Bug 515458
Opened 15 years ago
Closed 14 years ago
(CSP) Implement frame ancestor check
Categories
(Core :: DOM: Core & HTML, enhancement, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a2
People
(Reporter: geekboy, Assigned: geekboy)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 7 obsolete files)
22.96 KB,
patch
|
geekboy
:
review+
geekboy
:
ui-review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #420199 -
Flags: review?(jst) → review+
Comment 6•14 years ago
|
||
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•14 years ago
|
||
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 8•14 years ago
|
||
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•14 years ago
|
||
here's another whack, johnath.
Attachment #423083 -
Attachment is obsolete: true
Attachment #423094 -
Flags: ui-review?(johnath)
Attachment #423094 -
Flags: review+
Comment 10•14 years ago
|
||
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•14 years ago
|
||
Thanks johnath, updated and ready to land.
Attachment #423094 -
Attachment is obsolete: true
Attachment #423411 -
Flags: ui-review+
Attachment #423411 -
Flags: review+
Comment 12•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0b3cd1b80f3c
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
Updated•14 years ago
|
Target Milestone: mozilla1.9.3 → mozilla1.9.3a2
You need to log in
before you can comment on or make changes to this bug.
Description
•