807847 - Noise entries are erroneously entered into the misscache

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

When we receive completion results for noise entries, we have a bug in our logic that causes them to be entered in the miss-cache. This could cause us to miss a malware/phishing warning if the user is unlucky enough to hit the site that corresponds to the noise entries before the next update arrives.

This bug is made worse by a second bug: the original entry is added again when adding noise entries. Not only does this allow a trivial defeat of the noise (the entry that you get twice is the correct one), if the user forces a reload or revisits a phishing page, it's possible he will not get any warning because the entry is in the misscache.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

To reproduce, you must hit a new phishing site (that you haven't seen before), and then force the reload before any SafeBrowsing update hits. I'm able to reproduce on Nightly. The phishing site must also be one that is only in the database a single time for the URL that you are visiting, i.e. if www.foo.com and foo.com are both in the database, you won't get this problem.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Actually, it's reproducible on Nightly+bug 782106. In retrospect, it's obvious why: without bug 782106 fixed, the noise entries are encoded with the per-client key and don't correspond to anything. This also applies to the original entry, which is sent in the encoded version. Even if those replies are miss-cached, the wont correspond to any real entry and the bug isn't visible.

So this bug has been there for a while but fixing bug 782106 will expose it.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix noise entries in misscache. Fix duplicated noise.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=caa492f7913b

https://hg.mozilla.org/mozilla-central/rev/30bdcaf34723

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 677742 [details] [diff] [review]
Patch 1. Fix noise entries in misscache. Fix duplicated noise.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782106
User impact if declined: Users hitting the same malware site twice will not see a warning.
Testing completed (on m-c, etc.): Landed on m-c last friday.
Risk to taking this patch (and alternatives if risky): Very low. Disables some caching.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e071059b71c
https://hg.mozilla.org/releases/mozilla-aurora/rev/da1a7707fd86

Landed this already, based on email from akeybl where we discussed that this has to go together with bug 782106 and that he was going to approve it.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 677742 [details] [diff] [review]
Patch 1. Fix noise entries in misscache. Fix duplicated noise.

Approving (even though it's landed) just for bug data cleanliness and transparent process.

Comment 8

[u279076]

:gcp, can you please advise QA on how we might do some regression testing to ensure this is fixed?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Visit a phishing page. (Go to http://www.phishtank.com/ and try some URLs, not all of them will hit)

Examples at the current time:
mos.motowell.hu/images/1/index.html
accountinfo-paypal.de/login_scre.html

Hit refresh a few times. You should always hit the warning, and never go through to the site automatically. Close the tab, visit some other sites, enter the URL again. You should still see the warning.

Comment 10

[Ioana (away)]

Everything works as described in comment 9 on Firefox 17 beta 5 (20121106195758 - post-fix), but it also works like that on beta 2 (20121017073013 - pre-fix).

What did should go wrong specifically on pre-fix builds? I followed the details in all the above comments when checking on beta 2, but I do have one point I am not too sure about: when do SafeBrowsing updates hit?

Comment 11

[Gian-Carlo Pascutto [:gcp]]

>What did should go wrong specifically on pre-fix builds?

Exposing this bug in a reasonable manner needs bug 782106 (see comment 2). This fix was landed together with that bug, so there shouldn't have been any build where it would have been observable.

If this fix had been ineffective, you would have seen the explained behavior on the latest beta builds (because bug 782106 is fixed in them).

>I do have one point I am not too sure about: when do SafeBrowsing updates hit?

A first update will hit in a random time 5 minutes after starting the browser. Subsequent updates are usually 30-45 minutes apart. An update contains part of the database.

You can monitor the <userdata>/Local/Mozilla/Profiles/xxx.xxxx/safebrowsing folder and check the timestamps.

Comment 12

[Ioana (away)]

Considering comment 11, verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121106195758

The warning was displayed at every phishing page open/refresh.

Comment 13

[Ioana (away)]

Verified as fixed on Firefox 18 beta 1:
Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/18.0 Firefox/18.0
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/18.0 Firefox/18.0

Comment 14

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0
BuildID: 20130109111322

Comment 15

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old