Last Comment Bug 341127 - Invalid read in rc4_wordconv
: Invalid read in rc4_wordconv
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: x86 Linux
: P1 normal (vote)
: 3.15
Assigned To: Wan-Teh Chang
:
Mentors:
https://klient1.ebanka.cz/ebts/versio...
: 387577 413252 451754 581284 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-11 00:36 PDT by Andrew Schultz
Modified: 2013-05-07 14:31 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix the invalid read and write in rc4_wordconv (8.74 KB, patch)
2013-02-11 18:21 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
[Optional] Use rc4_wordconv for x86 on all operating systems (681 bytes, patch)
2013-02-11 18:29 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Fix the remaining invalid read in rc4_wordconv (3.92 KB, patch)
2013-05-07 14:30 PDT, Wan-Teh Chang
wtc: checked‑in+
Details | Diff | Splinter Review

Description Andrew Schultz 2006-06-11 00:36:00 PDT
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) 2006-06-11 00:54:05 PDT
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 2 Nelson Bolyard (seldom reads bugmail) 2007-07-10 21:01:14 PDT
*** Bug 387577 has been marked as a duplicate of this bug. ***
Comment 3 Nelson Bolyard (seldom reads bugmail) 2008-01-20 18:55:03 PST
*** Bug 413252 has been marked as a duplicate of this bug. ***
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-08-22 12:39:06 PDT
*** Bug 451754 has been marked as a duplicate of this bug. ***
Comment 5 Wan-Teh Chang 2011-02-02 17:10:33 PST
*** Bug 581284 has been marked as a duplicate of this bug. ***
Comment 6 Paul Menzel 2013-01-01 08:53:25 PST
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 2013-01-01 08:55:09 PST
(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 2013-02-08 20:41:40 PST
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 2013-02-11 18:21:04 PST
Created attachment 712745 [details] [diff] [review]
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 2013-02-11 18:29:36 PST
Created attachment 712747 [details] [diff] [review]
[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 2013-02-15 11:10:49 PST
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 2013-05-07 11:07:49 PDT
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 2013-05-07 14:30:22 PDT
Created attachment 746636 [details] [diff] [review]
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

Note You need to log in before you can comment on or make changes to this bug.