Invalid read in rc4_wordconv

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: Andrew Schultz, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → WONTFIX
Duplicate of this bug: 387577
Duplicate of this bug: 413252
Duplicate of this bug: 451754
(Assignee)

Updated

6 years ago
Duplicate of this bug: 581284

Comment 6

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

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

Comment 8

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

Comment 9

4 years ago
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/
(Assignee)

Comment 10

4 years ago
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/
(Assignee)

Comment 11

4 years ago
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
Last Resolved: 11 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: 3.14.4 → 3.15
(Assignee)

Comment 12

4 years ago
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 → ---
(Assignee)

Comment 13

4 years ago
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
Attachment #746636 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.