Closed Bug 1179301 Opened 9 years ago Closed 8 years ago

Latent buffer overrun bug in SafebrowsingHash

Categories

(Toolkit :: Safe Browsing, defect, P2)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: q1, Assigned: hchang)

References

Details

(Keywords: sec-audit, Whiteboard: #sbv4-m1 [adv-main51-])

Attachments

(1 file)

Bug 1179301 - Restrict the SafeBrowsingHash size and make toUint32/fromUint32 more robust.
58 bytes, text/x-review-board-request
gcp
: review+
Details 
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.
"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)
(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: safebrowsingv4
Priority: -- → P2
Assignee: nobody → hchang
Whiteboard: #sbv4-m1
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?
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.)
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 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+
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 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`?
(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 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.
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!
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
https://hg.mozilla.org/mozilla-central/rev/83e41bb049d8
Status: NEW → RESOLVED
Closed: 8 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.

Attachment

General

Creator:
Created:
Updated:
Size: