Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv

On 14 Mar 2023, at 04:19, Warner Losh <imp@FreeBSD.org> wrote:
> 
> The branch main has been updated by imp:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=adeca21464d25bc61f98968a5c1e76ab3c808ae4
> 
> commit adeca21464d25bc61f98968a5c1e76ab3c808ae4
> Author:     lucy <seafork@disroot.org>
> AuthorDate: 2023-03-13 22:01:12 +0000
> Commit:     Warner Losh <imp@FreeBSD.org>
> CommitDate: 2023-03-14 04:19:24 +0000
> 
>    Add GNU glibc compatible secure_getenv
> 
>    Add mostly glibc and msl compatible secure_getenv. Return NULL if
>    issetugid() indicates the process is tainted, otherwise getenv(x).  The
>    rational behind this is the fact that many Linux applications use this
>    function instead of getenv() as it's widely consider a, "best
>    practice".
> 
>    Reviewed by: imp, mjg (feedback)
>    Pull Request: https://github.com/freebsd/freebsd-src/pull/686
>    Signed-off-by: Lucy Marsh <seafork@disroot.org>
> ---
> include/stdlib.h             |  1 +
> lib/libc/stdlib/Makefile.inc |  4 ++--
> lib/libc/stdlib/Symbol.map   |  1 +
> lib/libc/stdlib/getenv.3     | 26 +++++++++++++++++++++++++-
> lib/libc/stdlib/getenv.c     | 12 ++++++++++++
> 5 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/include/stdlib.h b/include/stdlib.h
> index 01629ed84a11..c41e8704e810 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -111,6 +111,7 @@ void	 qsort(void *, size_t, size_t,
> 	    int (* _Nonnull)(const void *, const void *));
> int	 rand(void);
> void	*realloc(void *, size_t) __result_use_check __alloc_size(2);
> +char	*secure_getenv(const char *);
> void	 srand(unsigned);
> double	 strtod(const char * __restrict, char ** __restrict);
> float	 strtof(const char * __restrict, char ** __restrict);
> diff --git a/lib/libc/stdlib/Makefile.inc b/lib/libc/stdlib/Makefile.inc
> index 8ace2c051b82..964e7ce30594 100644
> --- a/lib/libc/stdlib/Makefile.inc
> +++ b/lib/libc/stdlib/Makefile.inc
> @@ -46,8 +46,8 @@ MAN+=	a64l.3 abort.3 abs.3 alloca.3 atexit.3 atof.3 \
> MLINKS+=a64l.3 l64a.3 a64l.3 l64a_r.3
> MLINKS+=atol.3 atoll.3
> MLINKS+=exit.3 _Exit.3
> -MLINKS+=getenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 setenv.3 \
> -	getenv.3 unsetenv.3
> +MLINKS+=getenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_getenv.3 \
> +	getenv.3 setenv.3 getenv.3 unsetenv.3
> MLINKS+=getopt_long.3 getopt_long_only.3
> MLINKS+=hcreate.3 hdestroy.3 hcreate.3 hsearch.3
> MLINKS+=hcreate.3 hcreate_r.3 hcreate.3 hdestroy_r.3 hcreate.3 hsearch_r.3
> diff --git a/lib/libc/stdlib/Symbol.map b/lib/libc/stdlib/Symbol.map
> index 9d2944fdb7e9..a105f781734d 100644
> --- a/lib/libc/stdlib/Symbol.map
> +++ b/lib/libc/stdlib/Symbol.map
> @@ -128,6 +128,7 @@ FBSD_1.6 {
> FBSD_1.7 {
> 	clearenv;
> 	qsort_r;
> +	secure_getenv;
> };
> 
> FBSDprivate_1.0 {
> diff --git a/lib/libc/stdlib/getenv.3 b/lib/libc/stdlib/getenv.3
> index 5566d7b01dcd..93c0d2ada6ad 100644
> --- a/lib/libc/stdlib/getenv.3
> +++ b/lib/libc/stdlib/getenv.3
> @@ -32,13 +32,14 @@
> .\"     @(#)getenv.3	8.2 (Berkeley) 12/11/93
> .\" $FreeBSD$
> .\"
> -.Dd November 7, 2021
> +.Dd March 13, 2023
> .Dt GETENV 3
> .Os
> .Sh NAME
> .Nm clearenv ,
> .Nm getenv ,
> .Nm putenv ,
> +.Nm secure_getenv ,
> .Nm setenv ,
> .Nm unsetenv
> .Nd environment variable functions
> @@ -50,6 +51,8 @@
> .Fn clearenv "void"
> .Ft char *
> .Fn getenv "const char *name"
> +.Ft char *
> +.Fn secure_getenv "const char *name"
> .Ft int
> .Fn setenv "const char *name" "const char *value" "int overwrite"
> .Ft int
> @@ -78,6 +81,20 @@ to by the
> .Fn getenv
> function.
> .Pp
> +The GNU-specific function,

I don’t think it is now?..

> +.Fn secure_getenv
> +wraps the
> +.Fn getenv
> +function to prevent it from being run in "secure execution".
> +Unlike in glibc,
> +.Fn secure_getenv
> +only checks if the
> +.Fa setuid
> +and
> +.Fa setgid
> +bits have been set or changed.
> +These checks are subject to extension and change.
> +.Pp
> The
> .Fn setenv
> function inserts or resets the environment variable
> @@ -139,6 +156,13 @@ is not in the current environment,
> .Dv NULL
> is returned.
> .Pp
> +The
> +.Fn secure_getenv
> +function returns
> +.Dv NULL
> +if the process is in "secure execution," otherwise it will call
> +.Fn getenv .
> +.Pp
> .Rv -std clearenv setenv putenv unsetenv
> .Sh ERRORS
> .Bl -tag -width Er
> diff --git a/lib/libc/stdlib/getenv.c b/lib/libc/stdlib/getenv.c
> index 7ca27ab710c4..86a846d58c69 100644
> --- a/lib/libc/stdlib/getenv.c
> +++ b/lib/libc/stdlib/getenv.c
> @@ -447,6 +447,18 @@ getenv(const char *name)
> }
> 
> 
> +/*
> + * Runs getenv() unless the current process is tainted by uid or gid changes, in
> + * which case it will return NULL.
> + */
> +char *
> +secure_getenv(const char *name)
> +{
> +	if (issetugid())
> +		return NULL;
> +	return getenv(name);

return (...);

(x2)

Jess

> +}
> +
> /*
>  * Set the value of a variable.  Older settings are labeled as inactive.  If an
>  * older setting has enough room to store the new value, it will be reused.  No