1034672 - CacheFileMetadata::WriteMetadata swaps arguments to memcpy

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

When WriteMetadata is called from the destructor (which is probably rare) we store to the metadata buffer unitialized (just allocated) memory instead of the real metadata.

See http://hg.mozilla.org/mozilla-central/annotate/ac6960197eb6/netwerk/cache2/CacheFileMetadata.cpp#l275

Introduced in bug 949250.

Rather reporting as a sec bug since this (very very potentially) could be used for some kind of metadata spoofing.

Comment 1

[Honza Bambas (:mayhemer)]

BTW: credits to Dragana! :)

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8451763 [details] [diff] [review]
v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Probably not very easily, this is low-rate execution code (AFAIK), however we could write cache metadata that are under control of an attacker, since we write an uninitialized just allocated heap memory.  This could lead to bad cache poisoning.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Could be, the patch is a one liner, pretty obvious what's wrong.

Which older supported branches are affected by this flaw?  Aurora.

If not all supported branches, which bug introduced the flaw?  913806 that enabled this code for ordinary users, but actually landed disabled on Fx 27

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Super easy to backport, that patch should apply on Aurora (slight merging/fuzz patch only may be needed).

How likely is this patch to cause regressions; how much testing does it need?  Zero (I believe) and not much...  I'll do manual tests, this code is however harder to trigger (pretty corner case).

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8451763 [details] [diff] [review]
v1

Since this is a sec-low (not a high or critical) security issue, you don't need sec-approval+ to check in. Go ahead and check it into trunk!

Comment 5

[Jason Duell]

Comment on attachment 8451763 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: cache2
[User impact if declined]: possible security hole
[Risks and why]:  exceedingly low: just fixing order of 2 args to memcpy
[String/UUID change made/needed]: none

cache 2 is on aurora, it would be worth it to get this simple one line fix on there before we move to beta.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8451763 [details] [diff] [review]
v1

Aurora+ Although this doesn't need to it can land concurrently on m-c and aurora.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd4e61980de
https://hg.mozilla.org/releases/mozilla-aurora/rev/b19c6c55d3dd

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/4bd4e61980de

Comment 9

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

Rare condition gleaned from a security audit + sec-low + no test media = marking qe-verify-. 

If you decide you need help with verification, just set needinfo to me and provide more information on how to proceed. Thank you.