linux-hardening - Re: [PATCH] lib/string_helpers: rework overflow-dependent code

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: <dyigpya2tb7obniv3g2rzhtahvjhximzjlvoi42c45fqkb7hx5@tw3loxvglexa>
Date: Fri, 9 Aug 2024 16:53:15 -0700
From: Justin Stitt <justinstitt@...gle.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Kees Cook <kees@...nel.org>, Andy Shevchenko <andy@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] lib/string_helpers: rework overflow-dependent code

On Fri, Aug 09, 2024 at 02:07:57PM GMT, Andy Shevchenko wrote:
> On Fri, Aug 9, 2024 at 2:11 AM Kees Cook <kees@...nel.org> wrote:
> >
> > On Fri, Aug 09, 2024 at 01:07:21AM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 9, 2024 at 12:44 AM Justin Stitt <justinstitt@...gle.com> wrote:
> > > >
> > > > When @size is 0, the desired behavior is to allow unlimited bytes to be
> > > > parsed. Currently, this relies on some intentional arithmetic overflow
> > > > where --size gives us SIZE_MAX when size is 0.
> > > >
> > > > Explicitly spell out the desired behavior without relying on intentional
> > > > overflow/underflow.
> > >
> > > Hmm... but why? Overflow for the _unsigned_ types is okay. No?
> >
> > Yes, it's well defined, but in trying to find a place to start making a
> > meaningful impact on unexpected wrap-around, after discussions with
> > Linus and Peter Zijlstra, we're going taking a stab at defining size_t
> > as not expecting to wrap. Justin has been collecting false positive
> > fixes while working on the compiler side of this, and I had asked him to
> > send this one now since I think it additionally helps with readability.
> 
> Okay, but the patch has an off-by-one error (which has no impact on
> the behavior as it's close to unrealistic to have the SIZE_MAX array).
> I prefer that patch can be reconsidered to keep original behaviour,
> otherwise it might be not so clear why 0 is SIZE_MAX - 1 in _this_
> case.

Right, it is technically different but still functionally provides the
"unlimited" behavior.

But, we could  do this too:

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 69ba49b853c7..0f76b5288833 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -320,11 +320,13 @@ static bool unescape_special(char **src, char **dst)
 int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
 {
 	char *out = dst;
+	bool unlimited = !size;
 
-	while (*src && --size) {
-		if (src[0] == '\\' && src[1] != '\0' && size > 1) {
+	while (*src && (unlimited || --size)) {
+		if (src[0] == '\\' && src[1] != '\0' &&
+		    (unlimited || size > 1)) {
 			src++;
-			size--;
+			size -= !unlimited;
 
 			if (flags & UNESCAPE_SPACE &&
 					unescape_space(&src, &out))



Really, I am fine with either.

Thanks
Justin