Gethash entries are not cast safely

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: francois, Assigned: tnguyen)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: #sbv4-m8)

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
When we collect partial hashes to send to the hash completer, we cast the prefix into a string:

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1120

specifically accessing the hash.fixedLengthPrefix union member:

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/LookupCache.h#37

That works fine for noise entries since they are created using hash.fixedLengthPrefix:

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#347

but it seems like it could be relying on an undefined behavior for the real lookup matches which are created using hash.complete:

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/Classifier.cpp#515

Maybe we should instead use LookupResult.PartialHash():

https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/LookupCache.h#46-49

and change it so that it looks at mPartialHashLength to determine whether to cast hash.complete or hash.fixedLengthPrefix?
(Assignee)

Comment 1

a year ago
(In reply to François Marier [:francois] from comment #0)
> When we collect partial hashes to send to the hash completer, we cast the
> prefix into a string:
> 
> https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/
> nsUrlClassifierDBService.cpp#1120
> 
> specifically accessing the hash.fixedLengthPrefix union member:
> 
> https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/
> LookupCache.h#37
> 
> That works fine for noise entries since they are created using
> hash.fixedLengthPrefix:
> 
> https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/
> nsUrlClassifierDBService.cpp#347
> 
> but it seems like it could be relying on an undefined behavior for the real
> lookup matches which are created using hash.complete:
> 
> https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/
> Classifier.cpp#515
> 
> Maybe we should instead use LookupResult.PartialHash():
> 
> https://searchfox.org/mozilla-central/rev/
> 7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/toolkit/components/url-classifier/
> LookupCache.h#46-49
> 
> and change it so that it looks at mPartialHashLength to determine whether to
> cast hash.complete or hash.fixedLengthPrefix?

I guess you are mentioning about the case we access hash.complete when only assigning fixedLengthPrefix
(Assignee)

Comment 2

a year ago
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #1)
> 
> I guess you are mentioning about the case we access hash.complete when only
> assigning fixedLengthPrefix
I don't see we have problem to access fixedLengthPrefix if we assign complete. Because I look at the struct SafebrowsingHash
https://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/toolkit/components/url-classifier/Entries.h#30
and it only take buf[s] as memory allocation. Then it will ok if we assign complete then access to fixedLengthPrefix (but it will be not ok oppositely), or am I missing anything?
But you are right, we should rewrite to reinterpret_cast to the fixedLengthPrefix.buf (not fixedLengthPrefix) and use PartialHash, it would be safer and easy to look and debug.
(Assignee)

Updated

a year ago
Assignee: nobody → tnguyen
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Comment hidden (obsolete)
(Assignee)

Comment 4

a year ago
Well, I think, this kind of accessing address of inactive member would be allowed and defined under specific (most?) compilers and hardware (I would guess there're some castings underlie). But it would be safe to fix it.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
In the patch, I only fixed the cast that touched to address of hash.fixedLengthPrefix (in the case we write to hash.complete).
I could not find the standard that explicitly says C++ supports type punning (and I assume yes). Otherwise, we may make a copy when accessing to inactive member.
Somewhere in our code for example
https://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1298
https://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1344
It's quite small and I think it may not have any performance impact. In fact, I am not quite sure whether it's really necessary to do that.
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8883521 [details]
Bug 1363882 - Remove casting address of inactive member union result.hash

https://reviewboard.mozilla.org/r/154446/#review159734
Attachment #8883521 - Flags: review?(francois) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93e96d634ed7
Remove casting address of inactive member union result.hash r=francois
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93e96d634ed7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.