Bug 1376459 (CVE-2017-7807)

AppCache issues (served on subpath allowed to set FALLBACK for whole origin, allowed to set on svg/xml)

RESOLVED FIXED in Firefox -esr52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: avlidienbrunn, Assigned: mayhemer, NeedInfo)

Tracking

(Depends on: 1 bug, {sec-high})

54 Branch
mozilla56
sec-high
Points:
---
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56- fixed)

Details

(Whiteboard: [necko-active][adv-main55+][adv-esr52.3+][post-critsmash-triage])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

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

Honza: flagging you given this is an appcache issue.
Group: firefox-core-security → core-security
Component: Untriaged → Networking: Cache
Flags: needinfo?(honzab.moz)
Flags: needinfo?(avlidienbrunn)
Product: Firefox → Core
(Reporter)

Comment 2

2 years ago
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).
Flags: needinfo?(avlidienbrunn)
(Assignee)

Comment 3

2 years ago
And this is a duplicate of bug 1036808.
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 4

2 years ago
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")?
Flags: needinfo?(honzab.moz)
Group: core-security → network-core-security
(Assignee)

Comment 5

2 years ago
(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?
Flags: needinfo?(honzab.moz) → needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
(Assignee)

Updated

2 years ago
Assignee: nobody → honzab.moz
Whiteboard: [necko-active]
(Assignee)

Comment 7

2 years ago
- 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.
Attachment #8883079 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 8

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

And thanks for this advice, Valentin :)

Comment 9

2 years ago
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.
Attachment #8883079 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6506888c5f7b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: network-core-security → core-security-release
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?
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 14

2 years ago
(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.
Flags: needinfo?(honzab.moz)
Keywords: sec-high
status-firefox54: --- → wontfix
status-firefox55: --- → affected
status-firefox-esr52: --- → affected
I think we should nominate this for Beta/ESR52 approval.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 16

2 years ago
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
Flags: needinfo?(honzab.moz)
Attachment #8885707 - Flags: approval-mozilla-esr52?
Attachment #8885707 - Flags: approval-mozilla-beta?
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.
(Assignee)

Comment 18

2 years ago
(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.
(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.
(Assignee)

Comment 20

2 years ago
In other words, this patch was probably not supposed to land so far?
(Assignee)

Comment 21

2 years ago
(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?)
Probably a good idea!

We would have landed this but I would have pushed a backport.
(Assignee)

Comment 23

2 years ago
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).
Attachment #8885707 - Flags: approval-mozilla-esr52?
Attachment #8885707 - Flags: approval-mozilla-beta?
(Assignee)

Comment 24

2 years ago
c23
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 25

2 years ago
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.)
Flags: needinfo?(honzab.moz)
Attachment #8891366 - Flags: approval-mozilla-beta?
(Assignee)

Comment 26

2 years ago
same as the beta version, merged.  see comment 16 for a?.
Attachment #8891368 - Flags: approval-mozilla-esr52?
(Assignee)

Updated

2 years ago
Attachment #8891368 - Attachment description: v1 (esr52) → esr52 v1 (functionally identical to v1 beta)
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
Attachment #8891366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8891368 [details] [diff] [review]
esr52 v1 (functionally identical to v1 beta)

app cache sec fix for 52.3 esr
Attachment #8891368 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/a86c77d533ee
status-firefox-esr52: affected → fixed
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
tracking-firefox-esr52: --- → ?
tracking-firefox55: ? → +
tracking-firefox56: ? → -
tracking-firefox-esr52: ? → 55+
Alias: CVE-2017-7807
Whiteboard: [necko-active] → [necko-active][adv-main55+][adv-esr52.3+]
Flags: qe-verify?
Whiteboard: [necko-active][adv-main55+][adv-esr52.3+] → [necko-active][adv-main55+][adv-esr52.3+][post-critsmash-triage]
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 !
Flags: needinfo?(avlidienbrunn)
(Assignee)

Updated

2 years ago
Depends on: 1405719
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.