1363879 - Sort gethash prefixes to hide noise entries

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

The hash completer ultimately adds the partial hashes it receives to an array:

https://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#304

which will ultimately (for V4) be put into a Set for de-duplication purposes:

https://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#472-475

but into a randomized array (for V2):

https://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#502-510

to make sure the noise entry is not always in the same place.

We should shuffle the V4 array in the same way as the V2 one. Ideally refactoring as much of that code into a common function.

Comment 1

[François Marier [:francois]]

GCP, do you remember why you decided to shuffle the gethash array in bug 807852 instead of simply sorting it?

If the array is sorted, the hash entries will be arbitrary mixed in (in a deterministic way) with the real entry and so it won't be possible to know which is which, no?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I probably didn't think of sorting. Shuffling may well be faster here, though it's a tough call due to JS being involved.

Comment 3

[François Marier [:francois]]

Since this is almost always a list of exactly 5 entries (1 real, 4 noise), the perf differences should be negligible.

I think we can simplify this code and simply use JavaScript's built-in in-place sort.

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: Sort gethash prefixes to hide noise entries

In V2 we shuffled the hash entries before sending the request to obscure the real
entry from noises. We should do the same to V4, sort() is enough for both because
the array almost contains 5 entries

MozReview-Commit-ID: 4uOXIF83KQL

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1363879 - Sort gethash prefixes to hide noise entries

In V2 we shuffled the hash entries before sending the request to obscure the real
entry from noises. We should also hide the real entry in V4. Using sort() is
enough for both V2 and V4 because the array contains 5 entries in almost cases

Review commit: https://reviewboard.mozilla.org/r/140040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140040/

Comment 6

[François Marier [:francois]]

Comment on attachment 8868446 [details]
Bug 1363879 - Sort gethash prefixes to hide noise entries

https://reviewboard.mozilla.org/r/140040/#review143684

::: commit-message-e66de:5
(Diff revision 1)
> +Bug 1363879 - Sort gethash prefixes to hide noise entries
> +
> +In V2 we shuffled the hash entries before sending the request to obscure the real
> +entry from noises. We should also hide the real entry in V4. Using sort() is
> +enough for both V2 and V4 because the array contains 5 entries in almost cases

"contains exactly 5 entries in almost all cases"

Comment 7

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8868446 [details]
Bug 1363879 - Sort gethash prefixes to hide noise entries

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140040/diff/1-2/

Comment 8

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8868446 [details]
Bug 1363879 - Sort gethash prefixes to hide noise entries

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140040/diff/2-3/

Comment 9

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5111ea4131bca397b19b7c26165d0541cfb969

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ff7323da7ec
Sort gethash prefixes to hide noise entries r=francois

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff7323da7ec

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab715ee9ef99
Add Advanced Layers to the reftest sandbox. (bug 1363879 part 24, r=mattwoodrow)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab715ee9ef99

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d08fac2a15fb
Backed out changeset ab715ee9ef99

Comment 15

[François Marier [:francois]]

The commit from comment 12, which just got backed out, had nothing to do with this bug. It had a typo in the bug number.

David, make sure you fix the typo in the commit message before you re-land this :)

Comment 16

[François Marier [:francois]]

Fixed for real by commit in comment 11.