1021612 - AddressSanitizer: heap-buffer-overflow [@ mozilla::net::CacheFileMetadata::OnDataRead]

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack with symbols

I tested Steven's asan build ( http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg ) from Jun 5 and when I was reloading 250 tabs, the asan build crashed with a heap buffer overflow at mozilla::net::CacheFileMetadata::OnDataRead.

Setting s-s and sec-critical until we know what is going on.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This is a startup crash, right?

It's probably not related to the bug 997908 crashes, but it's certainly good to know about.

I will keep updating my Mac ASan build (roughly) once a week.  I suspect it will help find a lot of bugs.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Steven Michaud from comment #1)
> This is a startup crash, right?

Not really. I started things up, then I pressed "Reload All Tabs" when I right-clicked a tab. The ASan crash then happened almost immediately.

Comment 3

[Honza Bambas (:mayhemer)]

Michal, this looks pretty serious, can you pleas look at it?

Comment 4

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Michal, this looks pretty serious, can you pleas look at it?

Sure, I'll have a look at it. Is there any chance to get the line numbers in the stack?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Is there any chance to get the line numbers in the stack?

Probably not, but I'm not 100% sure.

Comment 6

[Honza Bambas (:mayhemer)]

Wish bug 966024 has already been done by now...  so little time :(

Comment 7

[Michal Novotny [:michal]]

Attachment: fix

Offset calculation is wrong when the entry file's size is exactly 4096 bytes. mBufSize is 0 so we access data outside the empty buffer in CacheFileMetadata::OnDataRead().

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 8436226 [details] [diff] [review]
fix

https://tbpl.mozilla.org/?tree=Try&rev=35376ed71fb1

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: test v1

So I've checked we really have mBufferSize == 0 when a cache file is exactly 4096k.  However, I cannot make it crash (debug/opt).  Do I actually need ASAN to reproduce at least the crash?

This test has one disadvantage - when we change either of hashing or the entry format, it will need to be updated.  So it's here more as just a prove of concept.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8436226 [details] [diff] [review]
fix

Review of attachment 8436226 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for a quick fix.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> Do I actually need ASAN to reproduce at least the crash?

Probably.

Comment 12

[Michal Novotny [:michal]]

Comment on attachment 8436310 [details] [diff] [review]
test v1

Review of attachment 8436310 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Honza Bambas (:mayhemer) from comment #9)
> So I've checked we really have mBufferSize == 0 when a cache file is exactly
> 4096k.  However, I cannot make it crash (debug/opt).  Do I actually need
> ASAN to reproduce at least the crash?

In fact, the problematic size is from 4096 to 4099 because mBufSize is 0-3 and we need at least 4 bytes in CacheFileMetadata::OnDataRead() to read the entry properly. With size 4096-4099 we access 4-1 bytes before mBuf. This is probably a valid memory, so the process won't crash but we get completely random realOffset value. Depending on the value we either fail to read the entry (since realOffset will be likely larger than file size) or we might even load the entry correctly if the value is lower than previous offset.


> This test has one disadvantage - when we change either of hashing or the
> entry format, it will need to be updated.  So it's here more as just a prove
> of concept.

I think this test is fine. We'll use probably the same concept to test loading of corrupted entries etc.

::: netwerk/test/unit/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head_channels.js head_cache.js head_cache2.js
>  tail =
>  support-files =
> +  data/4k-cache-file

This file is missing in the patch.

Comment 13

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa491e07b4b

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Erm, I'm not sure which branches are affected by this. In any case, if this isn't nightly-only, sec-approval should have been requested, fwiw.

Michal, which branches are affected, or is this nightly-only?

Comment 15

[Michal Novotny [:michal]]

This is nightly only problem, since the code is disabled in other branches.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fa491e07b4b

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #12)
> Comment on attachment 8436310 [details] [diff] [review]
> test v1
> 
> Review of attachment 8436310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > So I've checked we really have mBufferSize == 0 when a cache file is exactly
> > 4096k.  However, I cannot make it crash (debug/opt).  Do I actually need
> > ASAN to reproduce at least the crash?
> 
> In fact, the problematic size is from 4096 to 4099 because mBufSize is 0-3
> and we need at least 4 bytes in CacheFileMetadata::OnDataRead() to read the
> entry properly. With size 4096-4099 we access 4-1 bytes before mBuf. This is
> probably a valid memory, so the process won't crash but we get completely
> random realOffset value. Depending on the value we either fail to read the
> entry (since realOffset will be likely larger than file size) or we might
> even load the entry correctly if the value is lower than previous offset.

Is it worth to extend the test for this?

> 
> 
> > This test has one disadvantage - when we change either of hashing or the
> > entry format, it will need to be updated.  So it's here more as just a prove
> > of concept.
> 
> I think this test is fine. We'll use probably the same concept to test
> loading of corrupted entries etc.

Should I land it as is or should we invent something smarter first?

> 
> ::: netwerk/test/unit/xpcshell.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  head = head_channels.js head_cache.js head_cache2.js
> >  tail =
> >  support-files =
> > +  data/4k-cache-file
> 
> This file is missing in the patch.

No, it's there, look at the raw diff.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> I tested Steven's asan build (
> http://people.mozilla.org/~stmichaud/bmo/firefox-asan.dmg ) from Jun 5 and
> when I was reloading 250 tabs, the asan build crashed with a heap buffer
> overflow at mozilla::net::CacheFileMetadata::OnDataRead.

Note: This build came from bug 997908 comment 104.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8436310 [details] [diff] [review]
test v1

This should probably go in.

Comment 20

[Michal Novotny [:michal]]

Comment on attachment 8436310 [details] [diff] [review]
test v1

Review of attachment 8436310 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Honza Bambas (:mayhemer) from comment #17)
> > In fact, the problematic size is from 4096 to 4099 because mBufSize is 0-3
> > and we need at least 4 bytes in CacheFileMetadata::OnDataRead() to read the
> > entry properly. With size 4096-4099 we access 4-1 bytes before mBuf. This is
> > probably a valid memory, so the process won't crash but we get completely
> > random realOffset value. Depending on the value we either fail to read the
> > entry (since realOffset will be likely larger than file size) or we might
> > even load the entry correctly if the value is lower than previous offset.
> 
> Is it worth to extend the test for this?

What exactly do you mean? The other filesizes? It's definitely sufficient to test just 4096 bytes.


> > > This test has one disadvantage - when we change either of hashing or the
> > > entry format, it will need to be updated.  So it's here more as just a prove
> > > of concept.
> > 
> > I think this test is fine. We'll use probably the same concept to test
> > loading of corrupted entries etc.
> 
> Should I land it as is or should we invent something smarter first?

As I said, this test is fine.


> > ::: netwerk/test/unit/xpcshell.ini
> > This file is missing in the patch.
> 
> No, it's there, look at the raw diff.

Sorry, I overlooked it.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #20)
> What exactly do you mean? The other filesizes? It's definitely sufficient to
> test just 4096 bytes.

Yes, and as you mention, it's enough to leave as it.  Thanks!

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8436310 [details] [diff] [review]
test v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/b94c916122c8

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> Comment on attachment 8436310 [details] [diff] [review]
> test v1
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b94c916122c8

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/e185bf9449f6

since xpcshell tests still don't run each with its own profile!

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/b94c916122c8

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Gary, in your scenario, did you open that many tabs with one specific file? Or were they blank tabs? And did you get to that quantity of tabs by opening them one at a time? Or did you spawn them via command line? Or...?

I'd like to reproduce and/or verify. Thanks.

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Matt Wobensmith from comment #25)
> And did you get to that quantity of tabs by opening them one at a time?

This is how I largely did it. I also had a folder of about 10-11 sites I had in a folder on the Bookmarks Toolbar, and clicked "Open All in Tabs" almost every day, so over a month, I had a few hundred tabs that I sometimes open and close each one.

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

So, each tab contained some page that you had already visited? 

Sorry, I'm trying to put together steps that I can use to verify. I can open 300 tabs, but what URL(s) to use is my first question.

OR... if you can easily check to see if it's fixed on your end, you could do that and we could then just mark this "verified."

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Matt Wobensmith from comment #27)
> So, each tab contained some page that you had already visited? 

Nope, not necessarily. The "Open All in Tabs" option opened tabs that I may have opened before.

In any case, for my use case I didn't hit this again, so let's just set it as verified.