From 6630f05d0a6d631c9ed2edfef9951df892287794 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Sun, 6 Nov 2005 21:54:40 +0000 Subject: [PATCH] Fix various buffer overruns in fmt_scan.c; the bulk of this is passing buffer length through to decode_rfc2047() and having that function do sufficient bookkeeping to avoid running off the end of the buffer. --- ChangeLog | 4 +++ h/prototypes.h | 2 +- sbr/fmt_rfc2047.c | 73 ++++++++++++++++++++++++++++++++++++++++------------- sbr/fmt_scan.c | 8 ++++-- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6d2553e..ca22c28 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2005-11-06 Peter Maydell + * sbr/fmt_rfc2047.c, sbr/fmt_scan.c, h/prototypes.h: fix various + possible overruns of the buffers in fmt_scan() which would cause + crashes if scan was run with '-width 16536' or similar. + * uip/popsbr.c: fix compile error which only showed up if nmh was configured with --enable-apop. diff --git a/h/prototypes.h b/h/prototypes.h index edddc47..21c15a7 100644 --- a/h/prototypes.h +++ b/h/prototypes.h @@ -46,7 +46,7 @@ char *copy (char *, char *); char **copyip (char **, char **, int); void cpydata (int, int, char *, char *); void cpydgst (int, int, char *, char *); -int decode_rfc2047 (char *, char *); +int decode_rfc2047 (char *, char *, size_t); void discard (FILE *); int done (int); int ext_hook(char *, char *, char *); diff --git a/sbr/fmt_rfc2047.c b/sbr/fmt_rfc2047.c index a87fc0e..9f5b26d 100644 --- a/sbr/fmt_rfc2047.c +++ b/sbr/fmt_rfc2047.c @@ -55,8 +55,11 @@ unqp (unsigned char byte1, unsigned char byte2) * Decode the string as a RFC-2047 header field */ +/* Add character to the destination buffer, and bomb out if it fills up */ +#define ADDCHR(C) do { *q++ = (C); dstlen--; if (!dstlen) goto buffull; } while (0) + int -decode_rfc2047 (char *str, char *dst) +decode_rfc2047 (char *str, char *dst, size_t dstlen) { char *p, *q, *pp; char *startofmime, *endofmime; @@ -68,8 +71,9 @@ decode_rfc2047 (char *str, char *dst) #ifdef HAVE_ICONV int use_iconv = 0; /* are we converting encoding with iconv? */ iconv_t cd; - int fromutf8; - char *saveq, *convbuf; + int fromutf8 = 0; + char *saveq, *convbuf = NULL; + size_t savedstlen; #endif if (!str) @@ -96,7 +100,7 @@ decode_rfc2047 (char *str, char *dst) * last iteration, then add it first. */ if (equals_pending) { - *q++ = '='; + ADDCHR('='); equals_pending = 0; between_encodings = 0; /* we have added non-whitespace text */ } @@ -107,7 +111,7 @@ decode_rfc2047 (char *str, char *dst) whitespace++; else between_encodings = 0; /* we have added non-whitespace text */ - *q++ = *p; + ADDCHR(*p); continue; } @@ -185,15 +189,25 @@ decode_rfc2047 (char *str, char *dst) * We will roll back the buffer the number of whitespace * characters we've seen since last encoded word. */ - if (between_encodings) + if (between_encodings) { q -= whitespace; + dstlen += whitespace; + } #ifdef HAVE_ICONV if (use_iconv) { - saveq = q; + saveq = q; + savedstlen = dstlen; if (!(q = convbuf = (char *)malloc(endofmime - startofmime))) continue; } +/* ADDCHR2 is for adding characters when q is or might be convbuf: + * in this case on buffer-full we want to run iconv before returning. + * I apologise for the dreadful name. + */ +#define ADDCHR2(C) do { *q++ = (C); dstlen--; if (!dstlen) goto iconvbuffull; } while (0) +#else +#define ADDCHR2(C) ADDCHR(C) #endif /* Now decode the text */ @@ -207,9 +221,9 @@ decode_rfc2047 (char *str, char *dst) *q++ = c; pp += 2; } else if (*pp == '_') { - *q++ = ' '; + ADDCHR2(' '); } else { - *q++ = *pp; + ADDCHR2(*pp); } } } else { @@ -231,7 +245,7 @@ decode_rfc2047 (char *str, char *dst) pp++; } if (pp < endofmime && c1 != -1 && c2 != -1) { - *q++ = (c1 << 2) | (c2 >> 4); + ADDCHR2((c1 << 2) | (c2 >> 4)); pp++; } /* 4 + 4 bits */ @@ -240,7 +254,7 @@ decode_rfc2047 (char *str, char *dst) pp++; } if (pp < endofmime && c2 != -1 && c3 != -1) { - *q++ = ((c2 & 0xF) << 4) | (c3 >> 2); + ADDCHR2(((c2 & 0xF) << 4) | (c3 >> 2)); pp++; } /* 2 + 6 bits */ @@ -249,37 +263,54 @@ decode_rfc2047 (char *str, char *dst) pp++; } if (pp < endofmime && c3 != -1 && c4 != -1) { - *q++ = ((c3 & 0x3) << 6) | (c4); + ADDCHR2(((c3 & 0x3) << 6) | (c4)); pp++; } } } #ifdef HAVE_ICONV + iconvbuffull: + /* NB that the string at convbuf is not necessarily NUL terminated here: + * q points to the first byte after the valid part. + */ /* Convert to native character set */ if (use_iconv) { size_t inbytes = q - convbuf; - size_t outbytes = BUFSIZ; ICONV_CONST char *start = convbuf; while (inbytes) { - if (iconv(cd, &start, &inbytes, &saveq, &outbytes) == + if (iconv(cd, &start, &inbytes, &saveq, &savedstlen) == (size_t)-1) { if (errno != EILSEQ) break; /* character couldn't be converted. we output a `?' * and try to carry on which won't work if * either encoding was stateful */ - iconv (cd, 0, 0, &saveq, &outbytes); + iconv (cd, 0, 0, &saveq, &savedstlen); + if (!savedstlen) + break; *saveq++ = '?'; - /* skip to next input character */ + savedstlen--; + if (!savedstlen) + break; + /* skip to next input character */ if (fromutf8) { - for (start++;(*start & 192) == 128;start++) + for (start++;(start < q) && ((*start & 192) == 128);start++) inbytes--; } else start++, inbytes--; + if (start >= q) + break; } } q = saveq; + /* Stop now if (1) we hit the end of the buffer trying to do + * MIME decoding and have just iconv-converted a partial string + * or (2) our iconv-conversion hit the end of the buffer. + */ + if (!dstlen || !savedstlen) + goto buffull; + dstlen = savedstlen; free(convbuf); } #endif @@ -301,8 +332,14 @@ decode_rfc2047 (char *str, char *dst) /* If an equals was pending at end of string, add it now. */ if (equals_pending) - *q++ = '='; + ADDCHR('='); *q = '\0'; return encoding_found; + + buffull: + /* q is currently just off the end of the buffer, so rewind to NUL terminate */ + q--; + *q = '\0'; + return encoding_found; } diff --git a/sbr/fmt_scan.c b/sbr/fmt_scan.c index 7558eca..8f81e77 100644 --- a/sbr/fmt_scan.c +++ b/sbr/fmt_scan.c @@ -442,14 +442,14 @@ fmt_scan (struct format *format, char *scanl, int width, int *dat) break; case FT_LS_DECODECOMP: - if (decode_rfc2047(fmt->f_comp->c_text, buffer2)) + if (decode_rfc2047(fmt->f_comp->c_text, buffer2, sizeof(buffer2))) str = buffer2; else str = fmt->f_comp->c_text; break; case FT_LS_DECODE: - if (str && decode_rfc2047(str, buffer2)) + if (str && decode_rfc2047(str, buffer2, sizeof(buffer2))) str = buffer2; break; @@ -458,6 +458,7 @@ fmt_scan (struct format *format, char *scanl, int width, int *dat) char *xp; strncpy(buffer, str, sizeof(buffer)); + buffer[sizeof(buffer)-1] = '\0'; str = buffer; while (isspace(*str)) str++; @@ -646,6 +647,7 @@ fmt_scan (struct format *format, char *scanl, int width, int *dat) if ((str = mn->m_pers) == NULL) { if ((str = mn->m_note)) { strncpy (buffer, str, sizeof(buffer)); + buffer[sizeof(buffer)-1] = '\0'; str = buffer; if (*str == '(') str++; @@ -690,6 +692,8 @@ fmt_scan (struct format *format, char *scanl, int width, int *dat) if (str) { int m; strncpy(buffer, str, sizeof(buffer)); + /* strncpy doesn't NUL-terminate if it fills the buffer */ + buffer[sizeof(buffer)-1] = '\0'; str = buffer; /* we will parse from buffer to buffer2 */ -- 1.7.10.4