linux-hardening - Removing more str APIs (was Re: [PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style)

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, 7 Feb 2024 02:42:42 -0800
From: Kees Cook <keescook@...omium.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>, Richard Weinberger <richard@....at>,
	linux-um@...ts.infradead.org, Justin Stitt <justinstitt@...gle.com>,
	Anton Ivanov <anton.ivanov@...bridgegreys.com>,
	Johannes Berg <johannes@...solutions.net>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	Jason Wang <jasowang@...hat.com>, kernel test robot <lkp@...el.com>,
	Nathan Chancellor <nathan@...nel.org>,
	Azeem Shaikh <azeemshaikh38@...il.com>,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Removing more str APIs (was Re: [PATCH v3 4/4] um: Convert strscpy()
 usage to 2-argument style)

On Tue, Feb 06, 2024 at 05:02:16PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@...omium.org> wrote:
> >
> > The ARCH=um build has its own idea about strscpy()'s definition. Adjust
> > the callers to remove the redundant sizeof() arguments ahead of treewide
> > changes, since it needs a manual adjustment for the newly named
> > sized_strscpy() export.
> 
> ...
> 
> > -               strscpy(dir, home, sizeof(dir));
> > +               strscpy(dir, home);
> >                 uml_dir++;
> >         }
> >         strlcat(dir, uml_dir, sizeof(dir));
> 
> An (unrelated) side note: are we going to get rid of strlcat() as well
> (after strlcpy() is gone)?

I think it would be worthwhile to remove it, yes. Switching to seq_buf
in many cases seemed to be the clear solution, which is what triggered
my trying to improve the allocation ergonomics for seq_buf recently:
https://lore.kernel.org/linux-hardening/20231027155634.make.260-kees@kernel.org/

Its not in super common usage, so I think we can start chipping away at
it:

$ git grep '\bstrlcat(' | wc -l
480

It's more risky cases (using the return value) is relatively rare,
though, so I hadn't been prioritizing its removal:

$ git grep ' = strlcat(\b' | wc -l
13

(And almost all of it is in security/selinux/ima.c)


As a comparison, strncpy has even fewer currently, with Justin making a
dent on it recently:

$ git grep '\bstrncpy(' | wc -l
311


> 
> ...
> 
> >         if (*umid == '\0') {
> > -               strscpy(tmp, uml_dir, sizeof(tmp));
> > +               strscpy(tmp, uml_dir);
> >                 strlcat(tmp, "XXXXXX", sizeof(tmp));
> 
> This code is interesting... (Esp. taking into account making a
> temporary folder out of this...)

I have tried to avoid reading UML code too closely. ;)

-- 
Kees Cook