[PATCH] newgrp: use libc explicit_bzero() when it is available

[PATCH] newgrp: use libc explicit_bzero() when it is available

[Sami Kerola]

This currently new function will be part of glibc 2.25.

Reference: https://sourceware.org/git/?p=glibc.git;a=commit;h=ea1bd74defcf9d5291d14972e63105168ca9eb4f
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 configure.ac         | 1 +
 login-utils/newgrp.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/configure.ac b/configure.ac
index 796364f71..c50f07a47 100644
--- a/configure.ac
+++ b/configure.ac
@@ -379,6 +379,7 @@ AC_CHECK_FUNCS([ \
 	__secure_getenv \
 	err \
 	errx \
+	explicit_bzero \
 	fsync \
 	utimensat \
 	getdomainname \
diff --git a/login-utils/newgrp.c b/login-utils/newgrp.c
index 367333ec3..63a45cd6a 100644
--- a/login-utils/newgrp.c
+++ b/login-utils/newgrp.c
@@ -60,6 +60,7 @@ static char *xgetpass(FILE *input, const char *prompt)
 	return pass;
 }
 
+#ifndef HAVE_EXPLICIT_BZERO
 /* Ensure memory is set to value c without compiler optimization getting
  * into way that could happen with memset(3). */
 static int xmemset_s(void *v, size_t sz, const int c)
@@ -72,6 +73,7 @@ static int xmemset_s(void *v, size_t sz, const int c)
 		*p++ = c;
 	return 0;
 }
+#endif
 
 /* try to read password from gshadow */
 static char *get_gshadow_pwd(const char *groupname)
@@ -148,7 +150,11 @@ static int allow_setgid(const struct passwd *pe, const struct group *ge)
 	if (pwd && *pwd && (xpwd = xgetpass(stdin, _("Password: ")))) {
 		char *cbuf = crypt(xpwd, pwd);
 
+#ifdef HAVE_EXPLICIT_BZERO
+		explicit_bzero(xpwd, strlen(xpwd));
+#else
 		xmemset_s(xpwd, strlen(xpwd), 0);
+#endif
 		free(xpwd);
 		if (!cbuf)
 			warn(_("crypt failed"));
-- 
2.11.0

Re: [PATCH] newgrp: use libc explicit_bzero() when it is available

[Bernhard Voelker]

On 01/05/2017 11:33 PM, Sami Kerola wrote:
> +#ifdef HAVE_EXPLICIT_BZERO
> +		explicit_bzero(xpwd, strlen(xpwd));
> +#else
>  		xmemset_s(xpwd, strlen(xpwd), 0);
> +#endif
>  		free(xpwd);

Do you think it's worth making things even more complicated
for overwriting a simple string?  I mean, a simple

  for (char *c = xpwd; c; c++)
    c = '\0';

would do, wouldn't it?

Have a nice day,
Berny

Re: [PATCH] newgrp: use libc explicit_bzero() when it is available

[Rüdiger Meier]

On Friday 06 January 2017 12:09:09 Bernhard Voelker wrote:
> On 01/05/2017 11:33 PM, Sami Kerola wrote:
> > +#ifdef HAVE_EXPLICIT_BZERO
> > +		explicit_bzero(xpwd, strlen(xpwd));
> > +#else
> >  		xmemset_s(xpwd, strlen(xpwd), 0);
> > +#endif
> >  		free(xpwd);
>
> Do you think it's worth making things even more complicated
> for overwriting a simple string?  I mean, a simple
>
>   for (char *c = xpwd; c; c++)
>     c = '\0';
>
> would do, wouldn't it?

I'm not an expert about this but explicit_bzero() seems to do some tricks to 
avoid any compiler optimizations. Maybe the compiler would just not execute 
the complete for loop if we never access bytes after the (first) NULL byte 
later!? If this issue would be trivial then they probably wouldn't have added 
the explicit_bzero() function at all.

see BSD's bzero man page:

     The explicit_bzero() variant behaves the same, but will not be removed by
     a compiler's dead store optimization pass, making it useful for clearing
     sensitive memory such as a password.

and source code:

http://www.leidinger.net/FreeBSD/dox/libkern/html/d5/da7/explicit__bzero_8c_source.html


cu,
Rudi

Re: [PATCH] newgrp: use libc explicit_bzero() when it is available

[Aurélien Aptel]

Bernhard Voelker <mail@bernhard-voelker.de> writes:
> Do you think it's worth making things even more complicated
> for overwriting a simple string?  I mean, a simple
>
>   for (char *c = xpwd; c; c++)
>     c = '\0';
>
> would do, wouldn't it?

Play around with this example:

https://godbolt.org/g/SOSqt2

Compilers are pretty good at eliminated dead code as soon as you enable
optimizations nowadays, it's actually not that easy.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Re: [PATCH] newgrp: use libc explicit_bzero() when it is available

[Karel Zak]

On Thu, Jan 05, 2017 at 10:33:40PM +0000, Sami Kerola wrote:
>  configure.ac         | 1 +
>  login-utils/newgrp.c | 6 ++++++
>  2 files changed, 7 insertions(+)

Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com