Fix RC4 reads/writes outside array bounds.

Fix RC4 reads/writes outside array bounds.

Encrypt one byte at a time before and after the main loop
that encrypts one word at a time.

Use rc4_wordconv for x86 on all operating systems. Improve
error reporting. 

R=rsleevi@chromium.org
BUG=174140
TEST=none

[wtc, 2013-02-08 17:15:55]

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (left):

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:384: register WORD streamWord, mask;

|mask| is used to mask off the unwanted bytes in the
initial or final partial word. So it is gone now.

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:385: register WORD *pInWord,
*pOutWord;

Here I want to make |pInWord| a const pointer.

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:392: /* Step 1:                       
                               */

Step 1 is a complete rewrite. Therefore, do not review the
diffs. Instead, understand what the original code is trying
to do at a high-level, and then just read the new code.

In Step 1 I use memcpy() in two locations to make the code
look simpler. It can be replaced by a for loop that copies
one byte at a time, if necessary.

NOTE: Step 1 consists of nested if-else statements. I
believe it is possible to untangle them so they are not
nested, something like this:

    if (outOffset) {
        // Consume the first partial output word one byte
        // at a time. inputLen and pOutWord will be updated.
    }

    // Set up pInWord and inWord for Step 2. This requires
    // an if-else statement, possibly an if-else-if.

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:455: /* Step 2: main loop             
                               */

Step 2 is unchanged, except that at the end we need to figure
out where the remaining input starts. That address is stored
in |finalIn|.

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:492: /* Step 3:                       
                               */

Step 3 is a complete rewrite, but is simple.

[wtc, 2013-02-08 17:46:43]

Please review patch set 3. I introduced an artificial bug
and the NSS tests still passed, which led me to discover
this code was not being used on Windows. I changed that.

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/3001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:361: register WORD inWord, nextInWord;

Visual C++ does not allow me to take the address of inWord
(which I do in the memcpy calls) because inWord is a register
variable. I fixed this by removing 'register', but it does
show it may be worthwhile to use a different solution, which
will need to deal with byte order.

[Ryan Sleevi, 2013-02-08 19:32:47]

This LGTM as a functional equivalent, but one area I'm particularly weak on is
grokking the endian-ness concerns you were mentioning. Otherwise, this looks
like a functional conversion, but without the UMRs.

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:375:
PORT_SetError(SEC_ERROR_INVALID_ARGS);
aside: Why not SEC_ERROR_OUTPUT_LEN?

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:408: pInWord++;
aside: Why not pre-increment, since you're no longer de-referencing the returned
pointer?

[wtc, 2013-02-09 01:38:25]

Please review patch set 4. Thanks.

The memcpy calls are gone. The for loops that replaced the
memcpy calls have to handle both little-endian and big-endian.
This is the endianness concerns I mentioned. Using the for
loops allows me to declare inWord (used by the main loop)
with 'register' again.

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:375:
PORT_SetError(SEC_ERROR_INVALID_ARGS);

On 2013/02/08 19:32:47, Ryan Sleevi wrote:
> aside: Why not SEC_ERROR_OUTPUT_LEN?

Good idea. I will make this change.

In my debugging, I've found SEC_ERROR_INVALID_ARGS to be
a useless error code when returned by a low-level function
because by the time I get the error code, the context for
the invalid argument is not obvious at all.

https://codereview.chromium.org/12226071/diff/6001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:408: pInWord++;

On 2013/02/08 19:32:47, Ryan Sleevi wrote:
> aside: Why not pre-increment, since you're no longer de-referencing the
returned
> pointer?

Post-increment is more common in C.  (I guess this is why
C++ was not named ++C.)  Our Style Guide also allows
post-increment in this case:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preincrement_a...

[PaulePantert, 2013-02-09 09:23:49]

Thank you Wan-Teh for your effort.

Could you please split this patch up if it is possible? That makes review
easier.

Could you please be more elaborate on the commit message?

 1. Mention the Mozilla Bugzilla ticket URL.
 2. Explain the change as it looks rather big.
 3. Give benchmark data. The original implementation had performance reasons, if
I am not mistaken. Changing the implementation should give performance data that
it does not decrease performance.
 4. Give an argument why a Valgrind suppression file is not a good way to
circumvent this.

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:19: #if defined(SOLARIS) ||
defined(HPUX) || defined(NSS_X86) || \
Separate change?

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:120: unsigned int i;
Also separate change.

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:128: PORT_SetError(SEC_ERROR_BAD_KEY);
Is that change required for the rewrite?

If not could you factor that out too please.

[Ryan Sleevi, 2013-02-11 18:50:35]

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:366: unsigned int outOffset =
(PRUword)output % WORDSIZE;
I'm not sure I understand the motivation here for this change. Could you explain
more? You're minimally casting away signed-ness, and I'd like to understand why
this was needed.

[wtc, 2013-02-11 19:14:05]

pm.debian: thank you for your comments. This changelist is
mainly for code review. I can split it into independent parts
for ease of backporting.

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
File mozilla/security/nss/lib/freebl/arcfour.c (right):

https://codereview.chromium.org/12226071/diff/8001/mozilla/security/nss/lib/f...
mozilla/security/nss/lib/freebl/arcfour.c:366: unsigned int outOffset =
(PRUword)output % WORDSIZE;

On 2013/02/11 18:50:35, Ryan Sleevi wrote:
> I'm not sure I understand the motivation here for this change. Could you
explain
> more? You're minimally casting away signed-ness, and I'd like to understand
why
> this was needed.

Certainly. This change is necessary to fix the signed/unsigned
compiler warning from Visual C++. The new code will compare
outOffset - inOffset with |i| on line 430. |i| is now an
unsigned int because it is compared with unsigned types in
other places. I don't want to fix this warning by adding a
cast, (int)i < outOffset - inOffset.

Here, ideally we should use the uintptr_t type from <stdint.h>
because we need to cast a pointer to an unsigned integer
type. The NSPR equivalent of uintptr_t is PRUword. (There
is a feature request to add PRIntPtr and PRUintPtr to make
the intention clearer.)

This MXR query shows PRUword is used this way in the PLArena
code: http://mxr.mozilla.org/nspr/search?string=%28PRUword%29

[Ryan Sleevi, 2013-02-11 19:17:53]

lgtm

[commit-bot: I haz the power, 2013-02-14 23:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/12226071/6002

[commit-bot: I haz the power, 2013-02-14 23:45:00]

Change committed as 182577