Fix the remaining heap-buffer-overflow (read) error in arcfour.c.

nss/lib/freebl/arcfour.c

 /* arcfour.c - the arc four algorithm.
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifdef FREEBL_NO_DEPEND
 #include "stubs.h"
 #endif
 
@@ skipping 354 matching lines @@
         unsigned int inOffset = (PRUword)input % WORDSIZE;
         unsigned int outOffset = (PRUword)output % WORDSIZE;
         register WORD streamWord;
         register const WORD *pInWord;
         register WORD *pOutWord;
         register WORD inWord, nextInWord;
         PRUint8 t;
         register Stype tmpSi, tmpSj;
         register PRUint8 tmpi = cx->i;
         register PRUint8 tmpj = cx->j;
-        unsigned int byteCount;

[wtc, 2013/05/07 18:00:50]

I moved this variable declaration to the lexical scope where
this variable is used.

         unsigned int bufShift, invBufShift;
         unsigned int i;
         const unsigned char *finalIn;
         unsigned char *finalOut;
 
         PORT_Assert(maxOutputLen >= inputLen);
         if (maxOutputLen < inputLen) {
                 PORT_SetError(SEC_ERROR_OUTPUT_LEN);
                 return SECFailure;
         }
         if (inputLen < 2*WORDSIZE) {
                 /* Ignore word conversion, do byte-at-a-time */
                 return rc4_no_opt(cx, output, outputLen, maxOutputLen, input, inputLen);
         }
         *outputLen = inputLen;
         pInWord = (const WORD *)(input - inOffset);
         pOutWord = (WORD *)(output - outOffset);
-        if (inOffset < outOffset) {
+        if (inOffset <= outOffset) {

[wtc, 2013/05/07 18:00:50]

How to review this change.

1. Verify that bufShift and invBufShift are not used if
inOffset == outOffset. So, the values of bufShift and
invBufShift don't really matter in that case.

2. If inOffset == outOffset, one of bufShift and invBufShift
is 0 and the other is 8*WORDSIZE.

3. In the original code, invBufShift is 0 if inOffset ==
outOffset. In the new code, bufShift is 0 in that case
because it makes the code below look a little nicer.

                 bufShift = 8*(outOffset - inOffset);
                 invBufShift = 8*WORDSIZE - bufShift;
         } else {
                 invBufShift = 8*(inOffset - outOffset);
                 bufShift = 8*WORDSIZE - invBufShift;
         }
         /*****************************************************************/
         /* Step 1:                                                       */
         /* If the first output word is partial, consume the bytes in the */
         /* first partial output word by loading one or two words of      */
         /* input and shifting them accordingly.  Otherwise, just load    */
         /* in the first word of input.  At the end of this block, at     */
         /* least one partial word of input should ALWAYS be loaded.      */
         /*****************************************************************/
         if (outOffset) {
-                byteCount = WORDSIZE - outOffset; 
+                unsigned int byteCount = WORDSIZE - outOffset; 
                 for (i = 0; i < byteCount; i++) {
                         ARCFOUR_NEXT_BYTE();
                         output[i] = cx->S[t] ^ input[i];
                 }
                 /* Consumed byteCount bytes of input */
                 inputLen -= byteCount;
                 pInWord++;
 
                 /* move to next word of output */
                 pOutWord++;
@@ skipping 39 matching lines @@
                         }
                         pInWord++;
                 } else {
                         /* Input is word-aligned.  The first word load of input 
                          * will produce a full word of input bytes, so nothing
                          * needs to be loaded here.
                          */
                         inWord = 0;
                 }
         }
-        /* Output buffer is aligned, inOffset is now measured relative to
-         * outOffset (and not a word boundary).
-         */
-        inOffset = (inOffset + WORDSIZE - outOffset) % WORDSIZE;

[wtc, 2013/05/07 18:00:50]

This redefinition of inOffset is very confusing. It is an
attempt to reuse a local variable. An optimizing compiler
can do this for us.

I moved one local variable (byteCount) into a smaller
lexical scope to help the compiler reuse it for something
else, such as the new preloadedByteCount variable.

         /*****************************************************************/
         /* Step 2: main loop                                             */
         /* At this point the output buffer is word-aligned.  Any unused  */
         /* bytes from above will be in inWord (shifted correctly).  If   */
         /* the input buffer is unaligned relative to the output buffer,  */
         /* shifting has to be done.                                      */
         /*****************************************************************/
-        if (inOffset) {
-                for (; inputLen >= WORDSIZE; inputLen -= WORDSIZE) {
+        if (bufShift) {

[wtc, 2013/05/07 18:00:50]

With the new values of bufShift and invBufShift above, if
bufShift is nonzero, it means inOffset != outOffset, which
is the condition described in the comment above.

+                /* preloadedByteCount is the number of input bytes pre-loaded
+                 * in inWord.
+                 */
+                unsigned int preloadedByteCount = bufShift/8;
+                for (; inputLen >= preloadedByteCount + WORDSIZE;
+                     inputLen -= WORDSIZE) {

[wtc, 2013/05/07 18:00:50]

How to understand this change:

1. At the beginning of each loop iteration, inWord already
contains some input bytes. Let preloadedByteCount be that
value.

We are about to read WORDSIZE bytes using *pInWord. So
we need to make sure we still have preloadedByteCount + WORDSIZE
bytes of input available to avoid reading beyond the end
of the buffer.

This is why the loop condition should be
    inputLen >= preloadedByteCount + WORDSIZE
rather than just
    inputLen >= WORDSIZE

2. Now, what is the value of preloadedByteCount? An easy
way to see that is line 483:

    inWord |= nextInWord RSH bufShift;

We need to shift nextInWord by bufShift bits to avoid
overwriting the preloaded input bytes in inWord. This
means preloadedByteCount is bufShift/8 bytes.

3. Finally, on line 495 we can compute the finalIn pointer
using the new preloadedByteCount variable now. To get the
remaining input, we need to go backward from pInWord by
preloadedByteCount bytes.

                         nextInWord = *pInWord++;
                         inWord |= nextInWord RSH bufShift;
                         nextInWord = nextInWord LSH invBufShift;
                         ARCFOUR_NEXT_WORD();
                         *pOutWord++ = inWord ^ streamWord;
                         inWord = nextInWord;
                 }
                 if (inputLen == 0) {
                         /* Nothing left to do. */
                         cx->i = tmpi;
                         cx->j = tmpj;
                         return SECSuccess;
                 }
-                finalIn = (const unsigned char *)pInWord - WORDSIZE + inOffset;
+                finalIn = (const unsigned char *)pInWord - preloadedByteCount;
         } else {
                 for (; inputLen >= WORDSIZE; inputLen -= WORDSIZE) {
                         inWord = *pInWord++;
                         ARCFOUR_NEXT_WORD();
                         *pOutWord++ = inWord ^ streamWord;
                 }
                 if (inputLen == 0) {
                         /* Nothing left to do. */
                         cx->i = tmpi;
                         cx->j = tmpj;
@@ skipping 58 matching lines @@
         /* Convert the byte-stream to a word-stream */
         return rc4_wordconv(cx, output, outputLen, maxOutputLen, input, inputLen);
 #else
         /* Operate on bytes, but unroll the main loop */
         return rc4_unrolled(cx, output, outputLen, maxOutputLen, input, inputLen);
 #endif
 }
 
 #undef CONVERT_TO_WORDS
 #undef USE_WORD