RT3757: base64 encoding bugs

Rewrite EVP_DecodeUpdate.

In particular: reject extra trailing padding, and padding in the middle
of the content. Don't limit line length. Add tests.

Previously, the behaviour was ill-defined, and depended on the position
of the padding within the input.

In addition, this appears to fix a possible two-byte oob read.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Dr Stephen Henson <steve@openssl.org>
(cherry picked from commit 3cdd1e94b1)
(cherry picked from commit 37faf11796)

CHANGES

@@ -4,6 +4,12 @@
 
  Changes between 1.0.1p and 1.0.1q [xx XXX xxxx]
 
+  *) Rewrite EVP_DecodeUpdate (base64 decoding) to fix several bugs.
+     This changes the decoding behaviour for some invalid messages,
+     though the change is mostly in the more lenient direction, and
+     legacy behaviour is preserved as much as possible.
+     [Emilia Käsper]
+
   *) In DSA_generate_parameters_ex, if the provided seed is too short,
      return an error
      [Rich Salz and Ismo Puustinen <ismo.puustinen@intel.com>]

crypto/evp/encode.c

@@ -103,6 +103,7 @@ abcdefghijklmnopqrstuvwxyz0123456789+/";
 #define B64_WS                  0xE0
 #define B64_ERROR               0xFF
 #define B64_NOT_BASE64(a)       (((a)|0x13) == 0xF3)
+#define B64_BASE64(a)           !B64_NOT_BASE64(a)
 
 static const unsigned char data_ascii2bin[128] = {
     0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
@@ -218,8 +219,9 @@ int EVP_EncodeBlock(unsigned char *t, const unsigned char *f, int dlen)
 
 void EVP_DecodeInit(EVP_ENCODE_CTX *ctx)
 {
-    ctx->length = 30;
+    /* Only ctx->num is used during decoding. */
     ctx->num = 0;
+    ctx->length = 0;
     ctx->line_num = 0;
     ctx->expect_nl = 0;
 }
@@ -228,139 +230,123 @@ void EVP_DecodeInit(EVP_ENCODE_CTX *ctx)
  * -1 for error
  *  0 for last line
  *  1 for full line
+ *
+ * Note: even though EVP_DecodeUpdate attempts to detect and report end of
+ * content, the context doesn't currently remember it and will accept more data
+ * in the next call. Therefore, the caller is responsible for checking and
+ * rejecting a 0 return value in the middle of content.
+ *
+ * Note: even though EVP_DecodeUpdate has historically tried to detect end of
+ * content based on line length, this has never worked properly. Therefore,
+ * we now return 0 when one of the following is true:
+ *   - Padding or B64_EOF was detected and the last block is complete.
+ *   - Input has zero-length.
+ * -1 is returned if:
+ *   - Invalid characters are detected.
+ *   - There is extra trailing padding, or data after padding.
+ *   - B64_EOF is detected after an incomplete base64 block.
  */
 int EVP_DecodeUpdate(EVP_ENCODE_CTX *ctx, unsigned char *out, int *outl,
                      const unsigned char *in, int inl)
 {
-    int seof = -1, eof = 0, rv = -1, ret = 0, i, v, tmp, n, ln, exp_nl;
+    int seof = 0, eof = 0, rv = -1, ret = 0, i, v, tmp, n, decoded_len;
     unsigned char *d;
 
     n = ctx->num;
     d = ctx->enc_data;
-    ln = ctx->line_num;
-    exp_nl = ctx->expect_nl;
 
-    /* last line of input. */
+    if (n > 0 && d[n - 1] == '=') {
-    if ((inl == 0) || ((n == 0) && (conv_ascii2bin(in[0]) == B64_EOF))) {
+        eof++;
+        if (n > 1 && d[n - 2] == '=')
+            eof++;
+    }
+
+     /* Legacy behaviour: an empty input chunk signals end of input. */
+    if (inl == 0) {
         rv = 0;
         goto end;
     }
 
-    /* We parse the input data */
     for (i = 0; i < inl; i++) {
-        /* If the current line is > 80 characters, scream alot */
-        if (ln >= 80) {
-            rv = -1;
-            goto end;
-        }
-
-        /* Get char and put it into the buffer */
         tmp = *(in++);
         v = conv_ascii2bin(tmp);
-        /* only save the good data :-) */
+        if (v == B64_ERROR) {
-        if (!B64_NOT_BASE64(v)) {
-            OPENSSL_assert(n < (int)sizeof(ctx->enc_data));
-            d[n++] = tmp;
-            ln++;
-        } else if (v == B64_ERROR) {
             rv = -1;
             goto end;
         }
 
-        /*
-         * have we seen a '=' which is 'definitly' the last input line.  seof
-         * will point to the character that holds it. and eof will hold how
-         * many characters to chop off.
-         */
         if (tmp == '=') {
-            if (seof == -1)
-                seof = n;
             eof++;
+        } else if (eof > 0 && B64_BASE64(v)) {
+            /* More data after padding. */
+            rv = -1;
+            goto end;
         }
 
-        if (v == B64_CR) {
+        if (eof > 2) {
-            ln = 0;
+            rv = -1;
-            if (exp_nl)
+            goto end;
-                continue;
         }
 
-        /* eoln */
+        if (v == B64_EOF) {
-        if (v == B64_EOLN) {
+            seof = 1;
-            ln = 0;
+            goto tail;
-            if (exp_nl) {
-                exp_nl = 0;
-                continue;
-            }
-        }
-        exp_nl = 0;
-
-        /*
-         * If we are at the end of input and it looks like a line, process
-         * it.
-         */
-        if (((i + 1) == inl) && (((n & 3) == 0) || eof)) {
-            v = B64_EOF;
-            /*
-             * In case things were given us in really small records (so two
-             * '=' were given in separate updates), eof may contain the
-             * incorrect number of ending bytes to skip, so let's redo the
-             * count
-             */
-            eof = 0;
-            if (d[n - 1] == '=')
-                eof++;
-            if (d[n - 2] == '=')
-                eof++;
-            /* There will never be more than two '=' */
         }
 
-        if ((v == B64_EOF && (n & 3) == 0) || (n >= 64)) {
+        /* Only save valid base64 characters. */
-            /*
+        if (B64_BASE64(v)) {
-             * This is needed to work correctly on 64 byte input lines.  We
+            if (n >= 64) {
-             * process the line and then need to accept the '\n'
+                /*
-             */
+                 * We increment n once per loop, and empty the buffer as soon as
-            if ((v != B64_EOF) && (n >= 64))
+                 * we reach 64 characters, so this can only happen if someone's
-                exp_nl = 1;
+                 * manually messed with the ctx. Refuse to write any more data.
-            if (n > 0) {
+                 */
-                v = EVP_DecodeBlock(out, d, n);
+                rv = -1;
-                n = 0;
-                if (v < 0) {
-                    rv = 0;
-                    goto end;
-                }
-                if (eof > v) {
-                    rv = -1;
-                    goto end;
-                }
-                ret += (v - eof);
-            } else {
-                eof = 1;
-                v = 0;
-            }
-
-            /*
-             * This is the case where we have had a short but valid input
-             * line
-             */
-            if ((v < ctx->length) && eof) {
-                rv = 0;
-                goto end;
-            } else
-                ctx->length = v;
-
-            if (seof >= 0) {
-                rv = 0;
                 goto end;
             }
-            out += v;
+            OPENSSL_assert(n < (int)sizeof(ctx->enc_data));
+            d[n++] = tmp;
+        }
+
+        if (n == 64) {
+            decoded_len = EVP_DecodeBlock(out, d, n);
+            n = 0;
+            if (decoded_len < 0 || eof > decoded_len) {
+                rv = -1;
+                goto end;
+            }
+            ret += decoded_len - eof;
+            out += decoded_len - eof;
         }
     }
-    rv = 1;
+
- end:
+    /*
+     * Legacy behaviour: if the current line is a full base64-block (i.e., has
+     * 0 mod 4 base64 characters), it is processed immediately. We keep this
+     * behaviour as applications may not be calling EVP_DecodeFinal properly.
+     */
+tail:
+    if (n > 0) {
+        if ((n & 3) == 0) {
+        decoded_len = EVP_DecodeBlock(out, d, n);
+        n = 0;
+        if (decoded_len < 0 || eof > decoded_len) {
+            rv = -1;
+            goto end;
+        }
+        ret += (decoded_len - eof);
+        } else if (seof) {
+            /* EOF in the middle of a base64 block. */
+            rv = -1;
+            goto end;
+        }
+    }
+
+    rv = seof || (n == 0 && eof) ? 0 : 1;
+end:
+    /* Legacy behaviour. This should probably rather be zeroed on error. */
     *outl = ret;
     ctx->num = n;
-    ctx->line_num = ln;
-    ctx->expect_nl = exp_nl;
     return (rv);
 }