linux-kernel - Re: [PATCH RFC] sscanf: Fix integer overflow with sscanf field width

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] [day] [month] [year] [list]

Message-Id: <5CE79C360200007800231E1D@prv1-mh.provo.novell.com>
Date:   Fri, 24 May 2019 01:24:38 -0600
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Fredrik Noring" <noring@...rew.org>
Cc:     <adobriyan@...il.com>, <akpm@...ux-foundation.org>,
        "Jessica Yu" <jeyu@...hat.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] sscanf: Fix integer overflow with sscanf field
 width

>>> On 23.05.19 at 19:27, <noring@...rew.org> wrote:
> This fixes 53809751ac23 ("sscanf: don't ignore field widths for numeric
> conversions"). sscanf overflows integers with simple strings such as dates.
> As an example, consider
> 
> 	r = sscanf("20190523123456", "%4d%2d%2d%2d%2d%2d",
> 		&year, &month, &day,
> 		&hour, &minute, &second);
> 
> 	pr_info("%d %04d-%02d-%2d %02d:%02d:%02d\n",
> 		r,
> 		year, month, day,
> 		hour, minute, second);
> 
> On a 32-bit machine this prints
> 
> 	6 0000-05-23 12:34:56
> 
> where the year is zero, and not 2019 as expected. The reason is that sscanf
> attempts to read 20190523123456 as a whole integer and then divide it with
> 10^10 to obtain 2019, which obviously fails. Of course, 64-bit machines fail
> similarly on longer numerical strings.

Right, and that's also what that commit's description says remains as
non-conforming behavior.

> I'm offering a simple patch to correct this below. The idea is to have a
> variant of _parse_integer() called _parse_integer_end(), with the ability
> to stop consuming digits. The functions
> 
> 	simple_{strtol,strtoll,strtoul,strtoull}()
> 
> now have the corresponding
> 
> 	sscanf_{strtol,strtoll,strtoul,strtoull}()
> 
> taking a field width into account. There are some code duplication issues
> etc. so one might consider making more extensive changes than these.

I'm not the maintainer here, but to me it looks mostly okay.

> +static long sscanf_strtol(const char *cp, int field_width,
> +	char **endp, unsigned int base)
> +{
> +	if (*cp == '-')
> +		return -sscanf_strtoul(cp + 1, field_width - 1, endp, base);

I'm afraid you may neither convert a field width of zero to -1 here,
nor convert a field width of 1 to zero (unlimited).

I'd also like to note that the 'u' and 'x' format characters also accept
a sign as per the standard, but that's an orthogonal issue which you
may or may not want to address at the same time.

Jan