From nobody Tue Mar 14 04:45:05 2023 X-Original-To: dev-commits-src-main@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 4PbLYJ2Blpz3xhQk for ; Tue, 14 Mar 2023 04:45:08 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (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 4PbLYJ0N5Zz4BLJ for ; Tue, 14 Mar 2023 04:45:08 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wm1-f49.google.com with SMTP id bh21-20020a05600c3d1500b003ed1ff06fb0so3807931wmb.3 for ; Mon, 13 Mar 2023 21:45:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678769106; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=vgeKdlgMAEslWEtZhVzLVG7D72fW8krx49x0Ou4A+UA=; b=q3KLL/FdZymgtCGUiDBPysmRgfKtEtr+5xw3vccuLBI0lOVyr8Vm9IuiqBgmMaCTYb FwYdGD7Pq2IAUMM/HSrYEXo/xl6i14aU5+xmnYa0qs7bmQCI+tyf2hMqC22Vy0PgXMGR 4KOFRLfI+gMzpitdt1ESQEvSVC9ccgIiKmPmzrZSrey9dscAVXhFxS5N1FZMGHqiFwOO e6D6/TEwUhwkk1vlZg9MBupqE1W9QsNUo/EZ405dJPhIDgB6ZQzi/zszuiPQic95BGzK xf+lkHfcXbVa5b6Dz3VOUcSLnJLN33TaVfTDoWrRhkpGQMgr6En/4eIalQGrJlb5YkQ4 OqbA== X-Gm-Message-State: AO0yUKXjRJvlW7kQmftZn+ccWClxyYA+GVfdiUnF1it4DZiYTlnGqz2Q 81KxUv5pXHfA0D6QQBXWSuRqUg== X-Google-Smtp-Source: AK7set+q2xQ4uxY50eW1QGDuWHNdN1GypGZ9/SZAdJ9OwiamrL0dBr/m275YeDzO+iuyBdHw3B/iLw== X-Received: by 2002:a1c:f60a:0:b0:3eb:3947:41ea with SMTP id w10-20020a1cf60a000000b003eb394741eamr13543305wmc.37.1678769106161; Mon, 13 Mar 2023 21:45:06 -0700 (PDT) Received: from smtpclient.apple (global-5-144.n-2.net.cam.ac.uk. [131.111.5.144]) by smtp.gmail.com with ESMTPSA id c24-20020a7bc018000000b003ed2357f7f1sm1495540wmb.37.2023.03.13.21.45.05 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Mar 2023 21:45:05 -0700 (PDT) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv From: Jessica Clarke In-Reply-To: Date: Tue, 14 Mar 2023 04:45:05 +0000 Cc: Warner Losh , src-committers , "" , dev-commits-src-main@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> To: Warner Losh X-Mailer: Apple Mail (2.3696.120.41.1.1) X-Rspamd-Queue-Id: 4PbLYJ0N5Zz4BLJ X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N On 14 Mar 2023, at 04:37, Warner Losh wrote: >=20 >=20 >=20 > On Mon, Mar 13, 2023, 10:21 PM Jessica Clarke = wrote: > On 14 Mar 2023, at 04:19, Warner Losh wrote: > >=20 > > The branch main has been updated by imp: > >=20 > > URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dadeca21464d25bc61f98968a5c1e76ab= 3c808ae4 > >=20 > > commit adeca21464d25bc61f98968a5c1e76ab3c808ae4 > > Author: lucy > > AuthorDate: 2023-03-13 22:01:12 +0000 > > Commit: Warner Losh > > CommitDate: 2023-03-14 04:19:24 +0000 > >=20 > > Add GNU glibc compatible secure_getenv > >=20 > > 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". > >=20 > > 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(-) > >=20 > > 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; > > }; > >=20 > > 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, >=20 > I don=E2=80=99t think it is now?.. >=20 > Yes... >=20 > > +.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) > > } > >=20 > >=20 > > +/* > > + * 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); >=20 > return (...); >=20 > (x2) >=20 > 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 = should rather than must. 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. >=20 > Warner=20 >=20 > Jess >=20 > > +} > > + > > /* > > * 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