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

On Mon, Mar 13, 2023, 10:45 PM Jessica Clarke <jrtc27@freebsd.org> wrote:

> On 14 Mar 2023, at 04:37, Warner Losh <imp@bsdimp.com> wrote:
> >
> >
> >
> > 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.
>
> I still see "Values in return statements should be enclosed in
> parentheses."? Which isn’t an RFC SHOULD, much of style(9) is should
> rather than must.
>

We need to do a pass to use rfc language. In this case,  thought it's been
encouraged but not required for a long time. I added the explicit language
years ago since it was only by example prior to that. I thought this was
one of the many lossenings we've had over the years, but if so there's been
no update since I added the should language... I know I've committed a lot
of new code without it over the years...

It may just have been a combo of many other projects forbidding the
practice and it was one of the variations my script found... I'll have to
go find the script to see.

But like I said before... git clang-format...

Warner

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