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

VERIFIED FIXED

Status

()

Core
Networking: Cache
VERIFIED FIXED
16 years ago
11 years ago

People

(Reporter: timeless, Assigned: Alfred Kayser)

Tracking

({privacy, valgrind})

Trunk
x86
All
privacy, valgrind
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
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.

Updated

16 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.3beta

Comment 2

16 years ago
Since valgrind isn't available on Windows I took the liberty to fix the OS field.
OS: Windows 2000 → Linux

Comment 3

16 years ago
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.
OS: Linux → All
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

16 years ago
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

16 years ago
> 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

16 years ago
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

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

Comment 9

16 years ago
i'm also seeing this using valgrind.  investigating.. 

Comment 10

16 years ago
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

16 years ago
hmm... mBufSize == 16384

Comment 12

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 13

16 years ago
discussed with darin, the concern is that the data we don't initialize could
have private information from before cache allocates it.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 14

16 years ago
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?
(Assignee)

Comment 15

13 years ago
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

13 years ago
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)

Updated

12 years ago
Assignee: gordon → nobody
Status: REOPENED → NEW
Keywords: privacy
Priority: P2 → --
QA Contact: tever → networking.cache
Summary: valgrind doesn't like nsDiskCacheBlockFile::WriteBlocks → valgrind doesn't like nsDiskCacheBlockFile::WriteBlocks (uninitialized memory written to cache)
Target Milestone: mozilla1.3beta → ---
(Assignee)

Comment 17

12 years ago
Created attachment 219907 [details] [diff] [review]
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.
Attachment #219907 - Flags: review?(darin)

Comment 18

12 years ago
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
Attachment #219907 - Flags: review?(darin) → review+
(Assignee)

Comment 19

12 years ago
Created attachment 220027 [details] [diff] [review]
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
Attachment #219907 - Attachment is obsolete: true
Attachment #220027 - Flags: review?(darin)

Comment 20

12 years ago
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
Attachment #220027 - Flags: review?(darin) → review+
(Assignee)

Comment 21

12 years ago
Created attachment 220536 [details] [diff] [review]
V3: Two comments from darin addressed and some redundant includes removed
Attachment #220027 - Attachment is obsolete: true
Attachment #220536 - Flags: superreview?(bzbarsky)
Please ask someone else for sr; I won't be able to get to this review within a reasonable timeframe (this month).
(Assignee)

Updated

12 years ago
Attachment #220536 - Flags: superreview?(bzbarsky) → superreview?(cbiesinger)
Comment on attachment 220536 [details] [diff] [review]
V3: Two comments from darin addressed and some redundant includes removed

unfortunately I'm not a superreviewer...
Attachment #220536 - Flags: superreview?(cbiesinger)

Comment 24

12 years ago
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.
Attachment #220536 - Flags: review?(cbiesinger)
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.
(Assignee)

Comment 26

12 years ago
Created attachment 222617 [details] [diff] [review]
V4: Moved back the declaration of 'fileSize' to the top

Darin, can you do the SR, or must someelse (who?) do this?
Attachment #220536 - Attachment is obsolete: true
Attachment #222617 - Flags: superreview?(darin)
Attachment #220536 - Flags: review?(cbiesinger)

Comment 27

12 years ago
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
Attachment #222617 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 28

12 years ago
Created attachment 223149 [details] [diff] [review]
v4b: removed the tab

Darin, can you do the honours and check this in for me?
Thanks in advance!
Attachment #222617 - Attachment is obsolete: true

Comment 29

12 years ago
-> alfred
Assignee: nobody → alfredkayser

Comment 30

12 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 16 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

12 years ago
Verified as working as designed! Thanks!
Status: RESOLVED → VERIFIED
I still see these valgrind warnings quite frequently.  Reopening.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

12 years ago
David, could you report where exactly these valgrinds occur, and where they happen?
Created attachment 252679 [details]
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)
The warning always seems to be for the word before the end of the block.
I'll file a separate bug for this; it seems like a new problem.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
I filed bug 368119; restoring this bug to its state before I reopened.
Status: RESOLVED → VERIFIED
Keywords: valgrind
You need to log in before you can comment on or make changes to this bug.