Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv
Date: Tue, 14 Mar 2023 04:45:05 UTC
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.
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