linux-kernel - Re: [PATCH v3 1/1] lib/vsprintf: Implement spprintf() 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: <20240201123004.GA938078@google.com>
Date: Thu, 1 Feb 2024 12:30:04 +0000
From: Lee Jones <lee@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Crutcher Dunnavant <crutcher+kernel@...astacks.com>,
	Juergen Quade <quade@...r.de>,
	David Laight <David.Laight@...lab.com>
Subject: Re: [PATCH v3 1/1] lib/vsprintf: Implement spprintf() to catch
 truncated strings

On Thu, 01 Feb 2024, Andy Shevchenko wrote:

> On Tue, Jan 30, 2024 at 04:09:53PM +0000, 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
> > spprintf() 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 = spprintf (buf, 10, "%s", "123456789");  // ret = 9
> > 
> >   Failure: Truncation - only 9 of 10 Bytes written; '-' is truncated
> > 
> >     ret = snprintf (buf, 10, "%s", "123456789---"); // ret = 12
> > 
> >       Reports: "12 Bytes would have been written if buf was large enough"
> >       Issue: Too easy for programmers to assume ret is Bytes written
> > 
> >     ret = scnprintf(buf, 10, "%s", "123456789---"); // ret = 9
> > 
> >       Reports: "9 Bytes actually written"
> >       Issue: Not testable - returns 9 on success AND failure (see above)
> > 
> >     ret = spprintf (buf, 10, "%s", "123456789---"); // ret = 10
> > 
> >       Reports: "Data provided is too large to fit in the buffer"
> >       Issue: No tangible impact: No way to tell how much data was lost
> > 
> > Since spprintf() only reports the total size of the buffer, it's easy to
> > test if they buffer overflowed since if we include the compulsory '\0',
> > only 9 Bytes additional Bytes can fit, so the return of 10 informs the
> > caller of an overflow.  Also, if the return data is plugged straight
> > into an additional call to spprintf() after the occurrence of an
> > overflow, no out-of-bounds will occur:
> > 
> >     int size = 10;
> >     char buf[size];
> >     char *b = buf;
> > 
> >     ret = spprintf(b, size, "1234");
> >     size -= ret;
> >     b += ret;
> >     // ret = 4  size = 6  buf = "1234\0"
> > 
> >     ret = spprintf(b, size, "5678");
> >     size -= ret;
> >     b += ret;
> >     // ret = 4  size = 2  buf = "12345678\0"
> > 
> >     ret = spprintf(b, size, "9***");
> >     size -= ret;
> >     b += ret;
> >     // ret = 2  size = 0  buf = "123456789\0"
> > 
> > Since size is now 0, further calls result in no changes of state.
> > 
> >     ret = spprintf(b, size, "----");
> >     size -= ret;
> >     b += ret;
> >     // ret = 0  size = 0  buf = "123456789\0"
> 
> > [0] https://lwn.net/Articles/69419/
> > [1] https://github.com/KSPP/linux/issues/105
> 
> Link: ... [0]
> Link: ... [1]

OOI, what does that do?

Does tooling pick-up on them?

These links are for humans.

Is there documentation I can go look at?

> > Signed-off-by: Lee Jones <lee@...nel.org>
> 
> ...
> 
> I'm a bit late in this discussion, but the commit message doesn't spit a single
> word on why seq_buf() approach can't be used in those cases?

When I can carve out a little more free time, investigating seq_buf() is
the next step.

-- 
Lee Jones [李琼斯]