CacheFileIOManager::WriteInternal writes uninitialised padding bytes to disk

RESOLVED FIXED in Firefox 52

Status

()

Core
Networking: Cache
RESOLVED FIXED
a year ago
26 days ago

People

(Reporter: jseward, Assigned: michal)

Tracking

({csectype-disclosure, privacy, sec-low})

Trunk
mozilla53
csectype-disclosure, privacy, sec-low
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
In longer runs (10 mins+) of mochitests on valgrind, for the
tests in layout/style/test, it is sometimes reported that the
CacheFileIOManager is writing heap allocated uninitialised values
to disk.  In netwerk/cache2/CacheIndex.cpp, setting kMinUnwrittenChanges
and kMinDumpInterval both to 1 causes it to be reported at every
browser startup.  The uninitialised values originate from alignment
holes in struct CacheIndexRecord.
(Reporter)

Comment 1

a year ago
Created attachment 8815209 [details]
Valgrind complaint
(Reporter)

Comment 2

a year ago
The layout of struct CacheIndexRecord (I would guess) is

  SHA1Sum::Hash   mHash;               0..19
  uint32_t        mFrecency;          20..23
  uint32_t        mExpirationTime;    24..27
  // Alignment hole #1                28..31
  OriginAttrsHash mOriginAttrsHash;   32..40
  uint32_t      mFlags;               40..43
  // Alignment hole #2                44..47

on the assumption that OriginAttrsHash a.k.a. uint64_t forces 64-bit
alignment and that the struct as a whole must have a 64-bit aligned
size.
(Reporter)

Comment 3

a year ago
Created attachment 8815218 [details] [diff] [review]
bug1320894-1.cset

A possible fix.  This changes the layout to

  SHA1Sum::Hash   mHash;              0..19
  uint32_t        mFrecency;         20..23
  OriginAttrsHash mOriginAttrsHash;  24..31
  uint32_t        mExpirationTime;   32..35
  uint32_t        mFlags;            36..39

At least on x86_64-Linux, it stops Valgrind complaining, and the
static assertion holds.

I don't like it as a solution though.  This surely changes the
on-disk format, and it is fragile .. if the compilers change the
layout it could fail again.

It appears that the on-disk format created here is dependant on
the layout that compilers choose for this struct.  Is that intended,
or do I misunderstand something?
(Reporter)

Updated

a year ago
Attachment #8815218 - Flags: feedback?(michal.novotny)
(Assignee)

Comment 4

a year ago
Created attachment 8815375 [details] [diff] [review]
fix

- members are reordered so there are no holes
- structs CacheIndexHeader and CacheIndexRecord are now properly serialized instead of using memcpy
- parsing of cache index with holes would fail, but I bumped version so we won't even try to parse previously written index
Assignee: nobody → michal.novotny
Attachment #8815218 - Attachment is obsolete: true
Attachment #8815218 - Flags: feedback?(michal.novotny)
Attachment #8815375 - Flags: review?(valentin.gosu)
Attachment #8815375 - Flags: feedback?(jseward)
(Reporter)

Comment 5

a year ago
Comment on attachment 8815375 [details] [diff] [review]
fix

Works OK for me.
Attachment #8815375 - Flags: feedback?(jseward) → feedback+
Comment on attachment 8815375 [details] [diff] [review]
fix

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

::: netwerk/cache2/CacheIndex.cpp
@@ +2014,5 @@
>  
>    rv = indexFile->OpenNSPRFileDesc(PR_RDWR, 0600, &fd);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  int64_t offset = PR_Seek64(fd, 2 * sizeof(uint32_t), PR_SEEK_SET);

Add comment mentioning why we seek to this position.
Also add a static_assert to verify that offsetof(CacheIndexHeader, mIsDirty) is 2*sizeof(uint32_t).

@@ +2154,3 @@
>      } else {
> +      uint32_t * isDirty = reinterpret_cast<uint32_t *>(
> +                             moz_xmalloc(sizeof(uint32_t)));

We can probably avoid this extra allocation, right?

@@ +2160,5 @@
>        // nullptr is passed as the listener and the call doesn't fail
>        // synchronously.
> +      rv = CacheFileIOManager::Write(mIndexHandle, 2 * sizeof(uint32_t),
> +                                     reinterpret_cast<char *>(isDirty),
> +                                     sizeof(isDirty), true, false, nullptr);

BUG: Since isDirty is a pointer, it's size isn't necessarily 4 bytes. Probably best to use sizeof(uint32_t).
Attachment #8815375 - Flags: review?(valentin.gosu)
(Assignee)

Comment 7

a year ago
Created attachment 8815472 [details] [diff] [review]
patch v2

Thanks for quick review.

(In reply to Valentin Gosu [:valentin] from comment #6)
> > +  int64_t offset = PR_Seek64(fd, 2 * sizeof(uint32_t), PR_SEEK_SET);
> 
> Add comment mentioning why we seek to this position.
> Also add a static_assert to verify that offsetof(CacheIndexHeader, mIsDirty)
> is 2*sizeof(uint32_t).

fixed

> @@ +2154,3 @@
> >      } else {
> > +      uint32_t * isDirty = reinterpret_cast<uint32_t *>(
> > +                             moz_xmalloc(sizeof(uint32_t)));
> 
> We can probably avoid this extra allocation, right?

No. When CacheFileIOManager::Write is called with the callback, the buffer is owned by the caller and must not be released until the callback is called. This is not our case here, we're neither interested in the result, nor we have a suitable buffer to be used for this. When CacheFileIOManager::Write is called without a callback, the buffer ownership is transferred and it is released by CacheFileIOManager, so we have to allocate the buffer here.

> > +      rv = CacheFileIOManager::Write(mIndexHandle, 2 * sizeof(uint32_t),
> > +                                     reinterpret_cast<char *>(isDirty),
> > +                                     sizeof(isDirty), true, false, nullptr);
> 
> BUG: Since isDirty is a pointer, it's size isn't necessarily 4 bytes.
> Probably best to use sizeof(uint32_t).

Good catch! Fixed.
Attachment #8815375 - Attachment is obsolete: true
Attachment #8815472 - Flags: review?(valentin.gosu)
Comment on attachment 8815472 [details] [diff] [review]
patch v2

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

Looks good. Thanks!
Attachment #8815472 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 9

a year ago
Created attachment 8815690 [details] [diff] [review]
patch v3

fixed a typo that cleared flags during writing in CacheIndexEntry::WriteToBuf
Attachment #8815472 - Attachment is obsolete: true
Attachment #8815690 - Flags: review+
(Assignee)

Comment 10

a year ago
Comment on attachment 8815690 [details] [diff] [review]
patch v3

I'm not sure whether this patch needs security-approval before landing.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I guess it's impossible. User have no control over the uninitialized data going to disk. When the data is read back it is stored again in the hole in the structure.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think this is really a security problem.

Which older supported branches are affected by this flaw?
This was introduced with bug 1201042 which was uplifted to 51.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It's likely that the patch can be used on 51 as is. If not, it's easy to backport it.

How likely is this patch to cause regressions; how much testing does it need?
Cache index should be written to disk when running mochitests if there are enough changes to cache and if the tests run at least 60 seconds. But we don't have a test that would check the written data. I.e. any changes in cache data format are usually checked manually.
Attachment #8815690 - Flags: sec-approval?
Group: core-security → network-core-security
Slight info leak, but fragmentary and only to local disk so doesn't seem too bad from a security POV.
Keywords: csectype-disclosure, privacy, sec-low
Comment on attachment 8815690 [details] [diff] [review]
patch v3

land away! (sec-approval+)
Attachment #8815690 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/331ca1a40ca2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

11 months ago
Group: network-core-security → core-security-release
Can we nominate this for Beta approval please?
status-firefox51: --- → wontfix
status-firefox52: --- → affected
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 15

10 months ago
Comment on attachment 8815690 [details] [diff] [review]
patch v3

Approval Request Comment
[Feature/Bug causing the regression]: 1201042
[User impact if declined]: uninitialized padding data is written to a local file
[Is this code covered by automated tests?]: cache index is written when running automated tests but it is never read from the disk
[Has the fix been verified in Nightly?]: the fix has been verified manually before landing and it is in the nightly already for quite a long time
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change in index format, in the worst  case we would not be able to parse the index after startup and we would need to build it from the scratch
[String changes made/needed]: none
Flags: needinfo?(michal.novotny)
Attachment #8815690 - Flags: approval-mozilla-beta?
Comment on attachment 8815690 [details] [diff] [review]
patch v3

avoid info leak, beta52+
Attachment #8815690 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/5dc896e5b700
status-firefox52: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr52/rev/5dc896e5b700
status-firefox-esr52: --- → fixed
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.