linux-kernel - Re: snprintf, overlapping destination and source

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87fuz24jsx.fsf@rasmusvillemoes.dk>
Date:	Wed, 16 Dec 2015 14:14:38 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Kees Cook <keescook@...omium.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Julia Lawall <julia.lawall@...6.fr>
Subject: Re: snprintf, overlapping destination and source

On Mon, Dec 07 2015, Kees Cook <keescook@...omium.org> wrote:

> On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:
>> I did a search for code doing
>>
>>   s[n]printf(buf, "...", ..., buf, ...)
>>
>> and found a few instances. They all do it with the format string
>> beginning with "%s" and buf being passed as the corresponding parameter
>> (obviously to append to the existing string). That works (AFAICT), both
>> with the current printf implementation and with the string()
>> modification which is now in -mm. It would obviously go horribly wrong
>> if anything, even non-specifiers, precede the "%s" in the format
>> string.
[...]
>
> If the replacement isn't ugly/complex/error-prone, we should fix it
> and find a way to detect the issue. Otherwise, we should leave it and
> add it to the printf test cases so we'll notice if it ever regresses.

The usual pattern is to keep the length so far in a variable and do

  len += snprintf(buf + len, bufsize - len, ...)

So this fails if/when overflow is actually possible (we'd end up with a
huge positive size being passed, trigger the WARN_ONCE in vsnprintf and
get a return value of 0, and that would then repeat). But scnprintf has
the property that if you pass a positive bufsize, the return value is
strictly less than that; so if one subtracts said return value from the
available buffer size, one still has a positive buffer size. (Maybe that
invariant should be spelled out somewhere.)

IOW, I think that most users of repeated snprintf(buf, bufsize, "%s...",
buf, ...) could be replaced by 'int len = 0; ... len += scnprintf(buf +
len, bufsize - len, "...", ...);'. Let me go see how that would look.

When sprintf is being used I think one can just directly convert to the
"len +=" model; one would overflow the buffer if and only if the other
would (I think).

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/