Message-ID: <202310181929.BCC265C@keescook>
Date:   Wed, 18 Oct 2023 19:56:36 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Randy Dunlap <rdunlap@...radead.org>
Cc:     Bagas Sanjaya <bagasdotme@...il.com>,
        James Dutton <james.dutton@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Justin Stitt <justinstitt@...gle.com>,
        Calvince Otieno <calvncce@...il.com>,
        Azeem Shaikh <azeemshaikh38@...il.com>,
        Andy Shevchenko <andy@...nel.org>
Subject: Re: Is strncpy really less secure than strscpy ?

On Wed, Oct 18, 2023 at 07:27:20PM -0700, Randy Dunlap wrote:
> 
> 
> On 10/18/23 18:49, Bagas Sanjaya wrote:
> > [Disclaimer: I have little to no knowledge of C, so things may be wrong.
> >  Please correct me if it is the case. Also Cc: recent people who work on
> >  strscpy() conversion.]

Here are the current docs on the deprecated use of strncpy:
https://docs.kernel.org/process/deprecated.html#strncpy-on-nul-terminated-strings
which could probably be improved.

> Also Cc: the STRING maintainers.
> 
> > On Thu, Oct 19, 2023 at 12:22:33AM +0100, James Dutton wrote:
> >> Is strncpy really less secure than strscpy ?

Very. :)

> >> If one uses strncpy and thus put a limit on the buffer size during the
> >> copy, it is safe. There are no writes outside of the buffer.
> >> If one uses strscpy and thus put a limit on the buffer size during the
> >> copy, it is safe. There are no writes outside of the buffer.
> > 
> > Well, assuming that the string is NUL-terminated, the end result should
> > be the same.

Note the use of "If" in the above sentences. :) This is what makes
strncpy so dangerous -- it's only "correct" if the length argument is
less than the size of the _source_ buffer. And by "correct", I mean
that only then will strncpy produce a C-string. If not, it's a memcpy
and leaves the buffer unterminated. This lack of %NUL-termination leads
to all kinds of potential "downstream" problems with reading past the
end of the buffer, etc.

One of the easiest ways to avoid bugs is to remove ambiguity, and
strncpy is full of it. :P

Almost more important than the potential lack of %NUL-termination is
the fact that strncpy() doesn't tell other maintainers _why_ it was used
because it has several "effects" that aren't always intended:

- is the destination supposed to be %NUL terminated? (We covered this)
- is the destination supposed to be %NUL padded?

strncpy pads the destination -- is this needed? Is it a waste of CPU
time?

> > 
> >> But, one can fit more characters in strncpy than strscpy because
> >> strscpy enforces the final \0 on the end.
> >> One could argue that strncpy is better because it might save the space
> >> of one char at the end of a string array.

At the cost of creating non-C-strings. And this is a completely bonkers
result for the "C String API" to produce. :P

> >> There are cases where strncpy might be unsafe. For example copying
> >> between arrays of different sizes, and that is a case where strscpy
> >> might be safer, but strncpy can be made safe if one ensures that the
> >> size used in strncpy is the smallest of the two different array sizes.

The CONFIG_FORTIFY_SOURCE option in the kernel adds a bunch of
sanity-checking to the APIs (some of which can be determined at compile
time), but it doesn't remove the ambiguity of using strncpy. We want the
kernel to have maintainable code, and when it's not clear which of a
handful of side-effects are _intended_ from an API, that's a bad API. :)

> > 
> > Code example on both cases?
> > 
> >>
> >> If one blindly replaces strncpy with strscpy across all uses, one
> >> could unintentionally be truncating the results and introduce new
> >> bugs.

Yes, of course. That's why we're not blindly replacing them. :) And the
diagnostic criteria has been carefully described:
https://github.com/KSPP/linux/issues/90

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists