bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Latent buffer overrun bug in SafebrowsingHash

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: q1, Assigned: hchang)

Tracking

({sec-audit})

unspecified
mozilla51
sec-audit
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: #sbv4-m1 [adv-main51-])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
SafebrowsingHash::ToUint32 and SafebrowsingHash::FromUint32 (in current comm-central/source/mozilla/toolkit/components/url-classifier/Entries.h) have a latent buffer-overrun bug. Both functions assume that the hash buffer bug is at least 4 bytes long, but nothing prevents code from instantiating SafebrowsingHash with a smaller buffer, in which case these functions will read/write unowned data.

At present, it appears that SafebrowsingHash is instantiated only with buffer sizes 4 and 32, so the bug is latent.
(Reporter)

Comment 1

3 years ago
"hash buffer bug" should read "hash buffer".
Component: Untriaged → Safe Browsing
Product: Firefox → Toolkit
Group: core-security
Keywords: sec-audit
Flags: sec-bounty?
>but nothing prevents code from instantiating SafebrowsingHash with a smaller buffer

Just checking I understand correctly: only our own code could do this, right? SafeBrowsingHash is, per protocol definition, always 32 bits (4 byte) or 256 bits (32 byte). You are saying that if we would somehow make code changes to add new formats/hashes of less than 32 bits AND not update the relevant code, we'd have a buffer overrun.

I guess more defensive coding could add a guard against the template being instantiated with S < 4 but it feels like a non-issue to me because I'd be surprised if much of the surrounding logic still worked if the instantiations aren't specifically 4 or 32 bytes.

That said, the code as is has another issue, which, while clumsy, is luckily not a security bug:

  uint8_t buf[S];

  uint32_t ToUint32() const {
      return *((uint32_t*)buf);
  }

  void FromUint32(uint32_t aHash) {
      *((uint32_t*)buf) = aHash;
  }

There's no guarantee that "buf" is aligned to 32-bits here, which is needed to make the type-punning cast not undefined behavior. (In practice it works on every architecture we support)
(Reporter)

Comment 3

3 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> >but nothing prevents code from instantiating SafebrowsingHash with a smaller buffer
> 
> Just checking I understand correctly: only our own code could do this,
> right? 

Well, any code that ends up in a Mozilla image that invokes SafebrowsingHash with a buffer size < 4.

> There's no guarantee that "buf" is aligned to 32-bits here, which is needed
> to make the type-punning cast not undefined behavior. (In practice it works
> on every architecture we support)

Hmm, I didn't know that natural alignment was a spec requirement. Gotta look it up ;)
Flags: sec-bounty? → sec-bounty-
Let's add the appropriate guard before we extend this for V4 (where hashes can be any size from 4-32 bytes).
Blocks: 1167038
Priority: -- → P2

Updated

2 years ago
Assignee: nobody → hchang
Whiteboard: #sbv4-m1
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70142

::: toolkit/components/url-classifier/Entries.h:33
(Diff revision 1)
>  
>  // This is the struct that contains 4-byte hash prefixes.
>  template <uint32_t S, class Comparator>
>  struct SafebrowsingHash
>  {
> +  static_assert(S >= 4, "The SafebrowsingHash should be at least 4 bytes.");

Wouldn't we rather fix the underlying issue (including the current Undefined Behavior) with memcpy or something like that?
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70142

> Wouldn't we rather fix the underlying issue (including the current Undefined Behavior) with memcpy or something like that?

Did you mean the buffer alignment issue? (as mentioned in comment 2)
For S >= 4, there should be no issue at all and I followed the comment 4 
to add the restriction in this bug as a solution.
Aside from alignment, the endianness may be another issue here.

Anyways, I am glad to fix the alignment and endianness issue in this bug.

(FYI, I am told by Dimi that the variable-length prefix implementation for V4 will not use this structure.)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70142

> Did you mean the buffer alignment issue? (as mentioned in comment 2)
> For S >= 4, there should be no issue at all and I followed the comment 4 
> to add the restriction in this bug as a solution.
> Aside from alignment, the endianness may be another issue here.
> 
> Anyways, I am glad to fix the alignment and endianness issue in this bug.
> 
> (FYI, I am told by Dimi that the variable-length prefix implementation for V4 will not use this structure.)

And for S < 4, I don't think we need to be worried about the undefined behavior. Just disallow it is good enough :)
(In reply to Henry Chang [:henry][:hchang] from comment #7)

> Did you mean the buffer alignment issue? (as mentioned in comment 2)

Yes.

> For S >= 4, there should be no issue at all 

I think there is. There is no guarantee uint8_t buf[S] is anything but byte aligned, and doing the type punning cast on that to integer will produce a non-aligned integer pointer, which is UD. Think about what could happen on ARM, for example. There might be strict aliasing problems as well.

A memcpy avoids all the issues here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70174
Attachment #8782300 - Flags: review?(gpascutto) → review+
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70174

Thanks for the review and the alignment lesson :)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review70430

::: toolkit/components/url-classifier/Entries.h:34
(Diff revision 3)
>  
>  // This is the struct that contains 4-byte hash prefixes.
>  template <uint32_t S, class Comparator>
>  struct SafebrowsingHash
>  {
> +  static_assert(S >= 4, "The SafebrowsingHash should be at least 4 bytes.");

Should we also check that `S < 32`?
(Assignee)

Comment 15

2 years ago
(In reply to François Marier [:francois] from comment #14)
> Comment on attachment 8782300 [details]
> Bug 1179301 - Restrict the SafeBrowsingHash size and make
> toUint32/fromUint32 more robust.
> 
> https://reviewboard.mozilla.org/r/72506/#review70430
> 
> ::: toolkit/components/url-classifier/Entries.h:34
> (Diff revision 3)
> >  
> >  // This is the struct that contains 4-byte hash prefixes.
> >  template <uint32_t S, class Comparator>
> >  struct SafebrowsingHash
> >  {
> > +  static_assert(S >= 4, "The SafebrowsingHash should be at least 4 bytes.");
> 
> Should we also check that `S < 32`?

From the implementation point of view, SafebrowsingHash could work correctly with any S >= 4 after fixing the ToUint32/FromUint32 issue.

BTW, there's no NativeEndian::read/writeUint32 :( I have to change to memcpy eventually...
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review71654

::: toolkit/components/url-classifier/tests/gtest/TestSafebrowsingHash.cpp:12
(Diff revision 4)
> +
> +  // typedef SafebrowsingHash<PREFIX_SIZE, PrefixComparator> Prefix;
> +  // typedef nsTArray<Prefix> PrefixArray;
> +
> +  const char PREFIX_RAW[4] = { 0x1, 0x2, 0x3, 0x4 };
> +  const uint32_t PREFIX_UINT32 = *((uint32_t*)PREFIX_RAW);

Undefined behavior for reasons already mentioned. If you don't want to memcpy here because that's what you're trying to test, then a union can also work around this.
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8782300 [details]
Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.

https://reviewboard.mozilla.org/r/72506/#review71654

> Undefined behavior for reasons already mentioned. If you don't want to memcpy here because that's what you're trying to test, then a union can also work around this.

My intention of making this test case is to ensure the modified behavior is the same as the original code (since it has been working) so I still use type punning to generate the expected answer. But yes it doesn't make sense to compare with a undefined behavior no matter how it's working perfectly is the past. I'll just change to memcpy :) Thanks!
Comment hidden (mozreview-request)

Comment 20

2 years ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83e41bb049d8
Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust. r=gcp

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83e41bb049d8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Whiteboard: #sbv4-m1 → #sbv4-m1 [adv-main51-]
You need to log in before you can comment on or make changes to this bug.