Closed Bug 1034672 Opened 5 years ago Closed 5 years ago

CacheFileMetadata::WriteMetadata swaps arguments to memcpy

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

(Keywords: sec-low)

Attachments

(1 file)

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.
BTW: credits to Dragana! :)
Attached patch v1Splinter Review
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8451763 - Flags: review?(michal.novotny)
Attachment #8451763 - Flags: review?(michal.novotny) → review+
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).
Attachment #8451763 - Flags: sec-approval?
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!
Attachment #8451763 - Flags: sec-approval?
Group: network-core-security → core-security
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.
Attachment #8451763 - Flags: approval-mozilla-aurora?
Comment on attachment 8451763 [details] [diff] [review]
v1

Aurora+ Although this doesn't need to it can land concurrently on m-c and aurora.
Attachment #8451763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4bd4e61980de
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
Flags: qe-verify-
Group: core-security
You need to log in before you can comment on or make changes to this bug.