Noise entries and negative cache should be restricted to their own provider

RESOLVED FIXED in Firefox 58

Status

()

Toolkit
Safe Browsing
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: francois, Assigned: tnguyen)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: #sbv4-m10)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8896047 [details]
Screenshot of about:url-classifier demonstrating the problem

It looks like we don't isolate Safe Browsing providers correctly when we:

1. add noise entries to a completion request
2. save the results of completion requests

Additionally, it looks like we still save entries for lists that aren't enabled.

Steps:

1. Add the Yandex malware provider and enable both goog-malware-shavar and ydx-malware-shavar for in urlclassifier.malwareTable:

user_pref("browser.safebrowsing.provider.yandex.lists", "ydx-phish-shavar,ydx-malware-shavar");
user_pref("browser.safebrowsing.provider.yandex.gethashURL", "https://sba.yandex.net/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
user_pref("browser.safebrowsing.provider.yandex.reportURL", "https://sba.yandex.net/report?");
user_pref("browser.safebrowsing.provider.yandex.updateURL", "https://sba.yandex.net/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
user_pref("browser.safebrowsing.provider.yandex.pver", "2.2");
user_pref("urlclassifier.malwareTable", "goog-malware-proto,goog-unwanted-proto,test-malware-simple,test-unwanted-simple,goog-malware-shavar,ydx-malware-shavar");

2. Close Firefox
3. Start Firefox with MOZ_LOG="UrlClassifierDbService:5,nsChannelClassifier:5,UrlClassifierStreamUpdater:1"
4. Go into about:url-classifier and trigger an update of yandex and google
5. Open https://d26dzxoao6i3hh.cloudfront.net/ in a new tab
6. Refresh the completion cache and show entries for ydx-malware-shavar and goog-malware-shavar

Expected:

ydx-malware-shavar should contain 4 entries in its cache (1 real entry + 3 noise) and goog-malware-shavar should have none.

Actual:

ydx-malware-shavar and goog-malware-shavar have the same 4 entries, and goog-unwanted-shavar has one entry (even though it's now even enabled!)
Here's the (redacted) MOZ_LOG:

[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x7ff7ed014700]: Classifying principal https://d26dzxoao6i3hh.cloudfront.net/ on channel with uri https://d26dzxoao6i3hh.cloudfront.net/
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x7ff7ed014700]: suspended channel 0x7ff7ebe82848
[URL Classifier]: D/UrlClassifierDbService Checking fragment d26dzxoao6i3hh.cloudfront.net/
[URL Classifier]: D/UrlClassifierDbService Checking fragment cloudfront.net/
[URL Classifier]: D/UrlClassifierDbService Checking table goog-phish-proto
[URL Classifier]: D/UrlClassifierDbService Checking table test-phish-simple
[URL Classifier]: D/UrlClassifierDbService Checking table ydx-phish-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table aqksb-phish-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table baidu-phish-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table goog-malware-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table goog-malware-proto
[URL Classifier]: D/UrlClassifierDbService Checking table goog-unwanted-proto
[URL Classifier]: D/UrlClassifierDbService Checking table test-malware-simple
[URL Classifier]: D/UrlClassifierDbService Checking table test-unwanted-simple
[URL Classifier]: D/UrlClassifierDbService Checking table baidu-malware-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table ydx-malware-shavar
[URL Classifier]: D/UrlClassifierDbService Checking table test-block-simple
[URL Classifier]: D/UrlClassifierDbService Checking table mozplugin-block-digest256
[URL Classifier]: D/UrlClassifierDbService Checking fragment d26dzxoao6i3hh.cloudfront.net/, hash 66BCA8E9056379C17D1F4516A4CF41A84460958F06FF1A7267ED1188CA04501C (E9A8BC66)
[URL Classifier]: D/UrlClassifierDbService Probe in V4 goog-phish-proto: E9A8BC66, found 0, complete 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-phish-simple: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in ydx-phish-shavar: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in aqksb-phish-shavar: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in baidu-phish-shavar: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in goog-malware-shavar: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in V4 goog-malware-proto: E9A8BC66, found 0, complete 0
[URL Classifier]: D/UrlClassifierDbService Probe in V4 goog-unwanted-proto: E9A8BC66, found 0, complete 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-malware-simple: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in test-unwanted-simple: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in baidu-malware-shavar: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in ydx-malware-shavar: E9A8BC66, has 1, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Found a result in ydx-malware-shavar: Not confirmed.
[URL Classifier]: D/UrlClassifierDbService Probe in test-block-simple: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Probe in mozplugin-block-digest256: E9A8BC66, has 0, confirmed 0
[URL Classifier]: D/UrlClassifierDbService Checking fragment cloudfront.net/, hash EEE32DCF38BC38F709128A8ECE92AAFB2137335D5CF881BEF2620B31406456C3 (CF2DE3EE)
...
[URL Classifier]: D/UrlClassifierDbService Found 1 results.
[URL Classifier]: D/UrlClassifierDbService Found 1 results.
[URL Classifier]: D/UrlClassifierDbService query took 0ms
[Main Thread]: D/UrlClassifierDbService The match from ydx-malware-shavar needs to be completed at https://sba.yandex.net/gethash?client=navclient-auto-ffox&appver=57.0a1&pver=2.2
[Main Thread]: D/UrlClassifierDbService The match from ydx-malware-shavar needs to be completed at https://sba.yandex.net/gethash?client=navclient-auto-ffox&appver=57.0a1&pver=2.2
[Main Thread]: D/UrlClassifierDbService The match from ydx-malware-shavar needs to be completed at https://sba.yandex.net/gethash?client=navclient-auto-ffox&appver=57.0a1&pver=2.2
[Main Thread]: D/UrlClassifierDbService The match from ydx-malware-shavar needs to be completed at https://sba.yandex.net/gethash?client=navclient-auto-ffox&appver=57.0a1&pver=2.2
[Main Thread]: D/UrlClassifierDbService The match from ydx-malware-shavar needs to be completed at https://sba.yandex.net/gethash?client=navclient-auto-ffox&appver=57.0a1&pver=2.2
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::LookupComplete [0x7ff7f64700c0] 5 pending completions
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0IBxAGGAEiAzAwMTABEPfQARoCGAWwPT9T' and checksum '1CIdjsE+nWoA+mF9WOfHgdg+UpIihXxbr3qg6CtIUiw=' for table goog-badbinurl-proto
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0IARAGGAEiAzAwMTABELzjAhoCGAW8212+' and checksum '9d4h9karw/luQRlfD638WkJjzM8QangvXoDfs6+as0w=' for table goog-malware-proto
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0IAxAGGAEiAzAwMTABEP7RAhoCGAUQYhyj' and checksum 'zxy3Ea9uEtJkYxA6w74dIUQUVQFQmTAyi5NFjyQealc=' for table goog-unwanted-proto
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0IBRAGGAEiAzAwMTABEODgAhoCGAWlSPLf' and checksum 'r6173iH2UIbdPsuJbC8xq0COJwt6TG3PMlDXARevkBA=' for table goog-phish-proto
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0ICRAGGAEiAzAwMTABEAgaAhgIpJieQg==' and checksum 'ssjnBUHFZiaDAPw2PtrftAUtriC9dUl7rRWto0J4iiQ=' for table goog-downloadwhite-proto
[URL Classifier]: D/UrlClassifierDbService Appending state 'Cg0IAhAGGAEiAzAwMTABEKfcARoCGAgcXcDU' and checksum 'SPrVN3wTrEkLDU9MOBXAd7+GlU3+nsFTM0Elv0SvSN8=' for table googpub-phish-proto
...
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-downloadable-mobile-shavar, 1484]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-downloadable-mobile-android-shavar, 1484]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-shavar, 70438]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, goog-malware-shavar, 70438]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-downloadable-shavar, 2034]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::CompletionFinished [0x7ff7f64700c0, NS_OK]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-unwanted-shavar, 12640]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, goog-unwanted-shavar, 12640]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-unwanted-software-shavar, 2454]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-unwanted-software-distribution-shavar, 2219]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::CompletionFinished [0x7ff7f64700c0, NS_OK]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-shavar, 68116]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, goog-malware-shavar, 68116]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-blacklist-shavar, 1570]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::CompletionFinished [0x7ff7f64700c0, NS_OK]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-shavar, 67738]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, goog-malware-shavar, 67738]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-driveby-shavar, 3091]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::CompletionFinished [0x7ff7f64700c0, NS_OK]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-shavar, 70161]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, goog-malware-shavar, 70161]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::Completion [0x7ff7f64700c0, ydx-malware-driveby-shavar, 5400]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::CompletionFinished [0x7ff7f64700c0, NS_OK]
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierLookupCallback::HandleResults [0x7ff7f64700c0, 5 results]
[Main Thread]: D/UrlClassifierDbService Confirmed result 66BCA8E9 from table ydx-malware-shavar
[Main Thread]: D/UrlClassifierDbService nsUrlClassifierClassifyCallback::HandleResult [0x7ff7ed0149a0, table ydx-malware-shavar prefix Zryo6Q==]
[Main Thread]: D/UrlClassifierDbService Skipping result 5F91319D from table ydx-malware-shavar (noise)
Main Thread]: D/UrlClassifierDbService Skipping result D0F43875 from table ydx-malware-shavar (noise)
[Main Thread]: D/UrlClassifierDbService Skipping result 94DF9BD5 from table ydx-malware-shavar (noise)
[Main Thread]: D/UrlClassifierDbService Skipping result D57C5356 from table ydx-malware-shavar (noise)
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x7ff7ed014700]:OnClassifyComplete NS_ERROR_MALWARE_URI (suspended channel)
[Main Thread]: D/nsChannelClassifier nsChannelClassifier::MarkEntryClassified[NS_ERROR_MALWARE_URI] https://d26dzxoao6i3hh.cloudfront.net/
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x7ff7ed014700]: cancelling channel 0x7ff7ebe82848 for https://d26dzxoao6i3hh.cloudfront.net/ with error code NS_ERROR_MALWARE_URI
[URL Classifier]: D/UrlClassifierDbService nsUrlClassifierDBServiceWorker::CacheCompletions [0x7ff7f7411ab0]
[URL Classifier]: D/UrlClassifierDbService Completion received, but table is not active, so not caching.
[URL Classifier]: D/UrlClassifierDbService Completion received, but table is not active, so not caching.
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x7ff7ed014700]: resuming channel 0x7ff7ebe82848 from OnClassifyComplete
(Assignee)

Updated

5 months ago
Assignee: nobody → tnguyen
(Assignee)

Updated

5 months ago
Status: NEW → ASSIGNED
Whiteboard: #sbv4-m9 → #sbv4-m10
Comment hidden (typo)
(Assignee)

Comment 3

4 months ago
hashcompleter: 12:21:35 GMT+0800 (CST): Received a 200 status code from the gethash server (success=true).
hashcompleter: 12:21:35 GMT+0800 (CST): Response body ydx-malware-shavar:70473:32
úé¿¡¡%|`hÔ!n.d‚‰öÝUü$b0AFngoog-malware-shavar:70473:32
úé¿¡¡%|`hÔ!n.d‚‰öÝUü$b0AFnydx-malware-driveby-shavar:5705:32
úé¿¡¡%|`hÔ!n.d‚‰öÝUü$b0AFnydx-malware-shavar:67915:32
všÿx¨œœ²ñÜ3€.¯iÚnÏ
_;%^’áÇgoog-malware-shavar:67915:32
všÿx¨œœ²ñÜ3€.¯iÚnÏ
_;%^’áÇydx-malware-driveby-shavar:3258:32
všÿx¨œœ²ñÜ3€.¯iÚnÏ
_;%^’áÇydx-malware-downloadable-mobile-shavar:1484:32
f¼¨écyÁ}E¤ÏA¨D`•ÿrgíˆÊPydx-malware-downloadable-mobile-android-shavar:1484:32
f¼¨écyÁ}E¤ÏA¨D`•ÿrgíˆÊPydx-malware-shavar:70438:32
f¼¨écyÁ}E¤ÏA¨D`•ÿrgíˆÊPgoog-malware-shavar:70438:32
f¼¨écyÁ}E¤ÏA¨D`•ÿrgíˆÊPydx-malware-downloadable-shavar:2034:32
f¼¨écyÁ}E¤ÏA¨D`•ÿrgíˆÊPydx-malware-shavar:67818:32
¼Zoþœåƒ%UžN,zô¦úÅ-ή&´qGÏgoog-malware-shavar:67818:32
¼Zoþœåƒ%UžN,zô¦úÅ-ή&´qGÏydx-malware-driveby-shavar:3166:32
¼Zoþœåƒ%UžN,zô¦úÅ-ή&´qGÏydx-malware-shavar:68347:32
úÛùèi½PÍÖ²®¼]"õÕLñzPäγwFë\$Wgoog-malware-shavar:68347:32
úÛùèi½PÍÖ²®¼]"õÕLñzPäγwFë\$Wydx-malware-driveby-shavar:3666:32
úÛùèi½PÍÖ²®¼]"õÕLñzPäγwFë\$W

The response looks weird, is that test site or real unsafe site?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8908002 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8908003 - Attachment is obsolete: true
Attachment #8908003 - Flags: review?(francois)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

4 months ago
(In reply to François Marier [:francois] from comment #0)
> Created attachment 8896047 [details]
> Screenshot of about:url-classifier demonstrating the problem
> 
> It looks like we don't isolate Safe Browsing providers correctly when we:
> 
> 1. add noise entries to a completion request
> 2. save the results of completion requests
> 

1 - We generate the noises from prefix set of the same table, so it's well isolated.
2 - As I can see, yandex server responded a completion list which is not well-formatted. This contains both google and yandex listnames. Anyway, we should fix it because we don't want a response from a provider could pollute other caches.
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3)
> hashcompleter: 12:21:35 GMT+0800 (CST): Received a 200 status code from the
> gethash server (success=true).

It would be really good to change that debug message to include the provider name "Received a 200 status code from the yandex gethash server..."

That way we can easily tell if a provider is attempting to inject data into another provider's cache.
Flags: needinfo?(francois)
(Reporter)

Comment 9

4 months ago
mozreview-review
Comment on attachment 8908010 [details]
Bug 1389315 - Isolate Safe Browsing completions cached by each provider.

https://reviewboard.mozilla.org/r/179696/#review185132

Great fix!

I think we can make the log messages even clearer so that next time these kinds of errors are easier to see.

::: commit-message-c15e2:1
(Diff revision 1)
> +Bug 1389315 - Isolate cached completion by provider

I would say "Isolate Safe Browsing completions cached by each provider."

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:677
(Diff revision 1)
>    // This adds a complete hash to any entry in |this._requests| that matches
>    // the hash.
>    handleItem: function HCR_handleItem(aData) {
> +    let provider = gUrlUtil.getProvider(aData.tableName);
> +    if (provider != this.provider) {
> +      log("Response contains table : " + aData.tableName +

I would suggest a few more details in this log message:

    "Ignoring table " + aData.tableName + " since it belongs to " + provider + " while the response came from " + this.provider + "."

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:777
(Diff revision 1)
>        if (!success) {
>          aStatusCode = Cr.NS_ERROR_ABORT;
>        }
>      }
>      let success = Components.isSuccessCode(aStatusCode);
>      log("Received a " + httpStatus + " status code from the gethash server (success=" + success + ").");

It would be really handy if that log message included the provider name:

    "Received a " + httpStatus + " status code from the " + provider + " gethash server (success=" + success + ")."
Attachment #8908010 - Flags: review?(francois) → review-
(Assignee)

Comment 10

4 months ago
mozreview-review-reply
Comment on attachment 8908010 [details]
Bug 1389315 - Isolate Safe Browsing completions cached by each provider.

https://reviewboard.mozilla.org/r/179696/#review185132

Thanks Francois
Comment hidden (mozreview-request)
(Reporter)

Comment 12

4 months ago
mozreview-review
Comment on attachment 8908010 [details]
Bug 1389315 - Isolate Safe Browsing completions cached by each provider.

https://reviewboard.mozilla.org/r/179696/#review185550
Attachment #8908010 - Flags: review?(francois) → review+
Before landing, make sure you test it manually in order to ensure we didn't introduce any syntax errors in the JS code.

Not exactly sure how to test it, but I guess you'd need to find a way to trigger a completion. Maybe visiting testsafebrowsing.appspot.com with a stale local DB.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

4 months ago
(In reply to François Marier [:francois] from comment #13)
> Before landing, make sure you test it manually in order to ensure we didn't
> introduce any syntax errors in the JS code.
> 
> Not exactly sure how to test it, but I guess you'd need to find a way to
> trigger a completion. Maybe visiting testsafebrowsing.appspot.com with a
> stale local DB.

Sure, it works, we don't need a stale DB. Every time starting FF, we have a fresh cache :), then we need to send fullhash request.
I see there're try failures due to "test" provider. I added the condition check this.provider != "test" to the patch, Francois, do you agree with that?
Flags: needinfo?(francois)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #16)
> Sure, it works, we don't need a stale DB. Every time starting FF, we have a
> fresh cache :), then we need to send fullhash request.
> I see there're try failures due to "test" provider. I added the condition
> check this.provider != "test" to the patch, Francois, do you agree with that?

So we have test providers that return entries for different lists?

Ideally, it would be better to fix the tests so that we don't have to add a special case like this.
Flags: needinfo?(francois)
(Assignee)

Comment 18

4 months ago
(In reply to François Marier [:francois] from comment #17)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #16)
> > Sure, it works, we don't need a stale DB. Every time starting FF, we have a
> > fresh cache :), then we need to send fullhash request.
> > I see there're try failures due to "test" provider. I added the condition
> > check this.provider != "test" to the patch, Francois, do you agree with that?
> 
> So we have test providers that return entries for different lists?
> 
> Ideally, it would be better to fix the tests so that we don't have to add a
> special case like this.

It looks like "" (empty) provider is different than "test" provider. Let me check and fix it
Comment hidden (mozreview-request)

Comment 21

4 months ago
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56e8182e239d
Isolate Safe Browsing completions cached by each provider. r=francois

Comment 22

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/56e8182e239d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.