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

On Tue, Mar 14, 2023 at 10:49 AM Mateusz Guzik <mjguzik@gmail.com> wrote:

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

 https://reviews.freebsd.org/D39078 might address this.

Warner


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