From nobody Tue Mar 14 04:37:07 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4PbLNK2QF6z3xgfG for ; Tue, 14 Mar 2023 04:37:21 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PbLNJ67lfz48Sv for ; Tue, 14 Mar 2023 04:37:20 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x529.google.com with SMTP id eh3so1132295edb.11 for ; Mon, 13 Mar 2023 21:37:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; t=1678768639; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Mwr0+AsswWxSMiQJgYL//2oUnN543Z4lnmjmmfVcQs4=; b=mLPEBpgBLvzogrz2nMSnz2KENSgXoAv+0I2hcNjW3X2CyXXRDrNPxpImrnlvl75QqF KqmbTZ588j3JCMYOXFNVcfloElJh8xWsANKghq4ARQFFNM7flZuxKRiKdgvha+gHZugv 713pduuZCId1HokA2LET5DdSJtNrKlGO7rRlPsyELWPIJa4hcPy/wnR7Zaf45+15IuZP IlqS9gn+xv+3MVgACc1r4bFksq6/kjEk0xZhdX/5TdjJ9EbFyaEquU4Rzs7JwFepixDm FyaQ5/gV7GFJxzQV0ubbNDWIGR4B2aDZU4Uc9wt1twSOlPstSsNFbEOzp3UL8yzOfOTo zGNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678768639; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Mwr0+AsswWxSMiQJgYL//2oUnN543Z4lnmjmmfVcQs4=; b=IZ/q0ITh0TyZTKw70Zyt3M+1wXjkwSmmjJHpQsAE49Gtzx0j53dor/MLhYmj6WA4/2 BDoLUlFk1SfCj4VqarlmXunGEFkPbywqg2Cz8vE6AtpG8yA8uN+QK8S2oUqb8c8p6T19 RSXYrIJpdm1xRg9jgV/sHvsQV79muMttMI6vDNo5LEXXblqwt9LTuJYN1YgEt5WfSRI2 po3Q2K3NaUqgPuj7j6gOxQT+2RWvmOzYEY/9FBZhOnBvuZXmOaqtDNLE65Xqgbsg4azs H5GbeWZvYy4Stn+ZDmIuxZoOrFfwS11yFYdjm7wirp88fga2XpMT6vlOVWRePstXRUD/ V2Sw== X-Gm-Message-State: AO0yUKUveyUPuinBRlx/IQ42i3Kf4OnOVqrTPNOwk1g35cBooJeWY+CX luooUAWaed5+VdChe90A7DOTVKOqKcJjHrUWHqfHxQ== X-Google-Smtp-Source: AK7set+yblMnx9EZMgZZ0B5oHgG67sx4VeWYHlgFBkbz+HIuHBvDckJOv6txAKcs8kUCe3JoUYyHG5flig46tEAx/sA= X-Received: by 2002:a17:907:1b1f:b0:8b1:38d6:9853 with SMTP id mp31-20020a1709071b1f00b008b138d69853mr483418ejc.2.1678768639472; Mon, 13 Mar 2023 21:37:19 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Mon, 13 Mar 2023 22:37:07 -0600 Message-ID: Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv To: Jessica Clarke Cc: Warner Losh , src-committers , "" , dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000006a8b8305f6d4c819" X-Rspamd-Queue-Id: 4PbLNJ67lfz48Sv X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --0000000000006a8b8305f6d4c819 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Mar 13, 2023, 10:21 PM Jessica Clarke wrote: > On 14 Mar 2023, at 04:19, Warner Losh wrote: > > > > The branch main has been updated by imp: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21464d25bc61f98968a5c1e76a= b3c808ae4 > > > > commit adeca21464d25bc61f98968a5c1e76ab3c808ae4 > > Author: lucy > > AuthorDate: 2023-03-13 22:01:12 +0000 > > Commit: Warner Losh > > 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 th= is > > 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 > > --- > > 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.in= c > > 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. 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 > > --0000000000006a8b8305f6d4c819 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


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=3Dadeca21464d25bc61f98968a5c1e76= ab3c808ae4
>
> commit adeca21464d25bc61f98968a5c1e76ab3c808ae4
> Author:=C2=A0 =C2=A0 =C2=A0lucy <seafork@disroot.org>
> AuthorDate: 2023-03-13 22:01:12 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Warner Losh <imp@FreeBSD.org>
> CommitDate: 2023-03-14 04:19:24 +0000
>
>=C2=A0 =C2=A0 Add GNU glibc compatible secure_getenv
>
>=C2=A0 =C2=A0 Add mostly glibc and msl compatible secure_getenv. Return= NULL if
>=C2=A0 =C2=A0 issetugid() indicates the process is tainted, otherwise g= etenv(x).=C2=A0 The
>=C2=A0 =C2=A0 rational behind this is the fact that many Linux applicat= ions use this
>=C2=A0 =C2=A0 function instead of getenv() as it's widely consider = a, "best
>=C2=A0 =C2=A0 practice".
>
>=C2=A0 =C2=A0 Reviewed by: imp, mjg (feedback)
>=C2=A0 =C2=A0 Pull Request: https://gi= thub.com/freebsd/freebsd-src/pull/686
>=C2=A0 =C2=A0 Signed-off-by: Lucy Marsh <seafork@disroot.org>= ;
> ---
> include/stdlib.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 1 +
> lib/libc/stdlib/Makefile.inc |=C2=A0 4 ++--
> lib/libc/stdlib/Symbol.map=C2=A0 =C2=A0|=C2=A0 1 +
> lib/libc/stdlib/getenv.3=C2=A0 =C2=A0 =C2=A0| 26 +++++++++++++++++++++= ++++-
> lib/libc/stdlib/getenv.c=C2=A0 =C2=A0 =C2=A0| 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=C2=A0 =C2=A0 =C2=A0 =C2=A0qsort(void *, size_= t, size_t,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int (* _Nonnull)(const void *,= const void *));
> int=C2=A0 =C2=A0 rand(void);
> void=C2=A0 *realloc(void *, size_t) __result_use_check __alloc_size(2)= ;
> +char *secure_getenv(const char *);
> void=C2=A0 =C2=A0srand(unsigned);
> double=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0strtod(const char * __restrict= , char ** __restrict);
> float=C2=A0 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=C2=A0 =C2=A0 =C2=A0 =C2=A0a64l.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 \ > -=C2=A0 =C2=A0 =C2=A0getenv.3 unsetenv.3
> +MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_geten= v.3 \
> +=C2=A0 =C2=A0 =C2=A0getenv.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 hsear= ch_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 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0clearenv;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0qsort_r;
> +=C2=A0 =C2=A0 =C2=A0secure_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 @@
> .\"=C2=A0 =C2=A0 =C2=A0@(#)getenv.3=C2=A0 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...

<= div dir=3D"auto">
> +.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)
> +{
> +=C2=A0 =C2=A0 =C2=A0if (issetugid())
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;
> +=C2=A0 =C2=A0 =C2=A0return 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= .

Though I'll see ab= out adding git clang-format setup to check. I wrote a stupidly simple scrip= t to check 2 years ago and the commits coming in at the time had way more e= xceptions 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.
<= div dir=3D"auto">
Warner=C2=A0

Jess

> +}
> +
> /*
>=C2=A0 * Set the value of a variable.=C2=A0 Older settings are labeled = as inactive.=C2=A0 If an
>=C2=A0 * older setting has enough room to store the new value, it will = be reused.=C2=A0 No

--0000000000006a8b8305f6d4c819--