1376036 - (CVE-2017-7814) Application Reputation checks don't cover blob: and data: URLs

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

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?

Comment 1

[François Marier [:francois]]

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]

Comment 2

[François Marier [:francois]]

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

Comment 3

[François Marier [:francois]]

Chrome is treating blob and data URIs as supported and potentially containing binary files:

https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_protection_service.cc?l=620&rcl=4d835e218b1fbc5e669af5a716dcf82e2dd61f73

and they shorten data URIs before including them as part of the remote ping:

https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_protection_service.cc?l=958&rcl=4d835e218b1fbc5e669af5a716dcf82e2dd61f73

Comment 4

[François Marier [:francois]]

Attachment: add-extra-logging.patch

Comment 5

[François Marier [:francois]]

Attachment: Test page for blob: and data: URIs

Here's a simpler test case that shows that both blob: and data: URIs are affected.

Comment 6

[François Marier [:francois]]

For what it's worth, ftp URLs are not affected. Downloads hosted on that scheme go through the necessary checks.

Comment 7

[François Marier [:francois]]

Attachment: Patch against mozilla-central

Comment 8

[François Marier [:francois]]

Once we have a Marionette test for download protection (bug 1237766), we should add these two test cases.

Comment 9

[François Marier [:francois]]

Dan, do you think that's something we should fix on release and ESR?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 11

[Daniel Veditz [:dveditz]]

(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?

Comment 12

[François Marier [:francois]]

(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.

Comment 13

[François Marier [:francois]]

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.

Comment 14

[François Marier [:francois]]

(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.

Comment 15

[François Marier [:francois]]

Attachment: Revised patch against mozilla-central

[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]);
   }

Comment 16

[François Marier [:francois]]

> [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 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71aa5b9fdb3862263b1bad71279b5c6552d7806c

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71aa5b9fdb38

need also beta uplift request

Comment 20

[François Marier [:francois]]

(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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Per comment 11, this is something we'll probably want for 52.4.

Comment 22

[François Marier [:francois]]

Attachment: Rebased patch against est52

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

Comment 23

[François Marier [:francois]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8888095 [details] [diff] [review]
Rebased patch against est52

safe browsing fix for esr52.4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/68a444daf85b