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
--0000000000006edcdc05f6df3524 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Mar 14, 2023 at 10:49=E2=80=AFAM 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=3Dadeca21464d25bc61f98968a5c1e76a= b3c808ae4 > >> > >> 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.i= nc > >> 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_geten= v.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; > >> }; > >> > >> 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=E2=80=99t 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 inactiv= e. > >> If an > >> * older setting has enough room to store the new value, it will be > >> reused. No > > > > > > > -- > Mateusz Guzik <mjguzik gmail.com> > --0000000000006edcdc05f6df3524 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Tue, Mar 14, 2023 at 10:49=E2=80= =AFAM Mateusz Guzik <<a href=3D"mailto:mjguzik@gmail.com">mjguzik@gmail.= com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"marg= in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e= x">On 3/14/23, Jessica Clarke <<a href=3D"mailto:jrtc27@freebsd.org" tar= get=3D"_blank">jrtc27@freebsd.org</a>> wrote:<br> > On 14 Mar 2023, at 04:19, Warner Losh <imp@FreeBSD.org> wrote:<b= r> >><br> >> The branch main has been updated by imp:<br> >><br> >> URL:<br> >> <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21464d25= bc61f98968a5c1e76ab3c808ae4" rel=3D"noreferrer" target=3D"_blank">https://c= git.FreeBSD.org/src/commit/?id=3Dadeca21464d25bc61f98968a5c1e76ab3c808ae4</= a><br> >><br> >> commit adeca21464d25bc61f98968a5c1e76ab3c808ae4<br> >> Author:=C2=A0 =C2=A0 =C2=A0lucy <<a href=3D"mailto:seafork@disr= oot.org" target=3D"_blank">seafork@disroot.org</a>><br> >> AuthorDate: 2023-03-13 22:01:12 +0000<br> >> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org><br> >> CommitDate: 2023-03-14 04:19:24 +0000<br> >><br> >>=C2=A0 =C2=A0 Add GNU glibc compatible secure_getenv<br> >><br> >>=C2=A0 =C2=A0 Add mostly glibc and msl compatible secure_getenv. Re= turn NULL if<br> >>=C2=A0 =C2=A0 issetugid() indicates the process is tainted, otherwi= se getenv(x).<br> >> The<br> >>=C2=A0 =C2=A0 rational behind this is the fact that many Linux appl= ications use this<br> >>=C2=A0 =C2=A0 function instead of getenv() as it's widely consi= der a, "best<br> >>=C2=A0 =C2=A0 practice".<br> >><br> >>=C2=A0 =C2=A0 Reviewed by: imp, mjg (feedback)<br> >>=C2=A0 =C2=A0 Pull Request: <a href=3D"https://github.com/freebsd/f= reebsd-src/pull/686" rel=3D"noreferrer" target=3D"_blank">https://github.co= m/freebsd/freebsd-src/pull/686</a><br> >>=C2=A0 =C2=A0 Signed-off-by: Lucy Marsh <<a href=3D"mailto:seafo= rk@disroot.org" target=3D"_blank">seafork@disroot.org</a>><br> >> ---<br> >> include/stdlib.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 1 +<br> >> lib/libc/stdlib/Makefile.inc |=C2=A0 4 ++--<br> >> lib/libc/stdlib/Symbol.map=C2=A0 =C2=A0|=C2=A0 1 +<br> >> lib/libc/stdlib/getenv.3=C2=A0 =C2=A0 =C2=A0| 26 +++++++++++++++++= ++++++++-<br> >> lib/libc/stdlib/getenv.c=C2=A0 =C2=A0 =C2=A0| 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=C2=A0 =C2=A0 =C2=A0 qsort(void *, size_t,= size_t,<br> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int (* _Nonnull)(const void *, c= onst void *));<br> >> int=C2=A0 =C2=A0rand(void);<br> >> void *realloc(void *, size_t) __result_use_check __alloc_size(2);<= br> >> +char=C2=A0 =C2=A0 =C2=A0 =C2=A0 *secure_getenv(const char *);<br> >> void=C2=A0 srand(unsigned);<br> >> double=C2=A0 =C2=A0 =C2=A0 =C2=A0 strtod(const char * __restrict, = char ** __restrict);<br> >> float=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0strtof(const char * __restr= ict, char ** __restrict);<br> >> diff --git a/lib/libc/stdlib/Makefile.inc b/lib/libc/stdlib/Makefi= le.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+=3D=C2=A0 =C2=A0 =C2=A0 a64l.3 abort.3 abs.3= alloca.3 atexit.3 atof.3 \<br> >> MLINKS+=3Da64l.3 l64a.3 a64l.3 l64a_r.3<br> >> MLINKS+=3Datol.3 atoll.3<br> >> MLINKS+=3Dexit.3 _Exit.3<br> >> -MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 setenv.3= \<br> >> -=C2=A0 =C2=A0 getenv.3 unsetenv.3<br> >> +MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_g= etenv.3 \<br> >> +=C2=A0 =C2=A0 getenv.3 setenv.3 getenv.3 unsetenv.3<br> >> MLINKS+=3Dgetopt_long.3 getopt_long_only.3<br> >> MLINKS+=3Dhcreate.3 hdestroy.3 hcreate.3 hsearch.3<br> >> MLINKS+=3Dhcreate.3 hcreate_r.3 hcreate.3 hdestroy_r.3 hcreate.3<b= r> >> hsearch_r.3<br> >> diff --git a/lib/libc/stdlib/Symbol.map b/lib/libc/stdlib/Symbol.m= ap<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> >>=C2=A0 =C2=A0 =C2=A0 clearenv;<br> >>=C2=A0 =C2=A0 =C2=A0 qsort_r;<br> >> +=C2=A0 =C2=A0 secure_getenv;<br> >> };<br> >><br> >> FBSDprivate_1.0 {<br> >> diff --git a/lib/libc/stdlib/getenv.3 b/lib/libc/stdlib/getenv.3<b= r> >> index 5566d7b01dcd..93c0d2ada6ad 100644<br> >> --- a/lib/libc/stdlib/getenv.3<br> >> +++ b/lib/libc/stdlib/getenv.3<br> >> @@ -32,13 +32,14 @@<br> >> .\"=C2=A0 =C2=A0 =C2=A0@(#)getenv.3 8.2 (Berkeley) 12/11/93<b= r> >> .\" $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&qu= ot; "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=E2=80=99t 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></b= lockquote><div><br></div><div>=C2=A0<a href=3D"https://reviews.freebsd.org/= D39078">https://reviews.freebsd.org/D39078</a>; might address this.<br></div= ><div><br></div><div>Warner</div><div>=C2=A0</div><blockquote class=3D"gmai= l_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,20= 4,204);padding-left:1ex"> >> +.Fn secure_getenv<br> >> +wraps the<br> >> +.Fn getenv<br> >> +function to prevent it from being run in "secure execution&q= uot;.<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 w= ill 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<b= r> >> 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> >> +=C2=A0 =C2=A0 if (issetugid())<br> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;<br> >> +=C2=A0 =C2=A0 return getenv(name);<br> ><br> > return (...);<br> ><br> > (x2)<br> ><br> > Jess<br> ><br> >> +}<br> >> +<br> >> /*<br> >>=C2=A0 * Set the value of a variable.=C2=A0 Older settings are labe= led as inactive.<br> >> If an<br> >>=C2=A0 * older setting has enough room to store the new value, it w= ill be<br> >> reused.=C2=A0 No<br> ><br> ><br> <br> <br> -- <br> Mateusz Guzik <mjguzik <a href=3D"http://gmail.com" rel=3D"noreferrer" t= arget=3D"_blank">gmail.com</a>><br> </blockquote></div></div> --0000000000006edcdc05f6df3524--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoYsfij=MnrR4F47a0YHVgT28E_TWko60We99kME7eueQ>