1320894 - CacheFileIOManager::WriteInternal writes uninitialised padding bytes to disk

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint

Comment 2

[Julian Seward [:jseward]]

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.

Comment 3

[Julian Seward [:jseward]]

Attachment: 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?

Comment 4

[Michal Novotny [:michal]]

Attachment: 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

Comment 5

[Julian Seward [:jseward]]

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

Works OK for me.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

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).

Comment 7

[Michal Novotny [:michal]]

Attachment: 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.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8815472 [details] [diff] [review]
patch v2

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

Looks good. Thanks!

Comment 9

[Michal Novotny [:michal]]

Attachment: patch v3

fixed a typo that cleared flags during writing in CacheIndexEntry::WriteToBuf

Comment 10

[Michal Novotny [:michal]]

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.

Comment 11

[Daniel Veditz [:dveditz]]

Slight info leak, but fragmentary and only to local disk so doesn't seem too bad from a security POV.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8815690 [details] [diff] [review]
patch v3

land away! (sec-approval+)

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/331ca1a40ca2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Can we nominate this for Beta approval please?

Comment 15

[Michal Novotny [:michal]]

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

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8815690 [details] [diff] [review]
patch v3

avoid info leak, beta52+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5dc896e5b700

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5dc896e5b700