[gmime-devel] Bug in base64 decoding - corrupted last bytes of data

[Damian Pietras <daper daper net>]

The code that does base64 decoding
(g_mime_encoding_base64_decode_step()) has a bug that leads to
corruption of last bytes of data in some situations.

If the input buffer doesn't contain a full 24-bit group at the end but
contains one of two padding characters ('=') the last bytes become
corrupted. Let's say that our input with base64 data ends with:

2Q==

and some bytes in g_mime_encoding_base64_decode_step() were already
read, but the current input buffer (inbuf, inlen) has only part of
data without that last '=' character. If we're at the beginning of a
24-bit group (i == 0 in g_mime_encoding_base64_decode_step()) the
function will read data into the save buffer, but doesn't output anything
more (i == 3 after reading all data in this chunk). The data will be put
in output buffer in the next call to this function, but this loop will
now eat our data in the output buffer:

while (inptr > inbuf && i) {
        inptr--;
        if (gmime_base64_rank[*inptr] != 0xff) {
                if (*inptr == '=' && outptr > outbuf)
                        outptr--;
                i--;
        }
}


There is no check that we have data left in the save buffer that needs
to be truncated, instead data in outptr are truncated by one byte. In
the next call when the single '=' character is read, data from
state->save are appended and another byte is truncated.

I've attached a patch to fix that. It changes arguments for
g_mime_encoding_base64_decode_step() to pass the whole GMimeEncoding
structure instead of adding another parameter because I needed another
variable there. The may be a simpler solution.

-- 
Damian Pietras

http://www.linuxprogrammingblog.com

Index: gmime/gmime-encodings.c
===================================================================
--- gmime/gmime-encodings.c	(revision 5727)
+++ gmime/gmime-encodings.c	(working copy)
@@ -223,6 +223,7 @@
 	}
 	
 	state->save = 0;
+	state->npads = 0;
 }
 
 
@@ -289,7 +290,7 @@
 		if (state->encode)
 			return g_mime_encoding_base64_encode_step (inptr, inlen, outptr, &state->state, &state->save);
 		else
-			return g_mime_encoding_base64_decode_step (inptr, inlen, outptr, &state->state, &state->save);
+			return g_mime_encoding_base64_decode_step (inptr, inlen, outptr, state);
 	case GMIME_CONTENT_ENCODING_QUOTEDPRINTABLE:
 		if (state->encode)
 			return g_mime_encoding_quoted_encode_step (inptr, inlen, outptr, &state->state, &state->save);
@@ -330,7 +331,7 @@
 		if (state->encode)
 			return g_mime_encoding_base64_encode_close (inptr, inlen, outptr, &state->state, &state->save);
 		else
-			return g_mime_encoding_base64_decode_step (inptr, inlen, outptr, &state->state, &state->save);
+			return g_mime_encoding_base64_decode_step (inptr, inlen, outptr, state);
 	case GMIME_CONTENT_ENCODING_QUOTEDPRINTABLE:
 		if (state->encode)
 			return g_mime_encoding_quoted_encode_close (inptr, inlen, outptr, &state->state, &state->save);
@@ -490,8 +491,7 @@
  * @inbuf: input buffer
  * @inlen: input buffer length
  * @outbuf: output buffer
- * @state: holds the number of bits that are stored in @save
- * @save: leftover bits that have not yet been decoded
+ * @state: holds decoding state information
  *
  * Decodes a chunk of base64 encoded data.
  *
@@ -499,7 +499,7 @@
  * @outbuf).
  **/
 size_t
-g_mime_encoding_base64_decode_step (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, int *state, guint32 *save)
+g_mime_encoding_base64_decode_step (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, GMimeEncoding *state)
 {
 	const register unsigned char *inptr;
 	register unsigned char *outptr;
@@ -512,8 +512,8 @@
 	outptr = outbuf;
 	
 	/* convert 4 base64 bytes to 3 normal bytes */
-	saved = *save;
-	i = *state;
+	saved = state->save;
+	i = state->state;
 	inptr = inbuf;
 	while (inptr < inend) {
 		c = gmime_base64_rank[*inptr++];
@@ -525,12 +525,17 @@
 				*outptr++ = saved >> 8;
 				*outptr++ = saved;
 				i = 0;
+
+				if (state->npads) {
+					outptr -= state->npads;
+					state->npads = 0;
+				}
 			}
 		}
 	}
 	
-	*save = saved;
-	*state = i;
+	state->save = saved;
+	state->state = i;
 	
 	/* quick scan back for '=' on the end somewhere */
 	/* fortunately we can drop 1 output char for each trailing = (upto 2) */
@@ -538,8 +543,12 @@
 	while (inptr > inbuf && i) {
 		inptr--;
 		if (gmime_base64_rank[*inptr] != 0xff) {
-			if (*inptr == '=' && outptr > outbuf)
-				outptr--;
+			if (*inptr == '=' && outptr > outbuf) {
+				if (state->state == 0)
+					outptr--;
+				else if (state->npads < 2)
+					state->npads++;
+			}
 			i--;
 		}
 	}
Index: gmime/gmime-encodings.h
===================================================================
--- gmime/gmime-encodings.h	(revision 5727)
+++ gmime/gmime-encodings.h	(working copy)
@@ -128,6 +128,7 @@
  * @encode: %TRUE if encoding or %FALSE if decoding
  * @save: saved bytes from the previous step
  * @state: current encder/decoder state
+ * @npads: number of padding characters remaining to process.
  *
  * A context used for encoding or decoding data.
  **/
@@ -137,6 +138,7 @@
 	gboolean encode;
 	guint32 save;
 	int state;
+	int npads;
 } GMimeEncoding;
 
 
@@ -151,7 +153,7 @@
 
 
 /* do incremental base64 (de/en)coding */
-size_t g_mime_encoding_base64_decode_step (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, int *state, guint32 *save);
+size_t g_mime_encoding_base64_decode_step (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, GMimeEncoding *state);
 size_t g_mime_encoding_base64_encode_step (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, int *state, guint32 *save);
 size_t g_mime_encoding_base64_encode_close (const unsigned char *inbuf, size_t inlen, unsigned char *outbuf, int *state, guint32 *save);
 
Index: gmime/gmime-utils.c
===================================================================
--- gmime/gmime-utils.c	(revision 5727)
+++ gmime/gmime-utils.c	(working copy)
@@ -1738,11 +1738,10 @@
 	const char *charset;
 	size_t len, ninval;
 	char *charenc, *p;
-	guint32 save = 0;
 	ssize_t declen;
-	int state = 0;
 	iconv_t cd;
 	char *buf;
+	GMimeEncoding state;
 	
 	/* skip over the charset */
 	if (!(inptr = memchr (inptr, '?', inend - inptr)) || inptr[2] != '?')
@@ -1756,7 +1755,8 @@
 		inptr += 2;
 		len = (size_t) (inend - inptr);
 		decoded = g_alloca (len);
-		declen = g_mime_encoding_base64_decode_step (inptr, len, decoded, &state, &save);
+		g_mime_encoding_init_decode (&state, GMIME_CONTENT_ENCODING_BASE64);
+		declen = g_mime_encoding_base64_decode_step (inptr, len, decoded, &state);
 		
 		if (declen == -1) {
 			d(fprintf (stderr, "encountered broken 'Q' encoding\n"));