Date: Tue, 14 Mar 2023 11:03:27 -0600 From: Warner Losh <imp@bsdimp.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Jessica Clarke <jrtc27@freebsd.org>, Warner Losh <imp@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv Message-ID: <CANCZdfoYsfij=MnrR4F47a0YHVgT28E_TWko60We99kME7eueQ@mail.gmail.com> In-Reply-To: <CAGudoHF5um_5MCj0TEn-U18wi=i8GW74NUYa5J7Yo20R1UbBag@mail.gmail.com> References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> <E40E7886-0EFC-4538-887F-ED067CB6FFFB@freebsd.org> <CAGudoHF5um_5MCj0TEn-U18wi=i8GW74NUYa5J7Yo20R1UbBag@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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> > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 14, 2023 at 10:49 AM Mateusz Guzik <<a href="mailto:mjguzik@gmail.com">mjguzik@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 3/14/23, Jessica Clarke <<a href="mailto:jrtc27@freebsd.org" target="_blank">jrtc27@freebsd.org</a>> wrote:<br> > On 14 Mar 2023, at 04:19, Warner Losh <imp@FreeBSD.org> wrote:<br> >><br> >> The branch main has been updated by imp:<br> >><br> >> URL:<br> >> <a href="https://cgit.FreeBSD.org/src/commit/?id=adeca21464d25bc61f98968a5c1e76ab3c808ae4" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=adeca21464d25bc61f98968a5c1e76ab3c808ae4</a><br>; >><br> >> commit adeca21464d25bc61f98968a5c1e76ab3c808ae4<br> >> Author: lucy <<a href="mailto:seafork@disroot.org" target="_blank">seafork@disroot.org</a>><br> >> AuthorDate: 2023-03-13 22:01:12 +0000<br> >> Commit: Warner Losh <imp@FreeBSD.org><br> >> CommitDate: 2023-03-14 04:19:24 +0000<br> >><br> >> Add GNU glibc compatible secure_getenv<br> >><br> >> Add mostly glibc and msl compatible secure_getenv. Return NULL if<br> >> issetugid() indicates the process is tainted, otherwise getenv(x).<br> >> The<br> >> rational behind this is the fact that many Linux applications use this<br> >> function instead of getenv() as it's widely consider a, "best<br> >> practice".<br> >><br> >> Reviewed by: imp, mjg (feedback)<br> >> Pull Request: <a href="https://github.com/freebsd/freebsd-src/pull/686" rel="noreferrer" target="_blank">https://github.com/freebsd/freebsd-src/pull/686</a><br>; >> Signed-off-by: Lucy Marsh <<a href="mailto:seafork@disroot.org" target="_blank">seafork@disroot.org</a>><br> >> ---<br> >> include/stdlib.h | 1 +<br> >> lib/libc/stdlib/Makefile.inc | 4 ++--<br> >> lib/libc/stdlib/Symbol.map | 1 +<br> >> lib/libc/stdlib/getenv.3 | 26 +++++++++++++++++++++++++-<br> >> lib/libc/stdlib/getenv.c | 12 ++++++++++++<br> >> 5 files changed, 41 insertions(+), 3 deletions(-)<br> >><br> >> diff --git a/include/stdlib.h b/include/stdlib.h<br> >> index 01629ed84a11..c41e8704e810 100644<br> >> --- a/include/stdlib.h<br> >> +++ b/include/stdlib.h<br> >> @@ -111,6 +111,7 @@ void qsort(void *, size_t, size_t,<br> >> int (* _Nonnull)(const void *, const void *));<br> >> int rand(void);<br> >> void *realloc(void *, size_t) __result_use_check __alloc_size(2);<br> >> +char *secure_getenv(const char *);<br> >> void srand(unsigned);<br> >> double strtod(const char * __restrict, char ** __restrict);<br> >> float strtof(const char * __restrict, char ** __restrict);<br> >> diff --git a/lib/libc/stdlib/Makefile.inc b/lib/libc/stdlib/Makefile.inc<br> >> index 8ace2c051b82..964e7ce30594 100644<br> >> --- a/lib/libc/stdlib/Makefile.inc<br> >> +++ b/lib/libc/stdlib/Makefile.inc<br> >> @@ -46,8 +46,8 @@ MAN+= a64l.3 abort.3 abs.3 alloca.3 atexit.3 atof.3 \<br> >> MLINKS+=a64l.3 l64a.3 a64l.3 l64a_r.3<br> >> MLINKS+=atol.3 atoll.3<br> >> MLINKS+=exit.3 _Exit.3<br> >> -MLINKS+=getenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 setenv.3 \<br> >> - getenv.3 unsetenv.3<br> >> +MLINKS+=getenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_getenv.3 \<br> >> + getenv.3 setenv.3 getenv.3 unsetenv.3<br> >> MLINKS+=getopt_long.3 getopt_long_only.3<br> >> MLINKS+=hcreate.3 hdestroy.3 hcreate.3 hsearch.3<br> >> MLINKS+=hcreate.3 hcreate_r.3 hcreate.3 hdestroy_r.3 hcreate.3<br> >> hsearch_r.3<br> >> diff --git a/lib/libc/stdlib/Symbol.map b/lib/libc/stdlib/Symbol.map<br> >> index 9d2944fdb7e9..a105f781734d 100644<br> >> --- a/lib/libc/stdlib/Symbol.map<br> >> +++ b/lib/libc/stdlib/Symbol.map<br> >> @@ -128,6 +128,7 @@ FBSD_1.6 {<br> >> FBSD_1.7 {<br> >> clearenv;<br> >> qsort_r;<br> >> + secure_getenv;<br> >> };<br> >><br> >> FBSDprivate_1.0 {<br> >> diff --git a/lib/libc/stdlib/getenv.3 b/lib/libc/stdlib/getenv.3<br> >> index 5566d7b01dcd..93c0d2ada6ad 100644<br> >> --- a/lib/libc/stdlib/getenv.3<br> >> +++ b/lib/libc/stdlib/getenv.3<br> >> @@ -32,13 +32,14 @@<br> >> .\" @(#)getenv.3 8.2 (Berkeley) 12/11/93<br> >> .\" $FreeBSD$<br> >> .\"<br> >> -.Dd November 7, 2021<br> >> +.Dd March 13, 2023<br> >> .Dt GETENV 3<br> >> .Os<br> >> .Sh NAME<br> >> .Nm clearenv ,<br> >> .Nm getenv ,<br> >> .Nm putenv ,<br> >> +.Nm secure_getenv ,<br> >> .Nm setenv ,<br> >> .Nm unsetenv<br> >> .Nd environment variable functions<br> >> @@ -50,6 +51,8 @@<br> >> .Fn clearenv "void"<br> >> .Ft char *<br> >> .Fn getenv "const char *name"<br> >> +.Ft char *<br> >> +.Fn secure_getenv "const char *name"<br> >> .Ft int<br> >> .Fn setenv "const char *name" "const char *value" "int overwrite"<br> >> .Ft int<br> >> @@ -78,6 +81,20 @@ to by the<br> >> .Fn getenv<br> >> function.<br> >> .Pp<br> >> +The GNU-specific function,<br> ><br> > I don’t think it is now?..<br> ><br> <br> It is not and this is not the place to point out any GNU-related<br> information either. Also wrapping getenv is an implementation detail<br> which should not be mentioned.<br> <br> Instead, modulo bad english, content-wise it should be +/-:<br> <br> The secure_getenv function acts like getenv, except intentionally<br> returns NULL when the environment is considered untrusted. Currently<br> this means executing in a setuid or setgid context, but the list of<br> limitations of subject to change.<br> <br> then in STANDARDS:<br> <br> The secure_getenv function was added for compatiblity with GNU libc.<br></blockquote><div><br></div><div> <a href="https://reviews.freebsd.org/D39078">https://reviews.freebsd.org/D39078</a>; might address this.<br></div><div><br></div><div>Warner</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> >> +.Fn secure_getenv<br> >> +wraps the<br> >> +.Fn getenv<br> >> +function to prevent it from being run in "secure execution".<br> >> +Unlike in glibc,<br> >> +.Fn secure_getenv<br> >> +only checks if the<br> >> +.Fa setuid<br> >> +and<br> >> +.Fa setgid<br> >> +bits have been set or changed.<br> >> +These checks are subject to extension and change.<br> >> +.Pp<br> >> The<br> >> .Fn setenv<br> >> function inserts or resets the environment variable<br> >> @@ -139,6 +156,13 @@ is not in the current environment,<br> >> .Dv NULL<br> >> is returned.<br> >> .Pp<br> >> +The<br> >> +.Fn secure_getenv<br> >> +function returns<br> >> +.Dv NULL<br> >> +if the process is in "secure execution," otherwise it will call<br> >> +.Fn getenv .<br> >> +.Pp<br> >> .Rv -std clearenv setenv putenv unsetenv<br> >> .Sh ERRORS<br> >> .Bl -tag -width Er<br> >> diff --git a/lib/libc/stdlib/getenv.c b/lib/libc/stdlib/getenv.c<br> >> index 7ca27ab710c4..86a846d58c69 100644<br> >> --- a/lib/libc/stdlib/getenv.c<br> >> +++ b/lib/libc/stdlib/getenv.c<br> >> @@ -447,6 +447,18 @@ getenv(const char *name)<br> >> }<br> >><br> >><br> >> +/*<br> >> + * Runs getenv() unless the current process is tainted by uid or gid<br> >> changes, in<br> >> + * which case it will return NULL.<br> >> + */<br> >> +char *<br> >> +secure_getenv(const char *name)<br> >> +{<br> >> + if (issetugid())<br> >> + return NULL;<br> >> + return getenv(name);<br> ><br> > return (...);<br> ><br> > (x2)<br> ><br> > Jess<br> ><br> >> +}<br> >> +<br> >> /*<br> >> * Set the value of a variable. Older settings are labeled as inactive.<br> >> If an<br> >> * older setting has enough room to store the new value, it will be<br> >> reused. No<br> ><br> ><br> <br> <br> -- <br> Mateusz Guzik <mjguzik <a href="http://gmail.com" rel="noreferrer" target="_blank">gmail.com</a>><br> </blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoYsfij=MnrR4F47a0YHVgT28E_TWko60We99kME7eueQ>