Closed Bug 1586630 (CVE-2020-12415) Opened 5 years ago Closed 4 years ago

Appcache fallback can be corrupted allowing manifests to poison an entire origin

Categories

(Core :: Networking: Cache, defect, P1)

Product:
Core
Component:
Networking: Cache
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox-esr68 --- wontfix
firefox-esr78 78+ fixed
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 + fixed

People

(Reporter: pauljt, Assigned: valentin)

Details

(Keywords: sec-high, Whiteboard: [necko-triaged][post-critsmash-triage][sec-survey][adv-main78+])

Attachments

(2 files, 1 obsolete file)

Bug 1586630 - Add check to GetURIDirectory r=mayhemer
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta-
dveditz
: sec-approval+
Details | Review
advisory.txt
300 bytes, text/plain
Details

There is an issue in our appcache implementation that allows a manifest served from a subdirectory, to confuse the browser into using the appcache to requests to the top level directory. (i.e. we dont enforced the rule that appcache is only supposed to be able to be responsible for requests to sub-directories.

This bug was confirmed by Honza. From email:

I quickly confirmed on IIS 10 with statically served content: when the <html manifest> attribute value is in a form of "/dir%2fsubdir%2fcache.manifest" and the manifest, served at /dir/subdir/cache.manifest, has FALLBACK: / some-evil-resource-url, we load the evil resource for anything that can't be found on the web site for the origin the document was loaded in.

An example of a real world attack scenario (which I'm not linking, so as not to draw attention) is given in https://bugzilla.mozilla.org/show_bug.cgi?id=1376459#c4.

While appcache is close to deprecation, as we have had external reports here of this issue, I'm filing this bug to consider if a fix is need in the short term, or if we can just push for appcache deprecation (1237782 ). The consensus from the discussion so far is that deprecation will not make ESR68, so we make need an interim fix.

As for rating, I'd argue that if you are using a shared origin for sensitive data, you are asking for trouble. But since there might be a real world case here I'm rating this as sec-high (regardless of whose at fault).

Keywords: sec-high

(In reply to Paul Theriault [:pauljt] (needinfo pls) from comment #0)

There is an issue in our appcache implementation that allows a manifest served from a subdirectory, to confuse the browser into using the appcache to requests to the top level directory. (i.e. we dont enforced the rule that appcache is only supposed to be able to be responsible for requests to sub-directories.

This bug was confirmed by Honza. From email:

I quickly confirmed on IIS 10 with statically served content: when the <html manifest> attribute value is in a form of "/dir%2fsubdir%2fcache.manifest" and the manifest, served at /dir/subdir/cache.manifest, has FALLBACK: / some-evil-resource-url, we load the evil resource for anything that can't be found on the web site for the origin the document was loaded in.

An example of a real world attack scenario (which I'm not linking, so as not to draw attention) is given in https://bugzilla.mozilla.org/show_bug.cgi?id=1376459#c4.

That was a general case that have been fixed by bug 1376459 - to force fallback entries to be in the directory or a subdir of the manifest.

The problem that we are considering here is escaping of the path that can bypass that check easily (as I demoed in the email). If you make a request to https://foo.com/dir1/dir2/resource it may or may not be the same as https://foo.com/dir1%2fdir2%2fresource (or its variations). For IIS (static) it is the same path, but we see the Directory as '/'; for Apache (static/rewrite) it is not the same path (we are safe there, the attack is not possible). I haven't tested e.g. nginx static/rewrite path escaping behavior, but I probably don't have to.

Based on the different interpretation of a path with escaped slashes by servers, we can't canonicalize the path by unescaping (%2f -> '/') as that would lead to a different resource being requested on, specifically, Apache servers.

We could do the unescaping exclusively for appcache resources (and force reload/cleaning/fallback entries cleanup of existing users' appcaches?) to mitigate this attack. I don't think that modifying the Directory getter on nsIURI(L?) behavior generally to always unescape would be a good idea.

The only proper way IMO is to disable appcache.

While appcache is close to deprecation, as we have had external reports here of this issue, I'm filing this bug to consider if a fix is need in the short term, or if we can just push for appcache deprecation (1237782 ). The consensus from the discussion so far is that deprecation will not make ESR68, so we make need an interim fix.

Group: core-security

P3, but if I hear voices this should be fixed soon-ish, we can re-prioritize.

Priority: -- → P3
Whiteboard: [necko-triaged]

Not sure what the status is of this, but it's clearly not getting fixed in any active releases.

status-firefox72: affected → wontfix
status-firefox73: affected → wontfix
status-firefox74: --- → fix-optional
status-firefox-esr68: --- → wontfix
Assignee: nobody → valentin.gosu

Valentin, if we aim for progress here and not perfection, I think just unescaping the URL will do. What do you think? Fixing this bug only makes sense if we want to leave appcache on for ESR 78, IMO.

[Tracking Requested - why for this release]:
We would like to have at least an imperfect mitigation for 78, which is ESR. Appcache will be left enabled for 78 ESR and this is the last well known security bug in appcache.

status-firefox78: --- → affected
tracking-firefox78: --- → ?

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Priority: P3 → P2

P1 for 78

Priority: P2 → P1

It seems this is the easiest fix to the problem.
We could also add the check to nsContentUtils::GetOfflineAppManifet, so that we don't even parse a manifest that is served from a path containing %2f.
What do you think?

Attachment #9149856 - Attachment description: Bug 1586630 - [DO_NOT_LAND] Test that the intercepted resources are limited to the manifest's directory → Bug 1586630 - [DO_NOT_LAND] Test that the intercepted resources are limited to the manifest's directory r=mayhemer

Comment on attachment 9150419 [details]
Bug 1586630 - Add check to GetURIDirectory r=mayhemer

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Relatively easily.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Applies cleanly on esr68
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause any regressions.
    Minimal testing required - mostly just to check that appcache websites still work as expected.
Attachment #9150419 - Flags: sec-approval?

Comment on attachment 9150419 [details]
Bug 1586630 - Add check to GetURIDirectory r=mayhemer

sec-approval+ to get this into Fx78

Attachment #9150419 - Flags: sec-approval? → sec-approval+

https://hg.mozilla.org/mozilla-central/rev/4626bc2c944d

Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox78: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Comment on attachment 9150419 [details]
Bug 1586630 - Add check to GetURIDirectory r=mayhemer

Beta/Release Uplift Approval Request

  • User impact if declined:
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is minimal - we now return an error code if the manifest's path contains %2f or %2F
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
  • User impact if declined: Potential for an attacker to hijack resources on the same server that they should not be able to.
  • Fix Landed on Version: 78
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is minimal - we now return an error code if the manifest's path contains %2f or %2F
  • String or UUID changes made by this patch:
Attachment #9150419 - Flags: approval-mozilla-esr68?
Attachment #9150419 - Flags: approval-mozilla-beta?

Comment on attachment 9150419 [details]
Bug 1586630 - Add check to GetURIDirectory r=mayhemer

77 is already built, and the next beta will be 78 after the merge next week.

Attachment #9150419 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Valentin, is there a reason to want this patch in esr68? It landed in 78 so it will be part of the next ESR which is what was requested in comment #6 and it had been set as wontfix for esr68 previously.

Flags: needinfo?(valentin.gosu)

(In reply to Pascal Chevrel:pascalc from comment #18)

had been set as wontfix for esr68 previously.

Oh, I missed that. Then we probably don't need it in 68.

Flags: needinfo?(valentin.gosu)
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-critsmash-triage]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage][sec-survey]

Done

Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-triaged][post-critsmash-triage][sec-survey] → [necko-triaged][post-critsmash-triage][sec-survey][adv-main78+]
Attached file advisory.txt —

Credit to Kevin Higgs, this issue was passed to us by a third party.

Alias: CVE-2020-12415
Attachment #9149856 - Attachment is obsolete: true
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: