184614 - valgrind doesn't like nsDiskCacheBlockFile::WriteBlocks (uninitialized memory written to cache)

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[timeless]

mozilla-cvs from yesterday, valgrind, qt port w/ fixes committed today.
starting up (mozilla -P foo http://www.mozilla.org)
[note that imagelib is borked, but i doubt that's relevant]

==27375== epoch 7102 (bb 355100k): thresh 201, out 17541 (283k -> 4215k), new TT
110843, TC 27074k
==27375==
==27375== Syscall param write(buf) contains uninitialised or unaddressable byte(s)
==27375==    at 0x406D6694: __libc_write (in /lib/libc-2.2.5.so)
==27375==    by 0x4056028A: pt_Write
(/mnt/hda3/temp/mozilla/nsprpub/pr/src/pthreads/ptio.c:1295)
==27375==    by 0x4053EAB9: PR_Write
(/mnt/hda3/temp/mozilla/nsprpub/pr/src/io/priometh.c:141)
==27375==    by 0x44271B85: nsDiskCacheBlockFile::WriteBlocks(void *, int, int)
(/mnt/hda3/temp/mozilla/netwerk/cache/src/nsDiskCacheBlockFile.cpp:284)
==27375==    Address 0x4CB6345A is 16374 bytes inside a block of size 16384 alloc'd
==27375==    at 0x40046D2B: malloc (vg_clientfuncs.c:100)
==27375==    by 0x40047294: realloc (vg_clientfuncs.c:262)
==27375==    by 0x4427C108: nsDiskCacheStreamIO::WriteToBuffer(char const *,
unsigned int) (/mnt/hda3/temp/mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp:763)
==27375==    by 0x4427B9A4: nsDiskCacheStreamIO::Write(char const *, unsigned
int, unsigned int *)
(/mnt/hda3/temp/mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp:605)
==27375== epoch 7117 (bb 355850k): thresh 192, out 17296 (278k -> 4255k), new TT
111685, TC 27035k

<drepper> timeless: I would take this warning serious
<drepper> timeless: it might be that the problem is harmless if the reader
<drepper> timeless: but still, it should not happen

Comment 1

[Ulrich Drepper]

The second of my lines should read:

it might be that the problem is harmless if the reader ignores the uninitialized
bytes


And all this also depends on the accuracy of valgrind computations but with
regard of uninitialized data it is usually correct.  So, as I said, I'd take
this serious.

Comment 2

[Ulrich Drepper]

Since valgrind isn't available on Windows I took the liberty to fix the OS field.

Comment 3

[David Bradley]

Purify doesn't like this either.

Consider this, what ever happens to be in memory, passwords, credit card
numbers, social security numbers, etc. could be getting written to cache files
(if this is writing cache files to disk).

cc'ing Mitch just to get his take on this.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

David,
   I'm not clear on the issue, but I agree that we should prevent sensitive
information from being written to disk where it can be avoided. We should have a
"secure string" class that a) is never swapped to disk, and b) is overwritten in
memory before it is freed.

Comment 5

[David Bradley]

1. A string is allocated to hold a password
2. It is later freed
3. Next, a buffer is created, and happens to get the same memory
4. We don't initialize all the memory, but we write out the full buffer
5. Password gets written to the tail end of the cache entry.

I don't know the probability of this occuring, it's probably pretty small, but
there are thousands of cache entries. So that might increase the chances. And
hard to say whether someone could successfully exploit this.

Comment 6

[Ulrich Drepper]

> 1. A string is allocated to hold a password
> 2. It is later freed
> 3. Next, a buffer is created, and happens to get the same memory
> 4. We don't initialize all the memory, but we write out the full buffer
> 5. Password gets written to the tail end of the cache entry.

This is a problem and definitely needs to be fixed.  But it is also a bug to not
have step

1a. the memory used for the password is overwritten

This should be done even if the memory is not freed but the password isn't used
anymore.  Otherwise one might end up with corefiles containing the password.

Might be worthwhile to examine the places where mozilla handles passwords.

Comment 7

[David Bradley]

Right, and that's what Mitch was talking about in his comment about special
string class. And maybe it's more than just a string class, but a special
allocator, such that any memory freed is wiped when freed, since other sensitive
data might not be just strings.

Even with such a string class, or allocator, there's nothing to keep a crash
that happens when the data is actually being used, from getting dumped to that
file. I'm not sure how you could completely prevent this from appearing in a
core file.

The solutions for such problems will not be easy to implement. However, fixing
this bug should be fairly straight forward, and plug one potential source of
having the data leak out.

Comment 8

[Darin Fisher]

this bug was also reported in bug 190668 (someone stumbled upon it while trying
to debug a crash).

Comment 9

[Darin Fisher]

i'm also seeing this using valgrind.  investigating.. 

Comment 10

[Darin Fisher]

ok, here's what's going on...  loading http://www.mozilla.org/start.html,
nsDiskCacheStreamIO::Flush() calls:

  cacheMap->WriteDataCacheBlocks(mBinding, mBuffer, mBufEnd);

with mBufEnd == 14794

however, nsDiskCacheMap::WriteDataCacheBlocks pretty much ignores its |size|
argument assuming the buffer size was rounded to the nearest block size.  in
this case, 16k.  apparently valgrind is telling us that the allocation size is
not 16k.

security folks shouldn't worry too much about this.  the cache does store the
correct data size.  so, even though we write junk data to the block files, only
the valid length of bytes (stored in the meta data) will be fed back into the app.

looking briefly at the code, i can't really see a way for the buffer to be less
than 16k.  investigating some more...

Comment 11

[Darin Fisher]

hmm... mBufSize == 16384

Comment 12

[Darin Fisher]

this bug is INVALID or maybe WONTFIX.  i didn't look closely enough at the
valgrind warning:

  Syscall param write(buf) contains uninitialised or unaddressable byte(s)

there's no bug here.  we intend to write uninitialized bytes.

Comment 13

[timeless]

discussed with darin, the concern is that the data we don't initialize could
have private information from before cache allocates it.

Comment 14

[Darin Fisher]

so, it's probably sufficient to just replace the malloc's with calloc's, but
some well placed memset's would of course be more efficient.

gordon: what do you think?

Comment 15

[Alfred Kayser]

There are two placed where nsDiskCacheBlockFile::WriteBlocks is called from:
one is from DiskCacheEntry and the other DiskCacheStream. Both make sure that
the buffer is large enough (see above), but only the first makes sure
that the 'unused' bytes are zerod. So, just DiskCacheStream need to also
memset the remaining bytes in the block, in the same fashion as
CreateDiskCacheEntry... The only reason though that WriteBlocks write a block
sized buffer (round to nearest 256, 1024 or 4096) is to make sure that the
cache file is extended for the next write. So, may be instead of writing
'unused' some other of ensuring block-stepwise file growth...

Comment 16

[Matt]

Still happens with firefox current CVS with valgrind 3.1-pre:

==5222== Syscall param write(buf) points to uninitialised byte(s)
==5222==    at 0x536D32F: write (in /lib/libpthread-0.10.so)
==5222==    by 0x524A163: pt_Write (in /home/matt/src/mozilla/firefox-bin/nsprpub/pr/src/libnspr4.so)
==5222==    by 0x85B24AF: nsDiskCacheBlockFile::WriteBlocks(void*, int, int) (nsDiskCacheBlockFile.cpp:304)
==5222==    by 0x85B5BF1: nsDiskCacheMap::WriteDataCacheBlocks(nsDiskCacheBinding*, char*, unsigned) (nsDiskCacheMap.cpp:845)
==5222==    by 0x85B7206: nsDiskCacheStreamIO::Flush() (nsDiskCacheStreams.cpp:515)
==5222==    by 0x85B7113: nsDiskCacheStreamIO::CloseOutputStream(nsDiskCacheOutputStream*) (nsDiskCacheStreams.cpp:462)
==5222==    by 0x85B6B89: nsDiskCacheOutputStream::Close() (nsDiskCacheStreams.cpp:240)
==5222==    by 0x85AC010: nsCacheEntryDescriptor::nsOutputStreamWrapper::Close() (nsCacheEntryDescriptor.cpp:606)
==5222==    by 0x85AC328: nsCacheEntryDescriptor::nsOutputStreamWrapper::~nsOutputStreamWrapper() (nsCacheEntryDescriptor.h:140)
==5222==    by 0x85ABDF7: nsCacheEntryDescriptor::nsOutputStreamWrapper::Release() (nsCacheEntryDescriptor.cpp:558)
==5222==    by 0x4E69C71: nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) (nsCOMPtr.h:531)
==5222==    by 0x4E69A86: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.cpp:89)
==5222==  Address 0x101C6AEB is 8,811 bytes inside a block of size 16,384 alloc'd
==5222==    at 0x4A18AC6: malloc (vg_replace_malloc.c:149)
==5222==    by 0x4A19E50: realloc (vg_replace_malloc.c:306)
==5222==    by 0x85B7673: nsDiskCacheStreamIO::WriteToBuffer(char const*, unsigned) (nsDiskCacheStreams.cpp:719)
==5222==    by 0x85B72C8: nsDiskCacheStreamIO::Write(char const*, unsigned, unsigned*) (nsDiskCacheStreams.cpp:561)
==5222==    by 0x85B6BB9: nsDiskCacheOutputStream::Write(char const*, unsigned, unsigned*) (nsDiskCacheStreams.cpp:259)
==5222==    by 0x85AC097: nsCacheEntryDescriptor::nsOutputStreamWrapper::Write(char const*, unsigned, unsigned*) (nsCacheEntryDescriptor.cpp:629)
==5222==    by 0x4E957B2: nsInputStreamTee::TeeSegment(char const*, unsigned) (nsInputStreamTee.cpp:80)
==5222==    by 0x4E95806: nsInputStreamTee::WriteSegmentFun(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*) (nsInputStreamTee.cpp:109)
==5222==    by 0x4E98FFA: nsPipeInputStream::ReadSegments(unsigned (*)(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*), void*, unsigned, unsigned*) (nsPipe3.cpp:762)
==5222==    by 0x4E95AE6: nsInputStreamTee::ReadSegments(unsigned (*)(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*), void*, unsigned, unsigned*) (nsInputStreamTee.cpp:156)

Comment 17

[Alfred Kayser]

Attachment: V1: Only write real data to the DiskCacheBlock files

Simple patch to make nsDiskCacheBlockFile::WriteBlocks only write the real size of the blocks. This prevents writing 'unsafe' data from memory to a file.
Also it prevents the need to pad the allocated diskEntry to whole blocksizes.

Comment 18

[Darin Fisher]

Comment on attachment 219907 [details] [diff] [review]
V1: Only write real data to the DiskCacheBlock files

>Index: nsDiskCacheBlockFile.cpp
...
> nsDiskCacheBlockFile::AllocateBlocks(PRInt32 numBlocks)
> {
>-    if (!mFD)  return -1;  // NS_ERROR_NOT_AVAILABLE;

Can you explain this change?  Why not do the same in DeallocateBlocks?

I'm not sure I understand the mFD null check.


r=darin

Comment 19

[Alfred Kayser]

Attachment: V2: Reading blockdata may be shorter than whole blocks

Reading the last blocks of a blockfile will not return whole blocks, because the write only wrote the real data. So the blockread functions needed also to be adapted. Side effect is that the read buffer for reading from the blocksize now also is only allocated for just the datasize needed (so for example for a 5123bytes object, instead of allocating 8K, just 5K is allocated).

About the mFD check in AllocateBlocks: AllocateBlocks is now private to nsDiskCacheBlockFile, and is only called from WriteBlocks where mFD is allready checked. DeallocateBlocks is called from nsDiskCacheMap requiring the check.

Note: fixed two other very minor things:
* moved two constants into nsDiskCacheBlockFile.cpp
* used PR_MALLOC instead of malloc directly

The rest is the same as previous patch

Comment 20

[Darin Fisher]

Comment on attachment 220027 [details] [diff] [review]
V2: Reading blockdata may be shorter than whole blocks

>Index: nsDiskCacheBlockFile.cpp

>+nsDiskCacheBlockFile::ReadBlocks( void *    buffer,
>+                                  PRInt32   startBlock,
>+                                  PRInt32   numBlocks,
>+                                  PRInt32 & bytesRead)

I prefer to use pointers for out-params so that the callsite
is self-documenting about which params are out-params:

  ReadBlocks(buffer, startBlock, numBlocks, &bytesRead)

When I read that, I know that bytesRead is an out-param :)


>Index: nsDiskCacheStreams.cpp

>-                mBuffer  = (char *) realloc(mBuffer, mBufSize);
>+                mBuffer  = (char *) PR_REALLOC(mBuffer, mBufSize);

What is the reason for this change?  PR_REALLOC is just going
to call realloc.


r=darin

Comment 21

[Alfred Kayser]

Attachment: V3: Two comments from darin addressed and some redundant includes removed

Comment 22

[Boris Zbarsky [:bzbarsky]]

Please ask someone else for sr; I won't be able to get to this review within a reasonable timeframe (this month).

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 220536 [details] [diff] [review]
V3: Two comments from darin addressed and some redundant includes removed

unfortunately I'm not a superreviewer...

Comment 24

[Darin Fisher]

Comment on attachment 220536 [details] [diff] [review]
V3: Two comments from darin addressed and some redundant includes removed

That fine.  I already reviewed this one.

Comment 25

[Mats Palmgren (inactive)]

V3 fails to compile on SuSE Linux 9.0, gcc 3.3.1:

nsDiskCacheBlockFile.cpp: In member function `nsresult
   nsDiskCacheBlockFile::Open(nsILocalFile*, unsigned int)':
nsDiskCacheBlockFile.cpp:112: warning: jump to label `error_exit'
nsDiskCacheBlockFile.cpp:67: warning:   from here
nsDiskCacheBlockFile.cpp:71: error:   crosses initialization of `PRInt32
   fileSize'

Moving back the declaration of 'fileSize' to the top fixed it.

Comment 26

[Alfred Kayser]

Attachment: V4: Moved back the declaration of 'fileSize' to the top

Darin, can you do the SR, or must someelse (who?) do this?

Comment 27

[Darin Fisher]

Comment on attachment 222617 [details] [diff] [review]
V4: Moved back the declaration of 'fileSize' to the top

>Index: nsDiskCacheBlockFile.cpp

> nsDiskCacheBlockFile::Open( nsILocalFile *  blockFile, PRUint32  blockSize)
> {
>-    PRInt32   fileSize;
>+	PRInt32 fileSize;

no tabs please :)


sr=darin

Comment 28

[Alfred Kayser]

Attachment: v4b: removed the tab

Darin, can you do the honours and check this in for me?
Thanks in advance!

Comment 29

[Darin Fisher]

-> alfred

Comment 30

[Darin Fisher]

fixed-on-trunk

Comment 31

[Alfred Kayser]

Verified as working as designed! Thanks!

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I still see these valgrind warnings quite frequently.  Reopening.

Comment 33

[Alfred Kayser]

David, could you report where exactly these valgrinds occur, and where they happen?

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind warning

This is the valgrind warning I see after starting up with a URL on the command line, using Firefox trunk as of Tue Jan 23 14:43:12 PST 2007 (with a bunch of patches in my tree, but nothing substantive in networking code)

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The warning always seems to be for the word before the end of the block.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll file a separate bug for this; it seems like a new problem.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 368119; restoring this bug to its state before I reopened.