341127 - Invalid read in rc4_wordconv

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Andrew Schultz]

With linux seamonkey CVS trunk from Jun 10, I get the following from valgrind when loading https://klient1.ebanka.cz/ebts/version_02/CZ/banka3.html

Invalid read of size 4
 rc4_wordconv (arcfour.c:530)
 RC4_Encrypt (arcfour.c:608)
 RC4_Encrypt (loader.c:515)
 NSC_EncryptUpdate (pkcs11c.c:781)
 PK11_CipherOp (pk11cxt.c:730)
 ssl3_CompressMACEncryptRecord (ssl3con.c:1851)
 ssl3_SendRecord (ssl3con.c:1968)
 ssl3_SendApplicationData (ssl3con.c:2084)
 ssl_SecureSend (sslsecur.c:1119)
 ssl_SecureWrite (sslsecur.c:1128)
 ssl_Write (sslsock.c:1413)
 nsSSLThread::Run() (nsSSLThread.cpp:783)
Address 0xC91FAB8 is 504 bytes inside a block of size 507 alloc'd
 malloc (vg_replace_malloc.c:149)
 malloc (nsTraceMalloc.c:1478)
 PR_Malloc (prmem.c:467)
 NS_Alloc_P (nsMemoryImpl.cpp:281)
 nsMemory::Alloc(unsigned) (nsMemory.h:68)
 nsSSLSocketThreadData::ensure_buffer_size(int) (nsNSSIOLayer.cpp:165)
 nsSSLThread::requestWrite(nsNSSSocketInfo*, void const*, int) (nsSSLThread.cpp:663)
 nsSSLIOLayerWrite(PRFileDesc*, void const*, int) (nsNSSIOLayer.cpp:1178)
 PR_Write (priometh.c:146)
 nsSocketOutputStream::Write(char const*, unsigned, unsigned*) (nsSocketTransport2.cpp:540)
 nsHttpConnection::OnReadSegment(char const*, unsigned, unsigned*) (nsHttpConnection.cpp:523)
 nsHttpTransaction::ReadRequestSegment(nsIInputStream*, void*, char const*, unsigned, unsigned, unsign
ed*) (nsHttpTransaction.cpp:411)

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Our RC4 code always does word-aligned word reads.  
Every word it reads contains at least one byte of data from the 
input buffer, but it may contain 1-3 bytes of data past the beginning
or the end of the buffer.  It masks and discards those extra bytes.
It never results in a segv, because every word contains at least one
byte from the buffer, so the word is never entirely past the beginning
or end of the buffer.  

We do this for performance.
Lots of software evaluation tools stumble on this.  
We're not going to hurt our performance to satisfy those tools.

Comment 6

[Paul Menzel]

Is there a Valgrind guide for Mozilla products where this common Valgrind error could be documented.

Maybe some Valgrind knowledgeable person could come up with a suppression file (`man valgrind`), so this message will get suppressed.

Comment 7

[Paul Menzel]

(In reply to Nelson Bolyard (seldom reads bugmail) from comment #1)

[…]

> We do this for performance.
> Lots of software evaluation tools stumble on this.  
> We're not going to hurt our performance to satisfy those tools.

Do you know of any sample code which could be used for benchmarking? Maybe compilers are smart enough nowadays to optimize that part of the code themselves?

Comment 8

[Wan-Teh Chang]

This invalid read is regularly reported by NSS users who run valgrind
on their code.  Too many people have wasted time on this.  I wrote a
patch last night.  It is being reviewed at
https://codereview.chromium.org/12226071/.

Comment 9

[Wan-Teh Chang]

Attachment: Fix the invalid read and write in rc4_wordconv

This patch has been reviewed by Ryan Sleevi at
https://codereview.chromium.org/12226071/

Comment 10

[Wan-Teh Chang]

Attachment: [Optional] Use rc4_wordconv for x86 on all operating systems

arcfour.c has several implementations of RC4. This patch causes
NSS to use rc4_wordconv for x86 on all operating systems rather
than just Unix/Linux/Mac OS X, where the macro 'i386' is defined.

The intention of this change is to exercise the new code in
rc4_wordconv as much as possible, in particular on Windows, in
the hope that bugs will be exposed more quickly.

(I also removed the test for the obsolete OS IRIX.)

This patch has also been reviewed by Ryan Sleevi at
https://codereview.chromium.org/12226071/

Comment 11

[Wan-Teh Chang]

Both patches checked in on the NSS trunk (NSS 3.14.4).

Checking in arcfour.c;
/cvsroot/mozilla/security/nss/lib/freebl/arcfour.c,v  <--  arcfour.c
new revision: 1.24; previous revision: 1.23
done

Comment 12

[Wan-Teh Chang]

It turns out my patch still missed a read beyond the end of the buffer.
AddressSanitizer calls that error a "heap-buffer-overflow".

I have written a patch to fix it: https://codereview.chromium.org/15027002/

Comment 13

[Wan-Teh Chang]

Attachment: Fix the remaining invalid read in rc4_wordconv

This patch was reviewed in https://codereview.chromium.org/15027002/

Pushed to the NSS hg repository:
https://hg.mozilla.org/projects/nss/rev/340f03d45de9