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

On Mon, Mar 13, 2023, 10:21 PM 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?..
>

Yes...

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

style(9) says they are optional now so it's below my radar.... but
honestly, it's good enough given the dozen other issues fixed in the pr.

Though I'll see about adding git clang-format setup to check. I wrote a
stupidly simple script to check 2 years ago and the commits coming in at
the time had way more exceptions and variations than I thought I'd catch
when I tested it out... but it could be turned into a gihub action easily
enough I think.

Warner

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