linux-kernel - Re: [PATCH v3] lib/string.c: implement stpcpy

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]

Date:   Wed, 26 Aug 2020 19:42:17 -0700
From:   Joe Perches <joe@...ches.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        stable <stable@...r.kernel.org>, Andy Lavr <andy.lavr@...il.com>,
        Arvind Sankar <nivedita@...m.mit.edu>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Yury Norov <yury.norov@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] lib/string.c: implement stpcpy

On Wed, 2020-08-26 at 19:33 -0700, Kees Cook wrote:
> On Wed, Aug 26, 2020 at 04:57:41PM -0700, Joe Perches wrote:
> > On Wed, 2020-08-26 at 16:38 -0700, Kees Cook wrote:
> > > On Thu, Aug 27, 2020 at 07:59:45AM +0900, Masahiro Yamada wrote:
> > []
> > > > OK, then stpcpy(), strcpy() and sprintf()
> > > > have the same level of unsafety.
> > > 
> > > Yes. And even snprintf() is dangerous because its return value is how
> > > much it WOULD have written, which when (commonly) used as an offset for
> > > further pointer writes, causes OOB writes too. :(
> > > https://github.com/KSPP/linux/issues/105
> > > 
> > > > strcpy() is used everywhere.
> > > 
> > > Yes. It's very frustrating, but it's not an excuse to continue
> > > using it nor introducing more bad APIs.
> > > 
> > > $ git grep '\bstrcpy\b' | wc -l
> > > 2212
> > > $ git grep '\bstrncpy\b' | wc -l
> > > 751
> > > $ git grep '\bstrlcpy\b' | wc -l
> > > 1712
> > > 
> > > $ git grep '\bstrscpy\b' | wc -l
> > > 1066
> > > 
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
> > > https://github.com/KSPP/linux/issues/88
> > > 
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > > https://github.com/KSPP/linux/issues/89
> > > 
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > > https://github.com/KSPP/linux/issues/90
> > > 
> > > We have no way right now to block the addition of deprecated API usage,
> > > which makes ever catching up on this replacement very challenging.
> > 
> > These could be added to checkpatch's deprecated_api test.
> > ---
> >  scripts/checkpatch.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 149518d2a6a7..f9ccb2a63a95 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -605,6 +605,9 @@ foreach my $entry (@mode_permission_funcs) {
> >  $mode_perms_search = "(?:${mode_perms_search})";
> >  
> >  our %deprecated_apis = (
> > +	"strcpy"				=> "strscpy",
> > +	"strncpy"				=> "strscpy",
> > +	"strlcpy"				=> "strscpy",
> >  	"synchronize_rcu_bh"			=> "synchronize_rcu",
> >  	"synchronize_rcu_bh_expedited"		=> "synchronize_rcu_expedited",
> >  	"call_rcu_bh"				=> "call_rcu",
> > 
> > 
> 
> Good idea, yeah. We, unfortunately, need to leave strncpy() off this
> list for now because it's not *strictly* deprecated (see the notes in
> bug report[1]), but the others can be.

OK, but it is in Documentation/process/deprecated.rst

strncpy() on NUL-terminated strings
-----------------------------------
Use of strncpy() does not guarantee that the destination buffer
will be NUL terminated. This can lead to various linear read overflows
and other misbehavior due to the missing termination. It also NUL-pads the
destination buffer if the source contents are shorter than the destination
buffer size, which may be a needless performance penalty for callers using
only NUL-terminated strings. The safe replacement is strscpy().
(Users of strscpy() still needing NUL-padding should instead
use strscpy_pad().)

If a caller is using non-NUL-terminated strings, strncpy() can
still be used, but destinations should be marked with the `__nonstring
<https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
attribute to avoid future compiler warnings.