linux-hardening - RE: [PATCH 1/1] lib/vsprintf: Implement ssprintf() to catch truncated strings

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: <54e518b6dd9647c1add38b706eccbb4b@AcuMS.aculab.com>
Date: Sat, 27 Jan 2024 14:32:23 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Lee Jones' <lee@...nel.org>, Rasmus Villemoes <linux@...musvillemoes.dk>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>, "Andrew
 Morton" <akpm@...ux-foundation.org>, Petr Mladek <pmladek@...e.com>, "Steven
 Rostedt" <rostedt@...dmis.org>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>, Sergey Senozhatsky
	<senozhatsky@...omium.org>, Crutcher Dunnavant
	<crutcher+kernel@...astacks.com>, Juergen Quade <quade@...r.de>
Subject: RE: [PATCH 1/1] lib/vsprintf: Implement ssprintf() to catch truncated
 strings

From: Lee Jones
> Sent: 25 January 2024 10:36
> On Thu, 25 Jan 2024, Rasmus Villemoes wrote:
> 
> > On 25/01/2024 09.39, Lee Jones wrote:
> > > There is an ongoing effort to replace the use of {v}snprintf() variants
> > > with safer alternatives - for a more in depth view, see Jon's write-up
> > > on LWN [0] and/or Alex's on the Kernel Self Protection Project [1].
> > >
> > > Whist executing the task, it quickly became apparent that the initial
> > > thought of simply s/snprintf/scnprintf/ wasn't going to be adequate for
> > > a number of cases.  Specifically ones where the caller needs to know
> > > whether the given string ends up being truncated.  This is where
> > > ssprintf() [based on similar semantics of strscpy()] comes in, since it
> > > takes the best parts of both of the aforementioned variants.  It has the
> > > testability of truncation of snprintf() and returns the number of Bytes
> > > *actually* written, similar to scnprintf(), making it a very programmer
> > > friendly alternative.
> > >
> > > Here's some examples to show the differences:
> > >
> > >   Success: No truncation - all 9 Bytes successfully written to the buffer
> > >
> > >     ret = snprintf (buf, 10, "%s", "123456789");  // ret = 9
> > >     ret = scnprintf(buf, 10, "%s", "123456789");  // ret = 9
> > >     ret = ssprintf (buf, 10, "%s", "123456789");  // ret = 9
> > >
> > >   Failure: Truncation - only 9 of 10 Bytes written; '-' is truncated
> > >
> > >     ret = snprintf (buf, 10, "%s", "123456789-"); // ret = 10
> > >
> > >       Reports: "10 Bytes would have been written if buf was large enough"
> > >       Issue: Programmers need to know/remember to check ret against "10"
> >
> > Yeah, so I'm not at all sure we need yet-another-wrapper with
> > yet-another-hard-to-read-prefix when people can just RTFM and learn how
> > to check for truncation or whatnot. But if you do this:
> 
> As wonderful as it would be for people to "just RTFM", we're seeing a
> large number of cases where this isn't happening.  Providing a more
> programmer friendly way is thought, by people way smarter than me, to be
> a solid means to solve this issue.  Please also see Kees Cook's related
> work to remove strlcpy() use.

My worry is that people will believe the length and forget that
it might be an error code.

So you replace one set of errors (truncated data), with another
worse set (eg write before start of buffer).

I'm sure that the safest return for 'truncated' is the buffer length.
The a series of statements like:
	buf += xxx(buf, buf_end - buf, .....);
can all be called with a single overflow check at the end.

Forget the check, and the length just contains a trailing '\0'
which might cause confusion but isn't going to immediately
break the world.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)