1376459 - (CVE-2017-7807) AppCache issues (served on subpath allowed to set FALLBACK for whole origin, allowed to set on svg/xml)

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[avlidienbrunn]

Attachment: source_and_results.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3142.0 Safari/537.36

Steps to reproduce:

It seems like the specification of how App Cache is working is a lot less restrictive compared to Service Workers. 

The following scenarios are possible using an App-Cache manifest in Firefox:

1. a file below a path (u/XYZ) can set FALLBACK for the complete origin
2. an SVG-file can set a manifest (even if mime is image/svg+xml)
3. an XML can set manifest (using xmlns=xhtml)
4. a manifest can be served on any content-type (spec says must be text/application-cache)

This currently affects a bunch of file serving services which uses sandboxed origins (exampleusercontent.com) but still serve inline content on the sandboxed domains. By triggering a cookie-bomb combined with a manifest, you're able to hijack any URL on the sandboxed domain using FALLBACK independently if the files are served from a sub-path on the domain. 

Manifest should (probably) not be possible to set from a subfolder onto the complete origin (this will probably break a bunch of apps tho)

Setting a manifest should probably be restricted to text/html only.

I've set up a test environment at http://avlidienbrunn.se/appcache/tests.html, see attached for full source code and results from other browsers.

FYI:
Chrome fixes:
1: Temporary fix by requiring Content-Type "text/application-cache" for setting FALLBACK outside of the current path, due to breaking existing apps. Currently tracked as 422987.
2: Chose not to fix
3: Chose not to fix
4: Chose not to fix (except in case #1)


Actual results:

1. Manifest was loaded
2. Manifest was loaded
3. Manifest was loaded
4. Manifest was loaded


Expected results:

1. Manifest was not loaded
2. Manifest was not loaded
3. Manifest was not loaded
4. Manifest was not loaded

Comment 1

[:Gijs (he/him)]

Can you link to the chrome issue, and is that public already?

Honza: flagging you given this is an appcache issue.

Comment 2

[avlidienbrunn]

The chrome issue is #69806 and is not public yet (at https://bugs.chromium.org/p/chromium/issues/detail?id=696806, and https://bugs.chromium.org/p/chromium/issues/detail?id=422987).

Comment 3

[Honza Bambas (:mayhemer)]

And this is a duplicate of bug 1036808.

Comment 4

[avlidienbrunn]

Honza: ah, thanks for the info.

It's been 3 years since the bug you linked, and this attack scenario is affecting a lot of sites now:

1. USER visits ATTACKERSITE
2. ATTACKERSITE poisons cache on s3.amazonaws.com by loading a manifest + cookie bomb on https://s3.amazonaws.com/attacker/example.html
3. (some time later) USER visits SITE
4. USER uploads FILE
5. SITE puts FILE on S3
6. USER accesses FILE from SITE like https://s3.amazonaws.com/companybucket/userxyz/file?X-Amz-Signature=EXAMPLE
7. USER instead loads ATTACKER's fallback page, which leaks the X-Amz-Signature
8. FILE is leaked to ATTACKER

Any chance for reconsideration? Has there been any update regarding deprecation of appcache (I am guessing that is "current appcache forecast")?

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to avlidienbrunn from comment #4)
> Honza: ah, thanks for the info.
> 
> It's been 3 years since the bug you linked, and this attack scenario is
> affecting a lot of sites now:
> 
> 1. USER visits ATTACKERSITE
> 2. ATTACKERSITE poisons cache on s3.amazonaws.com by loading a manifest +
> cookie bomb on https://s3.amazonaws.com/attacker/example.html
> 3. (some time later) USER visits SITE
> 4. USER uploads FILE
> 5. SITE puts FILE on S3
> 6. USER accesses FILE from SITE like
> https://s3.amazonaws.com/companybucket/userxyz/file?X-Amz-Signature=EXAMPLE
> 7. USER instead loads ATTACKER's fallback page, which leaks the
> X-Amz-Signature
> 8. FILE is leaked to ATTACKER
> 
> Any chance for reconsideration? Has there been any update regarding
> deprecation of appcache (I am guessing that is "current appcache forecast")?

nsHttpChannel::ProcessFallback can perform the check that the fallback target URI is under the same path as the manifest is.  I need to find the proper tools to test this.

Valentin, do you know some simple (or less simple) way how to check if a URI is in a (sub-)path of another URI?  Do we have any helpers?  Are there any escaping considerations (/ -> %something) that would prevent us from the correct recognition of it?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

We have nsIURL.directory - which returns the path excluding the current file.
So I assume checking if it's a subpath would be something like url1.directory.startsWith(url2.directory) or close to it.

Percent encoding may affect this. file:///etc%2Fzsh_command_not_found is essentially equivalent to file:///etc/zsh_command_not_found
I don't recall if we already have a bug for this, but we should probably fix it.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1 (FALLBACK but be inside manifest dir)

- during the manifest parsing we ignore FALLBACK namespace and the fallback page when they are not under the manifest's directory
- and since we have appcaches out there that already have these evil FALLBACK entries cached, we also do a secondary check in http channel for both the namespace and the fallback page

No test, I'm lazy...  Tested locally for both the parser and the http channel check.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #6)
> We have nsIURL.directory

And thanks for this advice, Valentin :)

Comment 9

[Jason Duell]

Comment on attachment 8883079 [details] [diff] [review]
v1 (FALLBACK but be inside manifest dir)

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

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +815,5 @@
> +  return NS_OK;
> +}
> +
> +static nsresult
> +IsFileContainedInPath(nsIURI* file, nsACString const &masterDirectory)

A little odd to not return a bool for a "IsFoo()" function, but no big deal.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (FALLBACK but be inside manifest dir)

Changed the func prefix as implied.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0af5ac9f1db1f6aa74f38e4d1e021175e566b48

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6506888c5f7b25b2a690117fb60fe79a90d274e6

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6506888c5f7b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can we assign a sec rating to this? Does it affect older branches (given comment 3, I assume so), and if so, do we need to consider backporting it?

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Can we assign a sec rating to this? 

According comment 4 which seems plausible, this is sec-high IMO.  OTOH, we have an open longstanding bug about this issue already - bug 1036808.

> Does it affect older branches (given
> comment 3, I assume so), and if so, do we need to consider backporting it?

It does affect old branches.  I believe this is backportable easily (the fix is relatively simple).  Not sure if fixing this would draw larger attention to this issue or not.  If yes, we may want to uplift.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I think we should nominate this for Beta/ESR52 approval.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8885707 [details] [diff] [review]
v1.1 (FALLBACK but be inside manifest dir)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sensitive data disclosure possibility, cache poisoning
User impact if declined: as above
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): hard to assess, this could break some sites using appcache and refering some resources not at/under the appcache manifest path on the server ; hard to say sooner than on m-b/m-r since this is not widely used web tech anymore
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: not a regression
[User impact if declined]: sensitive data disclosure possibility, cache poisoning
[Is this code covered by automated tests?]: we have tests for the functionality apparently passing on m-c
[Has the fix been verified in Nightly?]: yes, I think
[Needs manual test from QE? If yes, steps to reproduce]: probably comment 4, but there is no life test demo available
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: regarding crash rate, no ; regarding web compat, somewhat for badly written pages
[Why is the change risky/not risky?]: patch is simple enough
[String changes made/needed]: none

Comment 17

[Al Billings [:abillings - ex-MoCo]]

This is a rather large patch that shouldn't have been checked into trunk without security approval. I don't think we can take this in 55 and ESR52 (for this cycle) with a single beta left.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Al Billings [:abillings] from comment #17)
> This is a rather large patch that shouldn't have been checked into trunk
> without security approval. 

There was no sec rating done on this bug prior landing it.

Note that this is a known issue and we have an open (by means of not security sensitive) bug for it already - comment 14.

> I don't think we can take this in 55 and ESR52
> (for this cycle) with a single beta left.

Could making comment 4 and 5 hidden help here?  According what I read in comment 1, this broke sites when landed in chrome.

Maybe we end up backing this out.  It's been on Nightly for 14 days so far, tho.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> (In reply to Al Billings [:abillings] from comment #17)
> > This is a rather large patch that shouldn't have been checked into trunk
> > without security approval. 
> 
> There was no sec rating done on this bug prior landing it.

Yes, and per bug approval policy for security bugs, if it has no security rating and affects more than trunk, it *requires* a security rating before it can be checked in.

https://wiki.mozilla.org/Security/Bug_Approval_Process
 
> Maybe we end up backing this out.  It's been on Nightly for 14 days so far,
> tho.

The concern with security approval is that people watch our checkins and reverse engineer them. The genie is out of the bottle now and backing it out won't help with that.

Comment 20

[Honza Bambas (:mayhemer)]

In other words, this patch was probably not supposed to land so far?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Al Billings [:abillings] from comment #19)
> Yes, and per bug approval policy for security bugs

(One note - I got used to the "sec approval needed" note here in bugzilla, not seeing it left me believe there was nothing blocking me; maybe sec bugs w/o a sec-* keyword should have that notification too?)

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Probably a good idea!

We would have landed this but I would have pushed a backport.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8885707 [details] [diff] [review]
v1.1 (FALLBACK but be inside manifest dir)

I want to make a simpler patch to backport with added preference so we can disable this in case sites get broken.

Al, the effective part of this patch are the changes to nsHttpChannel.cpp, only.  That's what prevents the attack.

The parts in the offline manifest are there only to prevent caching of manifests that are found broken (potentially trying to inject the attack).

Comment 24

[Honza Bambas (:mayhemer)]

c23

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: beta v1 (simplified)

Approval Request Comment - see comment 16.

This adds the check only to the http channel (we disallow fallback when the namespace or the resource is outside the manifest path) and *doesn't* fail the cache update process (under the same conditions).

This also adds a deliberately hidden preference users can switch in an emergency if some safe sites get broken with this patch.  (Not sure this bit needs a review or not.)

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: esr52 v1 (functionally identical to v1 beta)

same as the beta version, merged.  see comment 16 for a?.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8891366 [details] [diff] [review]
beta v1 (simplified)

sec-high issue with app cache, beta55+, should be in 55 rc1

It seems worth not keeping release vulnerable for 7 more weeks

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5cb6429eb36

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 8891368 [details] [diff] [review]
esr52 v1 (functionally identical to v1 beta)

app cache sec fix for 52.3 esr

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a86c77d533ee

Comment 31

[Emil Ghitta, QA [:emilghitta]]

Hi avlidienbrunn!

Could you please help me test if this issue was fixed?

I am in a bit of a struggle trying to reproduce this issue.

Thanks so much !