Date: Tue, 14 Mar 2023 04:45:05 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv Message-ID: <BE0076A3-EF34-461F-805A-E193372415B3@freebsd.org> In-Reply-To: <CANCZdfq3_3xS1tac6t6db-rx%2B0%2Ba8tCvGjmH0Rt=Sh9=Y7T2bQ@mail.gmail.com> References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> <E40E7886-0EFC-4538-887F-ED067CB6FFFB@freebsd.org> <CANCZdfq3_3xS1tac6t6db-rx%2B0%2Ba8tCvGjmH0Rt=Sh9=Y7T2bQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 14 Mar 2023, at 04:37, Warner Losh <imp@bsdimp.com> wrote: >=20 >=20 >=20 > 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: > >=20 > > The branch main has been updated by imp: > >=20 > > URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21464d25bc61f98968a5c1e76ab= 3c808ae4 > >=20 > > 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 > >=20 > > Add GNU glibc compatible secure_getenv > >=20 > > 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". > >=20 > > 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(-) > >=20 > > 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+=3D a64l.3 abort.3 abs.3 alloca.3 = atexit.3 atof.3 \ > > MLINKS+=3Da64l.3 l64a.3 a64l.3 l64a_r.3 > > MLINKS+=3Datol.3 atoll.3 > > MLINKS+=3Dexit.3 _Exit.3 > > -MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 setenv.3 \ > > - getenv.3 unsetenv.3 > > +MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 = secure_getenv.3 \ > > + getenv.3 setenv.3 getenv.3 unsetenv.3 > > MLINKS+=3Dgetopt_long.3 getopt_long_only.3 > > MLINKS+=3Dhcreate.3 hdestroy.3 hcreate.3 hsearch.3 > > MLINKS+=3Dhcreate.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; > > }; > >=20 > > 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, >=20 > I don=E2=80=99t think it is now?.. >=20 > Yes... >=20 > > +.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) > > } > >=20 > >=20 > > +/* > > + * 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); >=20 > return (...); >=20 > (x2) >=20 > 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=E2=80=99t 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. >=20 > Warner=20 >=20 > Jess >=20 > > +} > > + > > /* > > * 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BE0076A3-EF34-461F-805A-E193372415B3>