From nobody Tue Mar 14 17:03:27 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 4PbfxS3NQnz3yTqt for ; Tue, 14 Mar 2023 17:03:40 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) (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 4PbfxS2V1pz4NB6 for ; Tue, 14 Mar 2023 17:03:40 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x52e.google.com with SMTP id x13so17477985edd.1 for ; Tue, 14 Mar 2023 10:03:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20210112.gappssmtp.com; s=20210112; t=1678813418; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=T5wLKQc33HlwcbZlJ2+NSNPgaMXFkxTp/k7UBMRR+Ck=; b=NOpc8kHiNwy96g9v/0Tv1N9GxuN3FYRJ3ZbAXT3HUYfzjMAgKmMwE+4wkURhSXAUjD XiTilg8LDsGZfHIKpmpLQfDo18fW7jimQW97l9e5kakDzTzpdUeumk3ObGHYSXwCgrM/ d8ueb0c5CxFQrETyabodrmTrdXVUqo4rlJBHSpaz3W3UoI+shecqNog4lFWg7gyMlfsh /MzPUhgtcNxh9K6TFTPRAM0Htm4Y6qZjrBaCu9r/JhVJ1ZVA05rRZUZ7bUTRV3AlEwYX T+AiMc8s8GyrdaezKt/QVY5KaqjWLCR2n8LwpluFc+sdH+/ppHiFMtb3hLkiXU91W3RI v4xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678813418; 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=T5wLKQc33HlwcbZlJ2+NSNPgaMXFkxTp/k7UBMRR+Ck=; b=NXys2U63xjR/6gKYlX9e8HAcLUCtJypUddG9/bgun7o73sWn/eEWOPRNogfdYrotcG m50LY0isx+2cPm/PMQOwyu2Wzz1+WYC+0MO3IA9Iv+4ZwUuQKekehztRz+jjMxGFNX45 /ESnDzy7hGM9pFc+QjI+MfScY7zGTsM+jhk9JeVFuvICc6/EPeVgXy2giDFubPxYt8nB wPAZGN+kaSOCVk4EU6AK5zG0gaUyAWUQZs8VnUqWPKYBXOJCnTvbMu6wcCVdG02TQ79s U29UkXGYI9dqxJ9AAHosrawcTHlXqDP5jBJMGnor/7g4+bAkD4DeWXfv87PDtgRFrdkQ jzKg== X-Gm-Message-State: AO0yUKULxN2KyOWGTbbFXpRMf7jj7bkQZsTdWWf/F9YwVbiVt6SSVlHb ndhjdc420AJ1xTiMHo57qZpDKgF92nyW1FKX4f4idJYEsN7k/ez0ob4= X-Google-Smtp-Source: AK7set/YJ6vtqfoPYeXMw1sVvhGjFWZhsQ42LD9ObJkNcjoTpOFj/gViPmDCApFQ0c5PFyWGg2uy6j1LQc5Vgw4ANM4= X-Received: by 2002:a17:907:72c1:b0:8e5:1a7b:8ab2 with SMTP id du1-20020a17090772c100b008e51a7b8ab2mr1887680ejc.4.1678813418145; Tue, 14 Mar 2023 10:03:38 -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: Tue, 14 Mar 2023 11:03:27 -0600 Message-ID: Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv To: Mateusz Guzik Cc: Jessica Clarke , Warner Losh , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Type: multipart/alternative; boundary="0000000000006edcdc05f6df3524" X-Rspamd-Queue-Id: 4PbfxS2V1pz4NB6 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 --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 = wrote: > On 3/14/23, 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 > 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 > >> --- > >> 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 > --0000000000006edcdc05f6df3524 Content-Type: text/html; 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://c= git.FreeBSD.org/src/commit/?id=3Dadeca21464d25bc61f98968a5c1e76ab3c808ae4
>>
>> 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. Re= turn NULL if
>>=C2=A0 =C2=A0 issetugid() indicates the process is tainted, otherwi= se getenv(x).
>> The
>>=C2=A0 =C2=A0 rational behind this is the fact that many Linux appl= ications use this
>>=C2=A0 =C2=A0 function instead of getenv() as it's widely consi= der a, "best
>>=C2=A0 =C2=A0 practice".
>>
>>=C2=A0 =C2=A0 Reviewed by: imp, mjg (feedback)
>>=C2=A0 =C2=A0 Pull Request: https://github.co= m/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 qsort(void *, size_t,= size_t,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int (* _Nonnull)(const void *, c= onst void *));
>> int=C2=A0 =C2=A0rand(void);
>> 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 *);
>> void=C2=A0 srand(unsigned);
>> double=C2=A0 =C2=A0 =C2=A0 =C2=A0 strtod(const char * __restrict, = char ** __restrict);
>> float=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0strtof(const char * __restr= ict, char ** __restrict);
>> diff --git a/lib/libc/stdlib/Makefile.inc b/lib/libc/stdlib/Makefi= le.inc
>> 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 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= \
>> -=C2=A0 =C2=A0 getenv.3 unsetenv.3
>> +MLINKS+=3Dgetenv.3 clearenv.3 getenv.3 putenv.3 getenv.3 secure_g= etenv.3 \
>> +=C2=A0 =C2=A0 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.m= ap
>> 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 clearenv;
>>=C2=A0 =C2=A0 =C2=A0 qsort_r;
>> +=C2=A0 =C2=A0 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 @@
>> .\"=C2=A0 =C2=A0 =C2=A0@(#)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&qu= ot; "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.

=C2=A0https://reviews.freebsd.org/D39078 might address this.

Warner
=C2=A0
>> +.Fn secure_getenv
>> +wraps the
>> +.Fn getenv
>> +function to prevent it from being run in "secure execution&q= uot;.
>> +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 w= ill 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 if (issetugid())
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;
>> +=C2=A0 =C2=A0 return getenv(name);
>
> return (...);
>
> (x2)
>
> Jess
>
>> +}
>> +
>> /*
>>=C2=A0 * Set the value of a variable.=C2=A0 Older settings are labe= led as inactive.
>> If an
>>=C2=A0 * older setting has enough room to store the new value, it w= ill be
>> reused.=C2=A0 No
>
>


--
Mateusz Guzik <mjguzik gmail.com>
--0000000000006edcdc05f6df3524--