Closed Bug 341127 Opened 14 years ago Closed 7 years ago

Invalid read in rc4_wordconv

Categories

(NSS :: Libraries, defect, P1)

3.11
x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: wtc)

References

()

Details

Attachments

(3 files)

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)
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Duplicate of this bug: 387577
Duplicate of this bug: 413252
Duplicate of this bug: 451754
Duplicate of this bug: 581284
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.
(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?
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/.
Assignee: nobody → wtc
Severity: major → normal
Status: RESOLVED → REOPENED
Priority: -- → P1
Resolution: WONTFIX → ---
Target Milestone: --- → 3.14.4
This patch has been reviewed by Ryan Sleevi at
https://codereview.chromium.org/12226071/
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/
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
Status: REOPENED → RESOLVED
Closed: 14 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: 3.14.4 → 3.15
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/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.