linux-kernel - [PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <155223699287.4075.13184771244455866778.stgit@buzz>
Date:   Sun, 10 Mar 2019 19:56:32 +0300
From:   Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:     linux-kernel@...r.kernel.org
Cc:     Tejun Heo <tj@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>
Subject: [PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

Traditional scanf implementations ignore integer overflows because
C language standard allows here undefined behavior (§7.21.6.2 #10).

So, sane and safe behavior wouldn't harm anything.

This patch carefully checks integer overflows and stops matching if result
does not fit into appropriate type before assigning it into argument.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
---
 lib/vsprintf.c |   86 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 30 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 276a0bc3b019..ada0501f1525 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3026,12 +3026,13 @@ EXPORT_SYMBOL_GPL(bprintf);
  * - "%[...]" requires field width
  * - %s without field width limited with SHRT_MAX
  * - "%*..." simply skips non white-space characters without conversion
+ * - integer overflows are handled as matching failure
  */
 int vsscanf(const char *buf, const char *fmt, va_list args)
 {
 	const char *str = buf;
-	char *next;
-	char digit;
+	const char *next;
+	unsigned int rv;
 	int num = 0;
 	u8 qualifier;
 	unsigned int base;
@@ -3230,29 +3231,31 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		 */
 		str = skip_spaces(str);
 
-		digit = *str;
-		if (is_sign && digit == '-')
-			digit = *(str + 1);
+		next = str;
 
-		if (!digit
-		    || (base == 16 && !isxdigit(digit))
-		    || (base == 10 && !isdigit(digit))
-		    || (base == 8 && (!isdigit(digit) || digit > '7'))
-		    || (base == 0 && !isdigit(digit)))
-			break;
+		/* skip leading sign */
+		if (is_sign && (*str == '+' || *str == '-'))
+			next++;
 
-		if (is_sign)
-			val.s = qualifier != 'L' ?
-				simple_strtol(str, &next, base) :
-				simple_strtoll(str, &next, base);
-		else
-			val.u = qualifier != 'L' ?
-				simple_strtoul(str, &next, base) :
-				simple_strtoull(str, &next, base);
+		/* 64-bit integer conversion, similar to _kstrtoull() */
+		next = _parse_integer_fixup_radix(next, &base);
+		rv = _parse_integer(next, base, &val.u);
+		if (rv == 0)
+			break;
+		if (rv & KSTRTOX_OVERFLOW)
+			goto overflow;
+		next += rv;
+
+		if (is_sign) {
+			if (*str == '-') {
+				val.s = -val.u;
+				if (val.s > 0)
+					goto overflow;
+			} else if (val.s < 0)
+				goto overflow;
+		}
 
-		if (field_width > 0 && next - str > field_width) {
-			if (base == 0)
-				_parse_integer_fixup_radix(str, &base);
+		if (field_width > 0) {
 			while (next - str > field_width) {
 				if (is_sign)
 					val.s = div_s64(val.s, base);
@@ -3264,22 +3267,37 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 
 		switch (qualifier) {
 		case 'H':	/* that's 'hh' in format */
-			if (is_sign)
+			if (is_sign) {
+				if ((signed char)val.s != val.s)
+					goto overflow;
 				*va_arg(args, signed char *) = val.s;
-			else
+			} else {
+				if ((unsigned char)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned char *) = val.u;
+			}
 			break;
 		case 'h':
-			if (is_sign)
+			if (is_sign) {
+				if ((short)val.s != val.s)
+					goto overflow;
 				*va_arg(args, short *) = val.s;
-			else
+			} else {
+				if ((unsigned short)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned short *) = val.u;
+			}
 			break;
 		case 'l':
-			if (is_sign)
+			if (is_sign) {
+				if ((long)val.s != val.s)
+					goto overflow;
 				*va_arg(args, long *) = val.s;
-			else
+			} else {
+				if ((unsigned long)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned long *) = val.u;
+			}
 			break;
 		case 'L':
 			if (is_sign)
@@ -3288,13 +3306,20 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 				*va_arg(args, unsigned long long *) = val.u;
 			break;
 		case 'z':
+			if ((size_t)val.u != val.u)
+				goto overflow;
 			*va_arg(args, size_t *) = val.u;
 			break;
 		default:
-			if (is_sign)
+			if (is_sign) {
+				if ((int)val.s != val.s)
+					goto overflow;
 				*va_arg(args, int *) = val.s;
-			else
+			} else {
+				if ((unsigned int)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned int *) = val.u;
+			}
 			break;
 		}
 		num++;
@@ -3304,6 +3329,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		str = next;
 	}
 
+overflow:
 	return num;
 }
 EXPORT_SYMBOL(vsscanf);