Closed Bug 1021612 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking: Cache, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- verified
firefox33 --- verified
firefox34 --- verified
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed

People

(Reporter: gkw, Assigned: michal)

Details

(Keywords: csectype-bounds, regression, sec-critical)

Attachments

(3 files)

Attached file 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.
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.
(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.
Michal, this looks pretty serious, can you pleas look at it?
Assignee: nobody → michal.novotny
(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?
> Is there any chance to get the line numbers in the stack?

Probably not, but I'm not 100% sure.
Wish bug 966024 has already been done by now...  so little time :(
Attached patch fixSplinter Review
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().
Attachment #8436226 - Flags: review?(honzab.moz)
Attached patch test v1Splinter Review
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 on attachment 8436226 [details] [diff] [review]
fix

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

Thanks for a quick fix.
Attachment #8436226 - Flags: review?(honzab.moz) → review+
> Do I actually need ASAN to reproduce at least the crash?

Probably.
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.
Attachment #8436310 - Flags: feedback+
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?
Flags: needinfo?(michal.novotny)
This is nightly only problem, since the code is disabled in other branches.
Flags: needinfo?(michal.novotny)
https://hg.mozilla.org/mozilla-central/rev/5fa491e07b4b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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.
(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 on attachment 8436310 [details] [diff] [review]
test v1

This should probably go in.
Attachment #8436310 - Flags: review?(michal.novotny)
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.
Attachment #8436310 - Flags: review?(michal.novotny) → review+
(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!
(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!
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.
Flags: needinfo?(gary)
(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.
Flags: needinfo?(gary)
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."
Flags: needinfo?(gary)
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.