Closed Bug 1376036 (CVE-2017-7814) Opened 2 years ago Closed 2 years ago

Application Reputation checks don't cover blob: and data: URLs

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 56+ fixed
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 + fixed

People

(Reporter: francois, Assigned: francois)

References

(Depends on 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage])

Attachments

(3 files, 2 obsolete files)

Steps to reproduce:

1. Download content.exe from https://testsafebrowsing.appspot.com/ (#2 under "Desktop Download Warnings") and unblock the file.
2. Upload it on https://p2p.dev.lcip.org/
3. Copy and paste the resulting sharing link into the URL bar and click the download button.

Expected:

The file is blocked by download protection. (Chrome does this)

Actual:

The file is not blocked.


This issue was discussed publicly on https://github.com/mozilla/something-awesome/issues/90 but without identifying it as a client bug.

We should also think about how we should handle these schemes (if at all):

- data: problematic since a remote lookup would send the content to Google (since the content is in the URL)
- filesystem: ???
- about: skip?
- javascript: skip?
Running Nightly with MOZ_LOG="ApplicationReputation:5", I get:

[Main Thread]: D/ApplicationReputation Application reputation service started up
[Main Thread]: D/ApplicationReputation Starting application reputation check [query=0x7f156f0f29e0]
[Main Thread]: D/ApplicationReputation Created pending lookup [this = 0x7f15788d3400]
[Main Thread]: D/ApplicationReputation Application Reputation verdict is 0, obtained in 0.004800 ms [this = 0x7f15788d3400]
[Main Thread]: D/ApplicationReputation Application Reputation check passed [this = 0x7f15788d3400]
[Main Thread]: D/ApplicationReputation Destroying pending lookup [this = 0x7f15788d3400]

instead of what I would get if the download was going through the Application Reputation checks:

[Main Thread]: D/ApplicationReputation Starting application reputation check [query=0x7f156e122200]
[Main Thread]: D/ApplicationReputation Created pending lookup [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation ApplicationReputation: Got 0 redirects
[Main Thread]: D/ApplicationReputation Created pending DB lookup [this = 0x7f156e2cc520]
[Main Thread]: D/ApplicationReputation Checking principal http://testsafebrowsing.appspot.com/ [this=0x7f156e2cc520]
[Main Thread]: D/ApplicationReputation Checking DB service for principal http://testsafebrowsing.appspot.com/ [this = 0x7f156e2cc520]
[Main Thread]: D/ApplicationReputation Didn't find principal http://testsafebrowsing.appspot.com/ on any list [this = 0x7f156e2cc520]
[Main Thread]: D/ApplicationReputation Created pending DB lookup [this = 0x7f156e573190]
[Main Thread]: D/ApplicationReputation Checking principal http://testsafebrowsing.appspot.com/s/content.exe [this=0x7f156e573190]
[Main Thread]: D/ApplicationReputation Checking DB service for principal http://testsafebrowsing.appspot.com/s/content.exe [this = 0x7f156e573190]
[Main Thread]: D/ApplicationReputation Destroying pending DB lookup [this = 0x7f156e2cc520]
[Main Thread]: D/ApplicationReputation Didn't find principal http://testsafebrowsing.appspot.com/s/content.exe on any list [this = 0x7f156e573190]
[Main Thread]: D/ApplicationReputation Suggested filename: content(3).exe [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation Sending remote query for application reputation [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation Got unsigned binary for remote application reputation check [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation Serialized protocol buffer [this = 0x7f1579350140]: (length=210) 
1http://testsafebrowsing.appspot.com/s/content.exe"
 
 �p��J����)]��N������WQ~�"[
1http://testsafebrowsing.appspot.com/s/content.exe
[Main Thread]: D/ApplicationReputation Destroying pending DB lookup [this = 0x7f156e573190]
[Main Thread]: D/ApplicationReputation Application Reputation verdict is 1, obtained in 286.559686 ms [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation Application Reputation check failed, blocking bad binary [this = 0x7f1579350140]
[Main Thread]: D/ApplicationReputation Destroying pending lookup [this = 0x7f1579350140]
This is the call that fails:

nsresult
PendingLookup::GetStrippedSpec(nsIURI* aUri, nsACString& escaped)
{
  // If aURI is not an nsIURL, we do not want to check the lists or send a
  // remote query.
  nsresult rv;
  nsCOMPtr<nsIURL> url = do_QueryInterface(aUri, &rv);
  NS_ENSURE_SUCCESS(rv, rv);

which is likely due to the URL being weird (a blob URL is an nsIURI but not an anIURL) as Danny hinted on
https://github.com/mozilla/something-awesome/issues/90#issuecomment-309929019
Attached patch add-extra-logging.patch (obsolete) — Splinter Review
Attachment #8880981 - Flags: review?(dlee)
Attachment #8880981 - Flags: review?(dlee) → review+
Assignee: nobody → francois
Here's a simpler test case that shows that both blob: and data: URIs are affected.
For what it's worth, ftp URLs are not affected. Downloads hosted on that scheme go through the necessary checks.
Summary: Application Reputation checks don't cover blob: URLs → Application Reputation checks don't cover blob: and data: URLs
Attached patch Patch against mozilla-central (obsolete) — Splinter Review
Attachment #8880981 - Attachment is obsolete: true
Attachment #8884467 - Flags: review?(gpascutto)
Once we have a Marionette test for download protection (bug 1237766), we should add these two test cases.
Depends on: 1237766
Dan, do you think that's something we should fix on release and ESR?
Status: NEW → ASSIGNED
Flags: needinfo?(dveditz)
Comment on attachment 8884467 [details] [diff] [review]
Patch against mozilla-central

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +1003,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = cryptoHash->Init(nsICryptoHash::SHA256);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  const char* rawSpecBytes = PromiseFlatCString(aSpec).get();

There's no need to round-trip through FlatCString here. BeginReading()+Length() on nsACString would work.

@@ +1018,5 @@
> +  static const char* const hex = "0123456789ABCDEF";
> +  hexEncodedHash.SetCapacity(2 * binaryHash.Length());
> +  for (size_t i = 0; i < binaryHash.Length(); ++i) {
> +    auto c = static_cast<const char>(binaryHash[i]);
> +    hexEncodedHash.Append(hex[(c >> 4) & 0x0F]);

Right-shifting a (potentially) signed char is UB.

@@ +1048,5 @@
> +    aUri->GetSpec(escaped);
> +    int32_t comma = escaped.FindChar(',');
> +    if (comma > -1 &&
> +        static_cast<nsCString::size_type>(comma) < escaped.Length() - 1) {
> +      MOZ_ASSERT(comma > 4, "Data URIs start with 'data:'");

Feels a bit weird to have the comma > -1 check above then. We will do the wrong thing without warning in release mode.
Attachment #8884467 - Flags: review?(gpascutto) → review+
(In reply to François Marier [:francois] from comment #9)
> Dan, do you think that's something we should fix on release and ESR?

I don't think we need to uplift it into beta, especially halfway through the cycle. Would be worthwhile to fix on ESR when we fix it on trunk (so likely 52.4 with Fx56, not uplifting now into 52.3).

How does the blob: check work since the URL should be blob:<origin>/<random>? For example, every time I load the testcase I get a different URL. Is that really what Chrome does? Would it make more sense to make a hash of the contents like they do for data: urls?
(In reply to Daniel Veditz [:dveditz] from comment #11)
> How does the blob: check work since the URL should be
> blob:<origin>/<random>? For example, every time I load the testcase I get a
> different URL. Is that really what Chrome does? Would it make more sense to
> make a hash of the contents like they do for data: urls?

For blobs, we do two things:

1. we check the origin (extracted out of the URL) against the blacklist
2. we send a hash of the contents to the server (remote lookup)

We also include, like Chrome, the original (random) URL as part of the remote lookup but as you point out, it's not really useful.

With this patch, our handling of data: and blob: URLs now matches Chrome's.
The URL mentioned in comment 0 changed to https://send.dev.mozaws.net/ and I can confirm that the test file is now blocked by the download manager when it is sent via this service.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> @@ +1048,5 @@
> > +    aUri->GetSpec(escaped);
> > +    int32_t comma = escaped.FindChar(',');
> > +    if (comma > -1 &&
> > +        static_cast<nsCString::size_type>(comma) < escaped.Length() - 1) {
> > +      MOZ_ASSERT(comma > 4, "Data URIs start with 'data:'");
> 
> Feels a bit weird to have the comma > -1 check above then. We will do the
> wrong thing without warning in release mode.

We'll do the same as Chrome, which is truncate the URL to just "data" and then submit the hash of the binary file as normal to the remote server. So the URL won't match anything, but the content hash might.
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Addressing GCP's review comments and carrying his r+.

Interdiff against the first patch:

--- b/toolkit/components/downloads/ApplicationReputation.cpp
+++ b/toolkit/components/downloads/ApplicationReputation.cpp
@@ -1004,9 +1004,8 @@
   rv = cryptoHash->Init(nsICryptoHash::SHA256);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  const char* rawSpecBytes = PromiseFlatCString(aSpec).get();
-  rv = cryptoHash->Update(reinterpret_cast<const uint8_t*>(rawSpecBytes),
-                           strlen(rawSpecBytes));
+  rv = cryptoHash->Update(reinterpret_cast<const uint8_t*>(aSpec.BeginReading()),
+                          aSpec.Length());
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsAutoCString binaryHash;
@@ -1018,7 +1017,7 @@
   static const char* const hex = "0123456789ABCDEF";
   hexEncodedHash.SetCapacity(2 * binaryHash.Length());
   for (size_t i = 0; i < binaryHash.Length(); ++i) {
-    auto c = static_cast<const char>(binaryHash[i]);
+    auto c = static_cast<const unsigned char>(binaryHash[i]);
     hexEncodedHash.Append(hex[(c >> 4) & 0x0F]);
     hexEncodedHash.Append(hex[c & 0x0F]);
   }
Attachment #8884467 - Attachment is obsolete: true
Attachment #8885435 - Flags: sec-approval?
Attachment #8885435 - Flags: review+
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Pretty easy since the patch shows that we're not looking at downloads encoded in data: or blob: URIs.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

The patch itself kind of does.

> Which older supported branches are affected by this flaw?

That problem has been around since we introduced the download protection feature.

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

No but I can provide them if necessary. They should be essentially the same since this code hasn't changed in a while.

> How likely is this patch to cause regressions; how much testing does it need?

It's not very likely to break anything. I tested it against the attached test page and I ran through all of the download tests on http://testsafebrowsing.appspot.com/.
Comment on attachment 8885435 [details] [diff] [review]
Revised patch against mozilla-central

You don't need sec-approval for sec-moderate or sec-low bugs.
Attachment #8885435 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71aa5b9fdb38

need also beta uplift request
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(In reply to Carsten Book [:Tomcat] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/71aa5b9fdb38
> 
> need also beta uplift request

In comment 11, Dan said we don't need to uplift to Beta.
Group: toolkit-core-security → core-security-release
[Tracking Requested - why for this release]: Per comment 11, this is something we'll probably want for 52.4.
Target Milestone: --- → mozilla56
Requesting uplift into ESR 52.4 as per comment 11.

[Approval Request Comment]
User impact if declined: Malware can evade download protection by using blob: and data: URIs
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): In the worst case, download protection could break entirely. Can be manually tested using https://testsafebrowsing.appspot.com/ and https://mozilla.github.io/safebrowsing-test/bug1376036.html
String or UUID changes made by this patch: none
Attachment #8888095 - Flags: approval-mozilla-esr52?
Dan, do we need to remove the security flag before this can been seen by release management and approved for 52?

Also, it has landed on central, it probably doesn't need to be hidden anymore.
Flags: needinfo?(dveditz)
We don't un-hide bugs until we're shipping the fix across all supported branches. And no, RelMan can see sec bugs, they typically just tend to wait to do ESR approvals until a couple weeks into the cycle to avoid complications with any possible dot releases.
Flags: needinfo?(dveditz)
Comment on attachment 8888095 [details] [diff] [review]
Rebased patch against est52

safe browsing fix for esr52.4
Attachment #8888095 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Alias: CVE-2017-7814
Whiteboard: [adv-main56+][adv-esr52.4+]
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.