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

On 3/14/23, Jessica Clarke <jrtc27@freebsd.org> wrote:
> 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?..
>

It is not and this is not the place to point out any GNU-related
information either. Also wrapping getenv is an implementation detail
which should not be mentioned.

Instead, modulo bad english, content-wise it should be +/-:

The secure_getenv function acts like getenv, except intentionally
returns NULL when the environment is considered untrusted. Currently
this means executing in a setuid or setgid context, but the list of
limitations of subject to change.

then in STANDARDS:

The secure_getenv function was added for compatiblity with GNU libc.

>> +.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
>
>


-- 
Mateusz Guzik <mjguzik gmail.com>