linux-hardening - Re: [PATCH v2] kdb: replace deprecated strncpy

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: <20240409183558.GA7510@aspen.lan>
Date: Tue, 9 Apr 2024 19:35:58 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Jason Wessel <jason.wessel@...driver.com>,
	Douglas Anderson <dianders@...omium.org>,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] kdb: replace deprecated strncpy

On Mon, Apr 08, 2024 at 05:46:42PM -0700, Justin Stitt wrote:
> On Fri, Apr 5, 2024 at 2:51 AM Daniel Thompson
> <daniel.thompson@...aro.org> wrote:
> >
> > >                       len_tmp = strlen(p_tmp);
> > > -                     strncpy(cp, p_tmp+len, len_tmp-len + 1);
> > > +                     strscpy(cp, p_tmp+len, len_tmp-len + 1);
> >
> > Again, I really don't think the third argument provides the number of
> > characters in the destination buffer.
> >
>
> Right, the third argument is the length of the "remaining" characters
> from the completion point.

Which is not how strscpy() is designed to be used.

> if you type "tes" and press tab then kallsyms_symbol_complete() will
> populate p_tmp with "test". Prior to rendering to the user, @cp points
> to "s", we need to catch the user up and print the rest of the symbol
> name since they've already typed "tes" we only need to print out "t".

I'm more concerned about the case where you fill the buffer entirely
then move the cursor left until you get to the tes and then press Tab.
I think at the point we write too many bytes to cp.

> len_tmp is the length of the entire symbol part as returned by
> kallsyms_symbol_complete() and len is the length of only the
> user-typed symbol. Therefore, the amount of remaining characters to
> print is given by len_tmp-len (and +1 for a NUL-byte).
>
> So, yeah, you're right. This isn't the length of the destination but I
> don't see why we can't use memcpy() (or strscpy()) and have this not
> be considered "broken". The pointer arithmetic checks out.

The problem with substituting strncpy() with memcpy() is that is *not*
obviously wrong... but it could be subtly wrong.

We can see that the person who originally wrote this code made a pretty
serious mistake with strncpy() and the third argument if garbage. It is
therefore important to figure out what the *correct* value for argument
#3 should have been *before* we attempt to replace strncpy() with
anything.

Transforming something we know to be broken without fixing it first
means it is impossible to know if the transformation is correct or not.
Hence the original question, how do we know there is enough space
after cp to store the string?

Daniel.