- (djm) [openbsd-compat/bsd-snprintf.c] Fix integer overflow in return

   value of snprintf replacement, similar to bugs in various libc
   implementations. This overflow is not exploitable in OpenSSH.
   While I'm fiddling with it, make it a fair bit faster by inlining the
   append-char routine; ok dtucker@

2 files changed, 107 insertions, 64 deletions

diff --git a/ChangeLog b/ChangeLog
index bad97ecf..7682c5c5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 20070114
  - (dtucker) [ssh-keygen.c] av -> argv to match earlier sync.
+ - (djm) [openbsd-compat/bsd-snprintf.c] Fix integer overflow in return
+   value of snprintf replacement, similar to bugs in various libc
+   implementations. This overflow is not exploitable in OpenSSH. 
+   While I'm fiddling with it, make it a fair bit faster by inlining the
+   append-char routine; ok dtucker@
 
 20070105
  - (djm) OpenBSD CVS Sync
@@ -2664,4 +2669,4 @@
    OpenServer 6 and add osr5bigcrypt support so when someone migrates
    passwords between UnixWare and OpenServer they will still work. OK dtucker@
 
-$Id: ChangeLog,v 1.4604 2007/01/13 23:26:25 dtucker Exp $
+$Id: ChangeLog,v 1.4605 2007/01/14 10:20:30 djm Exp $
diff --git a/openbsd-compat/bsd-snprintf.c b/openbsd-compat/bsd-snprintf.c
index 04651e1d..cefb1d1a 100644
--- a/openbsd-compat/bsd-snprintf.c
+++ b/openbsd-compat/bsd-snprintf.c
@@ -85,6 +85,11 @@
  *
  *    Move #endif to make sure VA_COPY, LDOUBLE, etc are defined even
  *    if the C library has some snprintf functions already.
+ *
+ * Damien Miller (djm@mindrot.org) Jan 2007
+ *    Fix integer overflows in return value.
+ *    Make formatting quite a bit faster by inlining dopr_outch()
+ *
  **************************************************************/
 
 #include "includes.h"
@@ -112,6 +117,8 @@
 #include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h>
+#include <errno.h>
 
 #ifdef HAVE_LONG_DOUBLE
 # define LDOUBLE long double
@@ -159,17 +166,27 @@
 # define MAX(p,q) (((p) >= (q)) ? (p) : (q))
 #endif
 
-static size_t dopr(char *buffer, size_t maxlen, const char *format, 
-		   va_list args_in);
-static void fmtstr(char *buffer, size_t *currlen, size_t maxlen,
-		    char *value, int flags, int min, int max);
-static void fmtint(char *buffer, size_t *currlen, size_t maxlen,
-		    LLONG value, int base, int min, int max, int flags);
-static void fmtfp(char *buffer, size_t *currlen, size_t maxlen,
-		   LDOUBLE fvalue, int min, int max, int flags);
-static void dopr_outch(char *buffer, size_t *currlen, size_t maxlen, char c);
-
-static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args_in)
+#define DOPR_OUTCH(buf, pos, buflen, thechar) \
+	do { \
+		if (++pos >= INT_MAX) { \
+			errno = ERANGE; \
+			return -1; \
+		if (pos < buflen) \
+			buf[pos] = thechar; \
+		} \
+	} while (0)
+
+static int dopr(char *buffer, size_t maxlen, const char *format, 
+    va_list args_in);
+static int fmtstr(char *buffer, size_t *currlen, size_t maxlen,
+    char *value, int flags, int min, int max);
+static int fmtint(char *buffer, size_t *currlen, size_t maxlen,
+    LLONG value, int base, int min, int max, int flags);
+static int fmtfp(char *buffer, size_t *currlen, size_t maxlen,
+    LDOUBLE fvalue, int min, int max, int flags);
+
+static int
+dopr(char *buffer, size_t maxlen, const char *format, va_list args_in)
 {
 	char ch;
 	LLONG value;
@@ -198,8 +215,8 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 		case DP_S_DEFAULT:
 			if (ch == '%') 
 				state = DP_S_FLAGS;
-			else 
-				dopr_outch (buffer, &currlen, maxlen, ch);
+			else
+				DOPR_OUTCH(buffer, currlen, maxlen, ch);
 			ch = *format++;
 			break;
 		case DP_S_FLAGS:
@@ -298,7 +315,9 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					value = va_arg (args, LLONG);
 				else
 					value = va_arg (args, int);
-				fmtint (buffer, &currlen, maxlen, value, 10, min, max, flags);
+				if (fmtint(buffer, &currlen, maxlen,
+				    value, 10, min, max, flags) == -1)
+					return -1;
 				break;
 			case 'o':
 				flags |= DP_F_UNSIGNED;
@@ -310,7 +329,9 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					value = (long)va_arg (args, unsigned LLONG);
 				else
 					value = (long)va_arg (args, unsigned int);
-				fmtint (buffer, &currlen, maxlen, value, 8, min, max, flags);
+				if (fmtint(buffer, &currlen, maxlen, value,
+				    8, min, max, flags) == -1)
+					return -1;
 				break;
 			case 'u':
 				flags |= DP_F_UNSIGNED;
@@ -322,7 +343,9 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					value = (LLONG)va_arg (args, unsigned LLONG);
 				else
 					value = (long)va_arg (args, unsigned int);
-				fmtint (buffer, &currlen, maxlen, value, 10, min, max, flags);
+				if (fmtint(buffer, &currlen, maxlen, value,
+				    10, min, max, flags) == -1)
+					return -1;
 				break;
 			case 'X':
 				flags |= DP_F_UP;
@@ -336,15 +359,18 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					value = (LLONG)va_arg (args, unsigned LLONG);
 				else
 					value = (long)va_arg (args, unsigned int);
-				fmtint (buffer, &currlen, maxlen, value, 16, min, max, flags);
+				if (fmtint(buffer, &currlen, maxlen, value,
+				    16, min, max, flags) == -1)
+					return -1;
 				break;
 			case 'f':
 				if (cflags == DP_C_LDOUBLE)
 					fvalue = va_arg (args, LDOUBLE);
 				else
 					fvalue = va_arg (args, double);
-				/* um, floating point? */
-				fmtfp (buffer, &currlen, maxlen, fvalue, min, max, flags);
+				if (fmtfp(buffer, &currlen, maxlen, fvalue,
+				    min, max, flags) == -1)
+					return -1;
 				break;
 			case 'E':
 				flags |= DP_F_UP;
@@ -353,7 +379,9 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					fvalue = va_arg (args, LDOUBLE);
 				else
 					fvalue = va_arg (args, double);
-				fmtfp (buffer, &currlen, maxlen, fvalue, min, max, flags);
+				if (fmtfp(buffer, &currlen, maxlen, fvalue,
+				    min, max, flags) == -1)
+					return -1;
 				break;
 			case 'G':
 				flags |= DP_F_UP;
@@ -362,10 +390,13 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					fvalue = va_arg (args, LDOUBLE);
 				else
 					fvalue = va_arg (args, double);
-				fmtfp (buffer, &currlen, maxlen, fvalue, min, max, flags);
+				if (fmtfp(buffer, &currlen, maxlen, fvalue,
+				    min, max, flags) == -1)
+					return -1;
 				break;
 			case 'c':
-				dopr_outch (buffer, &currlen, maxlen, va_arg (args, int));
+				DOPR_OUTCH(buffer, currlen, maxlen,
+				    va_arg (args, int));
 				break;
 			case 's':
 				strvalue = va_arg (args, char *);
@@ -374,11 +405,15 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 					max = strlen(strvalue);
 				}
 				if (min > 0 && max >= 0 && min > max) max = min;
-				fmtstr (buffer, &currlen, maxlen, strvalue, flags, min, max);
+				if (fmtstr(buffer, &currlen, maxlen,
+				    strvalue, flags, min, max) == -1)
+					return -1;
 				break;
 			case 'p':
 				strvalue = va_arg (args, void *);
-				fmtint (buffer, &currlen, maxlen, (long) strvalue, 16, min, max, flags);
+				if (fmtint(buffer, &currlen, maxlen,
+				    (long) strvalue, 16, min, max, flags) == -1)
+					return -1;
 				break;
 			case 'n':
 				if (cflags == DP_C_SHORT) {
@@ -400,7 +435,7 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 				}
 				break;
 			case '%':
-				dopr_outch (buffer, &currlen, maxlen, ch);
+				DOPR_OUTCH(buffer, currlen, maxlen, ch);
 				break;
 			case 'w':
 				/* not supported yet, treat as next char */
@@ -429,11 +464,12 @@ static size_t dopr(char *buffer, size_t maxlen, const char *format, va_list args
 			buffer[maxlen - 1] = '\0';
 	}
 	
-	return currlen;
+	return currlen < INT_MAX ? (int)currlen : -1;
 }
 
-static void fmtstr(char *buffer, size_t *currlen, size_t maxlen,
-		    char *value, int flags, int min, int max)
+static int
+fmtstr(char *buffer, size_t *currlen, size_t maxlen,
+    char *value, int flags, int min, int max)
 {
 	int padlen, strln;     /* amount to pad */
 	int cnt = 0;
@@ -453,24 +489,26 @@ static void fmtstr(char *buffer, size_t *currlen, size_t maxlen,
 		padlen = -padlen; /* Left Justify */
 	
 	while ((padlen > 0) && (cnt < max)) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		--padlen;
 		++cnt;
 	}
 	while (*value && (cnt < max)) {
-		dopr_outch (buffer, currlen, maxlen, *value++);
+		DOPR_OUTCH(buffer, *currlen, maxlen, *value++);
 		++cnt;
 	}
 	while ((padlen < 0) && (cnt < max)) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		++padlen;
 		++cnt;
 	}
+	return 0;
 }
 
 /* Have to handle DP_F_NUM (ie 0x and 0 alternates) */
 
-static void fmtint(char *buffer, size_t *currlen, size_t maxlen,
+static int
+fmtint(char *buffer, size_t *currlen, size_t maxlen,
 		    LLONG value, int base, int min, int max, int flags)
 {
 	int signvalue = 0;
@@ -527,31 +565,32 @@ static void fmtint(char *buffer, size_t *currlen, size_t maxlen,
 
 	/* Spaces */
 	while (spadlen > 0) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		--spadlen;
 	}
 
 	/* Sign */
 	if (signvalue) 
-		dopr_outch (buffer, currlen, maxlen, signvalue);
+		DOPR_OUTCH(buffer, *currlen, maxlen, signvalue);
 
 	/* Zeros */
 	if (zpadlen > 0) {
 		while (zpadlen > 0) {
-			dopr_outch (buffer, currlen, maxlen, '0');
+			DOPR_OUTCH(buffer, *currlen, maxlen, '0');
 			--zpadlen;
 		}
 	}
 
 	/* Digits */
 	while (place > 0) 
-		dopr_outch (buffer, currlen, maxlen, convert[--place]);
+		DOPR_OUTCH(buffer, *currlen, maxlen, convert[--place]);
   
 	/* Left Justified spaces */
 	while (spadlen < 0) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		++spadlen;
 	}
+	return 0;
 }
 
 static LDOUBLE abs_val(LDOUBLE value)
@@ -564,13 +603,13 @@ static LDOUBLE abs_val(LDOUBLE value)
 	return result;
 }
 
-static LDOUBLE POW10(int exp)
+static LDOUBLE POW10(int val)
 {
 	LDOUBLE result = 1;
 	
-	while (exp) {
+	while (val) {
 		result *= 10;
-		exp--;
+		val--;
 	}
   
 	return result;
@@ -604,7 +643,10 @@ static double my_modf(double x0, double *iptr)
 	}
 
 	if (i == 100) {
-		/* yikes! the number is beyond what we can handle. What do we do? */
+		/*
+		 * yikes! the number is beyond what we can handle.
+		 * What do we do?
+		 */
 		(*iptr) = 0;
 		return 0;
 	}
@@ -623,8 +665,9 @@ static double my_modf(double x0, double *iptr)
 }
 
 
-static void fmtfp (char *buffer, size_t *currlen, size_t maxlen,
-		   LDOUBLE fvalue, int min, int max, int flags)
+static int
+fmtfp (char *buffer, size_t *currlen, size_t maxlen,
+    LDOUBLE fvalue, int min, int max, int flags)
 {
 	int signvalue = 0;
 	double ufvalue;
@@ -729,24 +772,24 @@ static void fmtfp (char *buffer, size_t *currlen, size_t maxlen,
 	
 	if ((flags & DP_F_ZERO) && (padlen > 0)) {
 		if (signvalue) {
-			dopr_outch (buffer, currlen, maxlen, signvalue);
+			DOPR_OUTCH(buffer, *currlen, maxlen, signvalue);
 			--padlen;
 			signvalue = 0;
 		}
 		while (padlen > 0) {
-			dopr_outch (buffer, currlen, maxlen, '0');
+			DOPR_OUTCH(buffer, *currlen, maxlen, '0');
 			--padlen;
 		}
 	}
 	while (padlen > 0) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		--padlen;
 	}
 	if (signvalue) 
-		dopr_outch (buffer, currlen, maxlen, signvalue);
+		DOPR_OUTCH(buffer, *currlen, maxlen, signvalue);
 	
 	while (iplace > 0) 
-		dopr_outch (buffer, currlen, maxlen, iconvert[--iplace]);
+		DOPR_OUTCH(buffer, *currlen, maxlen, iconvert[--iplace]);
 
 #ifdef DEBUG_SNPRINTF
 	printf("fmtfp: fplace=%d zpadlen=%d\n", fplace, zpadlen);
@@ -757,41 +800,37 @@ static void fmtfp (char *buffer, size_t *currlen, size_t maxlen,
 	 * char to print out.
 	 */
 	if (max > 0) {
-		dopr_outch (buffer, currlen, maxlen, '.');
+		DOPR_OUTCH(buffer, *currlen, maxlen, '.');
 		
 		while (zpadlen > 0) {
-			dopr_outch (buffer, currlen, maxlen, '0');
+			DOPR_OUTCH(buffer, *currlen, maxlen, '0');
 			--zpadlen;
 		}
 
 		while (fplace > 0) 
-			dopr_outch (buffer, currlen, maxlen, fconvert[--fplace]);
+			DOPR_OUTCH(buffer, *currlen, maxlen,
+			    fconvert[--fplace]);
 	}
 
 	while (padlen < 0) {
-		dopr_outch (buffer, currlen, maxlen, ' ');
+		DOPR_OUTCH(buffer, *currlen, maxlen, ' ');
 		++padlen;
 	}
-}
-
-static void dopr_outch(char *buffer, size_t *currlen, size_t maxlen, char c)
-{
-	if (*currlen < maxlen) {
-		buffer[(*currlen)] = c;
-	}
-	(*currlen)++;
+	return 0;
 }
 #endif /* !defined(HAVE_SNPRINTF) || !defined(HAVE_VSNPRINTF) */
 
 #if !defined(HAVE_VSNPRINTF)
-int vsnprintf (char *str, size_t count, const char *fmt, va_list args)
+static int
+vsnprintf (char *str, size_t count, const char *fmt, va_list args)
 {
 	return dopr(str, count, fmt, args);
 }
 #endif
 
 #if !defined(HAVE_SNPRINTF)
-int snprintf(char *str, size_t count, SNPRINTF_CONST char *fmt, ...)
+static int
+snprintf(char *str, size_t count, const char *fmt, ...)
 {
 	size_t ret;
 	va_list ap;
@@ -802,4 +841,3 @@ int snprintf(char *str, size_t count, SNPRINTF_CONST char *fmt, ...)
 	return ret;
 }
 #endif
-