Closed Bug 1363879 Opened 3 years ago Closed 3 years ago

Sort gethash prefixes to hide noise entries

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: francois, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m7)

Attachments

(1 file, 1 obsolete file)

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.
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?
Flags: needinfo?(gpascutto)
I probably didn't think of sorting. Shuffling may well be faster here, though it's a tough call due to JS being involved.
Flags: needinfo?(gpascutto)
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.
Summary: Randomize the order of gethash prefixes to hide noise entries → Sort gethash prefixes to hide noise entries
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
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
Attachment #8868442 - Attachment is obsolete: true
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"
Attachment #8868446 - Flags: review?(francois) → review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ff7323da7ec
Sort gethash prefixes to hide noise entries r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ff7323da7ec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab715ee9ef99
Add Advanced Layers to the reftest sandbox. (bug 1363879 part 24, r=mattwoodrow)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/d08fac2a15fb
Backed out changeset ab715ee9ef99
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 :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
Fixed for real by commit in comment 11.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Version: unspecified → 55 Branch
You need to log in before you can comment on or make changes to this bug.