Closed Bug 561051 Opened 14 years ago Closed 14 years ago

CSP frame-ancestors check not performed for URLs loaded with view-source: scheme

Categories

(Core :: DOM: Core & HTML, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- -
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: bugzilla, Assigned: bsterne)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:low])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100421 Minefield/3.7a5pre (.NET CLR 3.5.30729)
Build Identifier: 

Pages that use frame-ancestors to prevent themselves from being framed can still be loaded in an iframe by using the view-source: protocol. This will cause the source of the page to be displayed instead of the rendered page. e.g

http://hackmill.com/csp/tests/resources/frame.php will not load in an iframe but
view-source:http://hackmill.com/csp/tests/resources/frame.php will.

Href attributes are linkified with view-source so they may be vulnerable to clickjacking. Additionally, the source may be stolen using the 'content extraction' technique (see http://www.blackhat.com/html/bh-eu-10/bh-eu-10-archives.html#Stone)




Reproducible: Always
Blocks: CSP
Attached file Testcase
Thought we had this bug filed already but can't find it. CSP needs to handle nested URI scheme, and should do so using the protocol handler flags rather than hard-coding checks for view-source: and jar: because add-ons or future features can always create new schemes. (Some about: uris look like they're implemented as nested, too)

In C++ we call NS_GetInnermostURI(); in JS you should be able to get the innermostURI property of a nsINestedURI (if the URI is one). For example,
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2524

I'm going to go out on a limb and guess we've got the same problem with the X-Frame-Options implementation, and that it will be fixed the same way in basically the same place so we might as well use the same bug and fix both at the same time. If it does please update the summary to cover both.

The general nested URI issue for loading sub-content might belong in a separate bug, it's certainly going to require completely different test cases.
Assignee: nobody → bsterne
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: maybe X-Frame-Options too?
If we're pushing X-F-O back into 3.6.9 we'll need this fixed there, too.
blocking1.9.2: --- → ?
blocking2.0: --- → ?
blocking1.9.2: ? → .9+
Setting this as blocking because bug 475530 is in and blocking.
This doesn't block 1.9.2 since this bug is about CSP which isn't on 1.9.2.  I _do_ think it blocks 2.0, but I don't have the permissions to set that flag.

I filed bug 586891 on the same issue for X-Frame-Options and set that to blocking 1.9.2, though I don't have a patch yet.  I'll try to get something together in the next 12 hours or so, but it won't be before code freeze :-(

Sid's also a better owner for this bug since he implemented frame-ancestors originally and is more familiar with the code.
Assignee: bsterne → sstamm
blocking1.9.2: .9+ → ---
Whiteboard: maybe X-Frame-Options too?
Whiteboard: [sg:low]
Depends on: 586891
I've got a patch that fixes this.  Coming up shortly.
Assignee: sstamm → bsterne
Blocks: 586891
No longer depends on: 586891
Attached patch fix (obsolete) — Splinter Review
How does this look, geekboy?  The view-source: angle to resource loading has exposed some interesting corner cases that CSP doesn't handle very elegantly. With this patch note:

1) When <iframe src="view-source:page-restricted-by-frame-ancestors"> is blocked by CSP, we don't have a way of reporting the HTTP version number, since we don't have a nsIHttpChannelInternal to go off of.

2) Paul's testcase passes, but a simpler version of the same test using a data: document as the parent document results in the _request for the frame_ being blocked by our contentPolicy rather than the neterror page being displayed.  We are treating the request as a violation of the parent document's CSP, which doesn't make sense since the parent is a data: document and has no CSP.  Load the following to demonstrate:
data:text/html,<iframe src="view-source:http://hackmill.com/csp/tests/resources/frame.php"></iframe>
Attachment #467487 - Flags: review?(sstamm)
As filed, this doesn't block. If this affects other nested protocols such as jar:, it should block, but showing the source of somebody else's page in an iframe doesn't seem like something we need to be really strict about.
blocking2.0: ? → -
This probably should block. The source code of a view-source page can be stolen using drag-and-drop clickjacking techniques. Basically if a malicious page can get a user to perform two drags (in no particular direction) it can get the entire source. 

URLs in view-source are also linkified so are draggable and clickable. With a single drag a page could steal a link URL (e.g a link containing an anti-CSRF token). With a single click it could get the user to follow a link URL, which may have undesired consequences

A good example is the source of a GMail mobile page: view-source:https://mail.google.com/mail/x/. It contains email IDs and subjects, anti-CSRF tokens and a clickable logout link.

My slides describe this in more detail: https://media.blackhat.com/bh-eu-10/presentations/Stone/BlackHat-EU-2010-Stone-Next-Generation-Clickjacking-slides.pdf see page 28 onwards.
Comment on attachment 467487 [details] [diff] [review]
fix

(In reply to comment #7)
> 2) Paul's testcase passes, but a simpler version of the same test using a data:
> document as the parent document results in the _request for the frame_ being
> blocked by our contentPolicy rather than the neterror page being displayed.  We
> are treating the request as a violation of the parent document's CSP, which
> doesn't make sense since the parent is a data: document and has no CSP.  Load
> the following to demonstrate:
> data:text/html,<iframe
> src="view-source:http://hackmill.com/csp/tests/resources/frame.php"></iframe>

I can't reproduce the strange behavior where the ContentPolicy stops the load... when I apply your patch and try out the "data:<iframe ..." case, my error console shows "frame-ancestors" being violated, which is what is expected.

There's still subtle weirdness in the way the "blocked URI" is reported (as the whole data: URI), but I think the behavior is right.

In contentSecurityPolicy.js:permitsAncestry() (http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#315), an ancestor who violates the policy is reported as the "blocked" URI because it's the only way to report what violated frame-ancestors.  Maybe this is a remnant from when the violation report called the blocked URI the "violator" or something.  The basic idea is that we want to report what URI caused the violation, and this is how it can be done in the unique case of frame ancestors.

Should we consider renaming "blocked-uri" to "violating-uri", or could we special case the violation report for frame-ancestors to call it "incompatible-ancestor"? (This is a bug in the spec, I guess, not the code).

Your patch looks pretty straightforward.  Ideally, I'd like to see the HTTP version numbers always present, but since nsIHttpChannelInternal is not frozen, I guess the best we can do is try.

r=geekboy
Attachment #467487 - Flags: review?(sstamm) → review+
> since we don't have a nsIHttpChannelInternal to go off of.

You could make nsViewSourceChannel implement it just like it does nsIHttpChannel.
Attached patch fix v2 (obsolete) — Splinter Review
Make nsViewSourceChannel implement nsIHttpChannelInternal
Attachment #467487 - Attachment is obsolete: true
Attachment #480361 - Flags: review?(bzbarsky)
Comment on attachment 480361 [details] [diff] [review]
fix v2

>+// nsIHttpChannelInternal methods

Why do you need all that?  Does doing what view-source does with mCachingChannel, say, not work?  It certainly doesn't look like you have any not-trivially-forwarding implementations here.
Attachment #480361 - Flags: review?(bzbarsky) → review-
Attached patch fix v3Splinter Review
Neat, I didn't realize we had macros to handle this stuff.  Added a test as well.
Attachment #480361 - Attachment is obsolete: true
Attachment #481299 - Flags: review?(bzbarsky)
Comment on attachment 481299 [details] [diff] [review]
fix v3

r=me.  Thanks!
Attachment #481299 - Flags: review?(bzbarsky) → review+
Attachment #481299 - Flags: approval2.0?
Attachment #481299 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/c5494ee56c47
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 605077
No longer depends on: 605077
Depends on: 608662
Works as expected on Latest Nightly 26 BuildID: 20130828030202
Status: RESOLVED → VERIFIED
QA Contact: mihai.morar
You need to log in before you can comment on or make changes to this bug.