451754 - valgrind - invalid read in rc4_wordconv

Status: RESOLVED DUPLICATE of bug 341127

(NSS :: Libraries, defect)

Description

[Bob Clary [:bc] (inactive)]

start stop browser. occurs in 1.9.0 and 1.9.1

==24010== Invalid read of size 4
==24010==    at 0xC793688: rc4_wordconv (arcfour.c:548)
==24010==    by 0xC7939EF: RC4_Encrypt (arcfour.c:608)
==24010==    by 0x6FF81E0: RC4_Encrypt (loader.c:358)
==24010==    by 0x6FDAC40: NSC_EncryptUpdate (pkcs11c.c:821)
==24010==    by 0x6E20F7A: PK11_CipherOp (pk11cxt.c:730)
==24010==    by 0x6DAFF75: ssl3_CompressMACEncryptRecord (ssl3con.c:1885)
==24010==    by 0x6DB03D6: ssl3_SendRecord (ssl3con.c:2002)
==24010==    by 0x6DB081E: ssl3_SendApplicationData (ssl3con.c:2118)
==24010==    by 0x6DCECF5: ssl_SecureSend (sslsecur.c:1180)
==24010==    by 0x6DCEDE1: ssl_SecureWrite (sslsecur.c:1197)
==24010==    by 0x6DD580D: ssl_Write (sslsock.c:1487)
==24010==    by 0x6CDF29D: nsSSLThread::Run() (nsSSLThread.cpp:1029)
==24010==  Address 0x7F2F698 is 664 bytes inside a block of size 666 alloc'd
==24010==    at 0x40053C0: malloc (vg_replace_malloc.c:149)
==24010==    by 0x4332BFD: PR_Malloc (prmem.c:467)
==24010==    by 0x42C5EAA: NS_Alloc_P (nsMemoryImpl.cpp:281)
==24010==    by 0x6CFADFE: nsMemory::Alloc(unsigned) (nsMemory.h:68)
==24010==    by 0x6D04F86: nsSSLSocketThreadData::ensure_buffer_size(int) (nsNSSIOLayer.cpp:192)
==24010==    by 0x6CDF8E3: nsSSLThread::requestWrite(nsNSSSocketInfo*, void const*, int, unsigned) (nsSSLThread.cpp:866)
==24010==    by 0x6D00500: PSMSend(PRFileDesc*, void const*, int, int, unsigned) (nsNSSIOLayer.cpp:1880)
==24010==    by 0x6D00546: nsSSLIOLayerWrite(PRFileDesc*, void const*, int) (nsNSSIOLayer.cpp:1892)
==24010==    by 0x4328019: PR_Write (priometh.c:146)
==24010==    by 0x6B53058: nsSocketOutputStream::Write(char const*, unsigned, unsigned*) (nsSocketTransport2.cpp:576)
==24010==    by 0x6BD745B: nsHttpConnection::OnReadSegment(char const*, unsigned, unsigned*) (nsHttpConnection.cpp:530)
==24010==    by 0x6BE5F45: nsHttpTransaction::ReadRequestSegment(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*) (nsHttpTransaction.cpp:411)

Comment 1

[Daniel Veditz [:dveditz]]

Lot's of 
   /* UMR? see comments above. */
in that file. would be nice to make those go away if possible just to eliminate continued false positives from tools like valgrind/purify.

Comment 2

[Bob Clary [:bc] (inactive)]

sorry if this is bugspam. 

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Not a vulnerability.

Comment 4

[Wan-Teh Chang]

The error is not UMR, but rather ABR (array bounds read).
We should at least update those comments about UMR in arcfour.c.

Why is the error not UMR?

Suppose 'short' is 2 bytes.  Define

    struct {
        short before_buf;
        unsigned char buf[1024];
        short after_buf;
    } s;

We fill s.buf with plaintext data to be encrypted.
Suppose we also initialize s.before_buf and s.after_buf.
Then RC4_Encrypt(..., s.buf, 1024) will read s.before_buf
and s.after_buf, before and after the bounds of the s.buf
array, but these reads are not UMR.

Optimized implementations of memcpy need to solve the
exact same problem.  (Think of memcpy as null encryption.)
We could find out if they have a solution that doesn't do
ABR.

Comment 5

[Wan-Teh Chang]

The call stack referred to arcfour.c:548, rev. 1.17:

               /* If the amount of remaining input is greater than the amount
                * bytes pulled from the current input word, need to do another
                * word load.  What's left in inWord will be consumed in step 3.
                */
               if (inputLen > WORDSIZE - inOffset)
548                    inWord |= *pInWord RSH bufShift; /* UMR?  See above. */
       } else {

Comment 6

[Wan-Teh Chang]

1. Here is the link to memcpy() in FreeBSD:
http://svn.freebsd.org/viewvc/base/head/lib/libc/string/bcopy.c?view=markup

Note that memcpy, memmove, and bcopy share the same
implementation.  For memcpy it is enough to just look
at the "Copy forward." part.

FreeBSD's memcpy() doesn't try as hard as our
rc4_wordconv when the input and output buffers have
different offsets from a word boundary.

2. glibc's portable memcpy code is in the following
files:
  string/memcpy.c
  sysdeps/generic/memcopy.h
  string/wordcopy.c

It is much more sophisticated than FreeBSD's memcpy(),
comparable to our rc4_wordconv.

Both implementations do byte copies at the beginning and
end.  Neither resort to invalid reads for performance.