From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10487 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: [PATCH] [RFC] fix integer overflows and uncaught EOVERFLOW in printf Date: Sat, 17 Sep 2016 01:17:40 -0400 Message-ID: <20160917051740.GX15995@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="itqfrb9Qq3wY07cp" X-Trace: blaine.gmane.org 1474089484 5761 195.159.176.226 (17 Sep 2016 05:18:04 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 17 Sep 2016 05:18:04 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Alexander Cherepanov To: musl@lists.openwall.com Original-X-From: musl-return-10500-gllmg-musl=m.gmane.org@lists.openwall.com Sat Sep 17 07:18:00 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1bl80h-0000Xz-W0 for gllmg-musl@m.gmane.org; Sat, 17 Sep 2016 07:17:56 +0200 Original-Received: (qmail 15844 invoked by uid 550); 17 Sep 2016 05:17:55 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 15812 invoked from network); 17 Sep 2016 05:17:54 -0000 Content-Disposition: inline Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:10487 Archived-At: --itqfrb9Qq3wY07cp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline This patch expands on issues raised by Alexander Cherepanov earlier this year with some concrete fixes. I'm not terribly concerned about what happens when the format string itself contains out-of-range numbers (although it would be nice to handle that right), but it is important to handle correctly cases where the data being printed (e.g. long strings) would cause the total length to overflow INT_MAX, or where it would cause internal overflows. This patch: 1. Returns with an error as soon as EOVERFLOW conditions are detected, rather than continuing to produce output. 2. Catches overflows where the format string itself is longer than INT_MAX (only possible on 64-bit archs). 3. Catches %s argument strings longer than INT_MAX (likewise). 4. Catches cases where adding a prefix length to a precision would overflow. 5. Correctly handles width specifiers that overflow (always an error) and precision specifiers that overflow (error for everything but strings; for strings they're equivalent to no precision/no limit). Checking wide strings and floating point conversions for overflows remains to be done. As discussed before, none of these overflows are presently dangerous, but they do lead to mildly wrong results in the relevant corner cases and really should be fixed. Rich --itqfrb9Qq3wY07cp Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="printf-int-overflows.diff" diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c index e439a07..64e87e2 100644 --- a/src/stdio/vfprintf.c +++ b/src/stdio/vfprintf.c @@ -436,9 +436,11 @@ static int fmt_fp(FILE *f, long double y, int w, int p, int fl, int t) } static int getint(char **s) { - int i; - for (i=0; isdigit(**s); (*s)++) - i = 10*i + (**s-'0'); + unsigned i; + for (i=0; isdigit(**s); (*s)++) { + if (i > INT_MAX/10 || **s-'0' > INT_MAX-10*i) i = -1; + else i = 10*i + (**s-'0'); + } return i; } @@ -446,7 +448,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, { char *a, *z, *s=(char *)fmt; unsigned l10n=0, fl; - int w, p; + int w, p, xp; union arg arg; int argpos; unsigned st, ps; @@ -459,18 +461,19 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, char mb[4]; for (;;) { + /* This error is only specified for snprintf, but since it's + * unspecified for other forms, do the same. Stop immediately + * on overflow; otherwise %n could produce wrong results. */ + if (l > INT_MAX - cnt) goto overflow; + /* Update output count, end loop when fmt is exhausted */ - if (cnt >= 0) { - if (l > INT_MAX - cnt) { - errno = EOVERFLOW; - cnt = -1; - } else cnt += l; - } + cnt += l; if (!*s) break; /* Handle literal text and %% format specifiers */ for (a=s; *s && *s!='%'; s++); for (z=s; s[0]=='%' && s[1]=='%'; z++, s+=2); + if (z-a > INT_MAX-cnt) goto overflow; l = z-a; if (f) out(f, a, l); if (l) continue; @@ -498,9 +501,9 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, } else if (!l10n) { w = f ? va_arg(*ap, int) : 0; s++; - } else return -1; + } else goto inval; if (w<0) fl|=LEFT_ADJ, w=-w; - } else if ((w=getint(&s))<0) return -1; + } else if ((w=getint(&s))<0) goto overflow; /* Read precision */ if (*s=='.' && s[1]=='*') { @@ -511,24 +514,29 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, } else if (!l10n) { p = f ? va_arg(*ap, int) : 0; s+=2; - } else return -1; + } else goto inval; + xp = (p>=0); } else if (*s=='.') { s++; p = getint(&s); - } else p = -1; + xp = 1; + } else { + p = -1; + xp = 0; + } /* Format specifier state machine */ st=0; do { - if (OOB(*s)) return -1; + if (OOB(*s)) goto inval; ps=st; st=states[st]S(*s++); } while (st-1=0) return -1; + if (argpos>=0) goto inval; } else { if (argpos>=0) nl_type[argpos]=st, arg=nl_arg[argpos]; else if (f) pop_arg(&arg, st, ap); @@ -584,6 +592,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, case 'u': a = fmt_u(arg.i, z); } + if (xp && p<0) goto overflow; if (p>=0) fl &= ~ZERO_PAD; if (!arg.i && !p) { a=z; @@ -599,9 +608,9 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, if (1) a = strerror(errno); else case 's': a = arg.p ? arg.p : "(null)"; - z = memchr(a, 0, p); - if (!z) z=a+p; - else p=z-a; + z = a + strnlen(a, p<0 ? INT_MAX : p); + if (p<0 && *z) goto overflow; + p = z-a; fl &= ~ZERO_PAD; break; case 'C': @@ -623,12 +632,15 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, continue; case 'e': case 'f': case 'g': case 'a': case 'E': case 'F': case 'G': case 'A': + if (xp && p<0) goto overflow; l = fmt_fp(f, arg.f, w, p, fl, t); continue; } if (p < z-a) p = z-a; + if (p > INT_MAX-pl) goto overflow; if (w < pl+p) w = pl+p; + if (w > INT_MAX-cnt) goto overflow; pad(f, ' ', w, pl+p, fl); out(f, prefix, pl); @@ -646,8 +658,15 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, for (i=1; i<=NL_ARGMAX && nl_type[i]; i++) pop_arg(nl_arg+i, nl_type[i], ap); for (; i<=NL_ARGMAX && !nl_type[i]; i++); - if (i<=NL_ARGMAX) return -1; + if (i<=NL_ARGMAX) goto inval; return 1; + +inval: + errno = EINVAL; + return -1; +overflow: + errno = EOVERFLOW; + return -1; } int vfprintf(FILE *restrict f, const char *restrict fmt, va_list ap) --itqfrb9Qq3wY07cp--