1367531 - (CVE-2017-7808) CSP frame-ancestors should not compare paths per CSP-3

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1364859 +++

See bug 1364859 comment 5 and bug 1364859 comment 9: Firefox accepts origins with paths in frame-ancestors and compares against those paths. This is actually correct according to CSP-2 if you follow the definitions of host-source, but I don't think that was intentional: frame-ancestors was intended to be a "better  X-Frame-Options" which operates only on origins.

This creates a cross-origin info leak, though, and in the latest CSP spec frame-ancestors is explicitly defined as being only origin-based

Comment 1

[Jun (Request needinfo for comment, I ignore other updates)]

As per https://wiki.mozilla.org/Security_Severity_Ratings, "Vulnerabilities which can provide an attacker additional information" is sec-moderate. As https://bugzilla.mozilla.org/show_bug.cgi?id=1364859 is sec-moderate and Christoph said "leaking the path seems like the bigger problem to me". So I think this should be sec-moderate.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to s.h.h.n.j.k from comment #1)
> As per https://wiki.mozilla.org/Security_Severity_Ratings, "Vulnerabilities
> which can provide an attacker additional information" is sec-moderate. As
> https://bugzilla.mozilla.org/show_bug.cgi?id=1364859 is sec-moderate and
> Christoph said "leaking the path seems like the bigger problem to me". So I
> think this should be sec-moderate.

Given that this only affects sites that use CSP frame-ancestors, and the limited amount of information leaked, this seems like a sec-low (or even lower to be honest). But I'll set the bounty? just in case so that its discussed by bounty committee.

Comment 3

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to Paul Theriault [:pauljt] from comment #2)
> Given that this only affects sites that use CSP frame-ancestors, and the
> limited amount of information leaked, this seems like a sec-low (or even
> lower to be honest). But I'll set the bounty? just in case so that its
> discussed by bounty committee.

Thanks for adding th flag. This affects any sites which loads untrusted frames (e.g. ads). It's attacker who uses frame-ancestors to leak path not victim.

Comment 4

[Daniel Veditz [:dveditz]]

In practice don't the paths leak through the referrer anyway? Very few sites take advantage of the Referrer Policy ability to minimize referrer information.

Comment 5

[Jun (Request needinfo for comment, I ignore other updates)]

In practice, yes. But the issue is that even if site uses Referrer Policy, attacker can use this bug to leak path information. Since the biggest issue against implementing ancestorOrigins in Firefox was that it leaks URL even if Referrer Policy is implemented, this could be same concern.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1367531_csp_frame_ancestors_path_leak.patch

Dan, instead of cloning the whole URI of every ancestor, we should just clone the 'prePath' and compare against that. We then also need to slightly tweak the actual enforcment mechanism, because so far the CSP backend only had to deal with full blown URLs. Anyway, just for the sake of completeness, we should not remove the path within frame-ancestors in the CSP Parser. Mostly because CSP reports need (should) report the policy that was shipped with the page.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1367531_csp_frame_ancestors_test_updates.patch

I think it's sufficient to only update that one test to make sure 'paths' for frame-ancestors are ignored. Agreed?

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8872460 [details] [diff] [review]
bug_1367531_csp_frame_ancestors_path_leak.patch

Review of attachment 8872460 [details] [diff] [review]:
-----------------------------------------------------------------

unfortunately r-

::: dom/security/nsCSPContext.cpp
@@ -1281,5 @@
>  
> -      // We don't care if this succeeds, just want to delete a userpass if
> -      // there was one.
> -      uriClone->SetUserPass(EmptyCString());
> -

PrePath includes the userinfo (see https://searchfox.org/mozilla-central/source/netwerk/base/nsIURI.idl#106). You can't get rid of this bit

::: dom/security/nsCSPUtils.cpp
@@ +709,5 @@
> +      // In case the URI does not have a path, we should not enforce
> +      // path matching. In practice, that should only be the case
> +      // for frame-ancestors because we strip the path before enforcement
> +      // to not leak any path information.
> +      return true;

Is this because nsCSPHostSrc doesn't know what kind of policy it's enforcing?

"/" is a perfectly legal (and common!) path and is not the same thing as having no path whatsoever. If I know an empty path is going to get off free then I'd set up fake domains and make "https://resource0001.evil.com/" return the script (or whatever) I needed to slip by a path restriction. OK, in practice the host checks _probably_ saves us here because it'd be hard to replace the root of a legit host, but the check is still doing the wrong thing.

Where is the parsing of the frame-ancestors directive done? Could we special-case _there_ to simply ignore entries with a path? Is that even the correct behavior now, or are we supposed to interpret "host-with-path" as "host"

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8872461 [details] [diff] [review]
bug_1367531_csp_frame_ancestors_test_updates.patch

Review of attachment 8872461 [details] [diff] [review]:
-----------------------------------------------------------------

maybe r-? this matches your code but I think the patch is wrong.

Also you messed with code dealing with userinfo, so we should have a test that has userinfo in it to make sure it's ignored.

::: dom/security/test/csp/file_frameancestors_main.js
@@ +11,5 @@
>  
> +  // In both cases (base.a, base.b) the path starts with /tests/. Let's make sure this
> +  // path within the CSP policy is completely ignored when enforcing frame ancestors.
> +  // To test this behavior we use /foo/ and /bar/ as dummy values for the path.
> +  var host = { a: 'http://mochi.test:8888/foo/', b: 'http://example.com:80/bar/' };

I don't know that this is right. What's the expected behavior? This is allowing paths in the CSP and then not enforcing them (ignoring the paths). Call this option (a). Alternate behaviors:

(b) ignore frame-ancestor directives that specify a path, just as we ignore unknown keywords. Under this interpretation this CSP will now no longer do anything. Actually in this example we'd end up with a directive and no usable sources which should be interpreted as 'none'!

(c) enforce the path against a path-stripped ancestor--in other words this host entry always blocks and there had better be another valid host entry in this directive.

(c) is pretty strict and maybe undesirable, but at least the errors would be known quickly. (b) Would lead to some breakage (especially if all directive sources have paths, would == 'none'), (a) might lead to allowing some sites (silently) that the author didn't expect. Do we know what Chrome does?

Looking at the spec https://w3c.github.io/webappsec-csp/#frame-ancestors-navigation-response in step 4.3 I think that is saying (c) is the correct behavior (a source-with-path at redirect count 0 matched against an origin will always be blocked).

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1367531_csp_frame_ancestors_path_leak.patch

(In reply to Daniel Veditz [:dveditz] from comment #8)
> > -      uriClone->SetUserPass(EmptyCString());
> You can't get rid of this bit

Good catch, I would have missed that. I put it back within this patch.
 
> "/" is a perfectly legal (and common!) path and is not the same thing as
> having no path whatsoever. If I know an empty path is going to get off free
> then I'd set up fake domains and make "https://resource0001.evil.com/"
> return the script (or whatever) I needed to slip by a path restriction.

You as an attacker can't influence the ancestor URI, so that wouldn't work. But anyway, not important anymore. I updated the patch to incorporate some changes in the CSP Parser. Please note that this patch is slightly hacky in a sense that our architecture does not allow any SRCs to know in what directive they are in. We should rearchitecture that at some point. This patch basically provides that information, but only for the frame-ancestors directive so that in the backend we know that we should ignore path comparison within nsCSPHostSrc when enforcing FRAME_ANCESTORS_DIRECTIVE.

A few more notes:
* Updating the parser to only ignore paths for frame-ancestors seems more painful to implement, hence the flag seems more appropriate.
* Ideally we should not mess with the delivered CSP so CSP reports can include the 'original' CSP.
* As far as testing goes, I think the updates to test_frameancestors_main.js in the other patch are accurate, hence I kindly ask you to revisit that patch given the updates I made to the parser.
* If we need a test for userpass, I am happy to file a followup for that.

Thanks for your initial feedback and review comments!

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 8873425 [details] [diff] [review]
bug_1367531_csp_frame_ancestors_path_leak.patch

Review of attachment 8873425 [details] [diff] [review]:
-----------------------------------------------------------------

Like the previous patch this implements approach (a) while I think the spec is saying (c). Approach (a) does have the advantage that "frame-ancestors 'self';" gives a sensible result without special-casing the interpretation of 'self' as an origin for that one directive. If Chrome has implemented (c) and we process a policy that works in Chrome it will also work in Firefox. If an author writea a policy for firefox like "frame-ancestors https://www.google.com/maps/" it will actually allow them to be framed anywhere on www.google.com (silently!) which might be a surprise, and then won't work when visited by Chrome. Probably not much risk of that in practice.

If (a) is the behavior we want (or is at least good enough) then this patch does that correctly. r=dveditz

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Thanks Dan! What do we do about the testcase? It's r- at the moment - are you considering to revise your decision? If not, please let me know what kind of testcase you would prefer. Thanks!

Comment 13

[Daniel Veditz [:dveditz]]

I guess I was hoping you'd test the userinfo thing: even though you put the change back a test would catch someone who breaks it in the future. But then you'd probably have to figure out how to suppress the anti-phishing prompt we have for those kinds of urls (it's probably a pref).

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #13)
> I guess I was hoping you'd test the userinfo thing:

I filed Bug 1370468 to write that test in a follow up.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4cbc063a8e7
https://hg.mozilla.org/mozilla-central/rev/ebc4d874b576

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something that can ride the 55 train or should we consider it for backport to ESR52 for the 52.3 release in August?

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Is this something that can ride the 55 train or should we consider it for
> backport to ESR52 for the 52.3 release in August?

The patch itself is pretty straight forward, meaning that backporting would be fine if needed. Given that we have been shipping frame-ancestors this way for quite so many years, and also that we consider this bug to be a sec-moderate, I think we should just let it ride the train. (thanks for checking though).

Comment 18

[Daniel Veditz [:dveditz]]

Originally reported by s.h.h.n.j.k in bug 1364859, awarding bug bounty to him for this aspect of it.

Comment 19

[Jun (Request needinfo for comment, I ignore other updates)]

Am I allowed to publish PoC for this after Firefox 55 is released? I'm asking this because https://bugzilla.mozilla.org/show_bug.cgi?id=1364859 won't be fixed in 55 and publishing PoC for this bug could give idea of stealing other URL parts too.