Date: Mon, 13 Mar 2023 23:03:33 -0600 From: Warner Losh <imp@bsdimp.com> To: Jessica Clarke <jrtc27@freebsd.org> 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: <CANCZdfp-r8H4QYsFWgdWzgZCDcRGztWm3Mmbs0qo36TLARX45A@mail.gmail.com> In-Reply-To: <BE0076A3-EF34-461F-805A-E193372415B3@freebsd.org> References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> <E40E7886-0EFC-4538-887F-ED067CB6FFFB@freebsd.org> <CANCZdfq3_3xS1tac6t6db-rx%2B0%2Ba8tCvGjmH0Rt=Sh9=Y7T2bQ@mail.gmail.com> <BE0076A3-EF34-461F-805A-E193372415B3@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000dcc71305f6d52663 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=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.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; > > > }; > > > > > > 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?.. > > > > 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=E2=80=99t an RFC SHOULD, much of style(9) is sho= uld > 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 > > --000000000000dcc71305f6d52663 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"auto"><div><br><br><div class=3D"gmail_quote"><div dir=3D"ltr" = class=3D"gmail_attr">On Mon, Mar 13, 2023, 10:45 PM Jessica Clarke <<a h= ref=3D"mailto:jrtc27@freebsd.org" target=3D"_blank" rel=3D"noreferrer">jrtc= 27@freebsd.org</a>> wrote:<br></div><blockquote class=3D"gmail_quote" st= yle=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 14= Mar 2023, at 04:37, Warner Losh <<a href=3D"mailto:imp@bsdimp.com" rel= =3D"noreferrer noreferrer" target=3D"_blank">imp@bsdimp.com</a>> wrote:<= br> > <br> > <br> > <br> > On Mon, Mar 13, 2023, 10:21 PM Jessica Clarke <<a href=3D"mailto:jr= tc27@freebsd.org" rel=3D"noreferrer noreferrer" target=3D"_blank">jrtc27@fr= eebsd.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: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21= 464d25bc61f98968a5c1e76ab3c808ae4" rel=3D"noreferrer noreferrer noreferrer"= target=3D"_blank">https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21464d25b= c61f98968a5c1e76ab3c808ae4</a><br> > > <br> > > commit adeca21464d25bc61f98968a5c1e76ab3c808ae4<br> > > Author:=C2=A0 =C2=A0 =C2=A0lucy <<a href=3D"mailto:seafork@dis= root.org" rel=3D"noreferrer noreferrer" target=3D"_blank">seafork@disroot.o= rg</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. R= eturn NULL if<br> > >=C2=A0 =C2=A0 issetugid() indicates the process is tainted, otherw= ise getenv(x).=C2=A0 The<br> > >=C2=A0 =C2=A0 rational behind this is the fact that many Linux app= lications use this<br> > >=C2=A0 =C2=A0 function instead of getenv() as it's widely cons= ider 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/= freebsd-src/pull/686" rel=3D"noreferrer noreferrer noreferrer" target=3D"_b= lank">https://github.com/freebsd/freebsd-src/pull/686</a><br>; > >=C2=A0 =C2=A0 Signed-off-by: Lucy Marsh <<a href=3D"mailto:seaf= ork@disroot.org" rel=3D"noreferrer noreferrer" target=3D"_blank">seafork@di= sroot.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 =C2=A0qsort(void *, = size_t, size_t,<br> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int (* _Nonnull)(const vo= id *, const void *));<br> > > int=C2=A0 =C2=A0 rand(void);<br> > > void=C2=A0 *realloc(void *, size_t) __result_use_check __alloc_si= ze(2);<br> > > +char *secure_getenv(const char *);<br> > > void=C2=A0 =C2=A0srand(unsigned);<br> > > double=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0strtod(const char * __res= trict, char ** __restrict);<br> > > float=C2=A0 strtof(const char * __restrict, char ** __restrict);<= br> > > diff --git a/lib/libc/stdlib/Makefile.inc b/lib/libc/stdlib/Makef= ile.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 =C2=A0a64l.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 =C2=A0getenv.3 unsetenv.3<br> > > +MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_= getenv.3 \<br> > > +=C2=A0 =C2=A0 =C2=A0getenv.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 = 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> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0clearenv;<br> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0qsort_r;<br> > > +=C2=A0 =C2=A0 =C2=A0secure_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> > > .\"=C2=A0 =C2=A0 =C2=A0@(#)getenv.3=C2=A0 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&q= uot; "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> > Yes...<br> > <br> > > +.Fn secure_getenv<br> > > +wraps the<br> > > +.Fn getenv<br> > > +function to prevent it from being run in "secure execution&= quot;.<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 changes, in<br> > > + * which case it will return NULL.<br> > > + */<br> > > +char *<br> > > +secure_getenv(const char *name)<br> > > +{<br> > > +=C2=A0 =C2=A0 =C2=A0if (issetugid())<br> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;<br> > > +=C2=A0 =C2=A0 =C2=A0return getenv(name);<br> > <br> > return (...);<br> > <br> > (x2)<br> > <br> > 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 p= r.<br> <br> I still see "Values in return statements should be enclosed in<br> parentheses."? Which isn=E2=80=99t an RFC SHOULD, much of style(9) is = should<br> rather than must.<br></blockquote></div></div><div dir=3D"auto"><br></div><= div dir=3D"auto">We need to do a pass to use rfc language. In this case,=C2= =A0 thought it's been encouraged but not required for a long time. I ad= ded the explicit language years ago since it was only by example prior to t= hat. I thought this was one of the many lossenings we've had over the y= ears, but if so there's been no update since I added the should languag= e... I know I've committed a lot of new code without it over the years.= ..</div><div dir=3D"auto"><br></div><div dir=3D"auto">It may just have been= a combo of many other projects forbidding the practice and it was one of t= he variations my script found... I'll have to go find the script to see= .</div><div dir=3D"auto"><br></div><div dir=3D"auto">But like I said before= ... git clang-format...</div><div dir=3D"auto"><br></div><div dir=3D"auto">= Warner</div><div dir=3D"auto"><br></div><div dir=3D"auto"><div class=3D"gma= il_quote"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;bord= er-left:1px #ccc solid;padding-left:1ex"> Jess<br> <br> > Though I'll see about adding git clang-format setup to check. I wr= ote 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 ea= sily enough I think.<br> > <br> > Warner <br> > <br> > Jess<br> > <br> > > +}<br> > > +<br> > > /*<br> > >=C2=A0 * Set the value of a variable.=C2=A0 Older settings are lab= eled as inactive.=C2=A0 If an<br> > >=C2=A0 * older setting has enough room to store the new value, it = will be reused.=C2=A0 No<br> <br> </blockquote></div></div></div> --000000000000dcc71305f6d52663--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfp-r8H4QYsFWgdWzgZCDcRGztWm3Mmbs0qo36TLARX45A>