1363882 - Gethash entries are not cast safely

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

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?

Comment 1

[Thomas Nguyen (:tnguyen)]

(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

Comment 2

[Thomas Nguyen (:tnguyen)]

(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.

Comment 3

[Thomas Nguyen (:tnguyen)]

Well, I see most of the compilers accept non-standard accessing inactive member of union. But it would be safe to fix it.

Comment 4

[Thomas Nguyen (:tnguyen)]

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 5

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1363882 - Remove casting address of inactive member union result.hash

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

Comment 6

[Thomas Nguyen (:tnguyen)]

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.

Comment 7

[François Marier [:francois]]

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

https://reviewboard.mozilla.org/r/154446/#review159734

Comment 8

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=503b9ff324ce6c9a4907aa3cb1e7792556cf2dac
looks good, except the one which is already filed in bug 1374268

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93e96d634ed7
Remove casting address of inactive member union result.hash r=francois

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93e96d634ed7