From nobody Tue Mar 14 16:49:21 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 4Pbfcz35xPz3ySsC; Tue, 14 Mar 2023 16:49:23 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) (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 4Pbfcz1Bnmz4MMj; Tue, 14 Mar 2023 16:49:23 +0000 (UTC) (envelope-from mjguzik@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-oi1-x235.google.com with SMTP id bh20so12245783oib.9; Tue, 14 Mar 2023 09:49:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678812562; h=content-transfer-encoding:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Xm/Pvo/SQLcdj1KmNyCfkGNndOh5DuCaRsC/l0PksZA=; b=Pz6AhId3Kx1hoF84+uDZKmB8EZw0ywKZPriPU1e0KWyhPcvzaS54lIeelbk9gdx38J lR38g955JIqjJgBSlRWUqu9syQ6WHufCwNH2Vv40PPHRZu8mGkX2FDXImRhyzupGYaWe bj77uKy/kxwCcuFPSDOcKjPsTgW7m6z5FCopmvt42FMQQJrQOjhV7cp1JT850lvw4hRV c9d6pcZW/b6/JbAL3/tXkzjmgh0Zhm3G5VxBslNzkRAMqYy0xZ5FX/MP2PcCA14w97RT 7mT9rAIiSc37kFqyFYH85iNa+mwftbHXQb6nL6k8JMUqdgPnCTDXd/DEsFKr71Aj/yYg eTIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678812562; h=content-transfer-encoding:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Xm/Pvo/SQLcdj1KmNyCfkGNndOh5DuCaRsC/l0PksZA=; b=BxLZyuWpBSb16WoQVyHheUVZ6roe+AglRWscNH4pYUMq/RavqVXUkr7PcZMg1pzk+j FDZ1o5VclW72nf6VJ0MHVQk4zIL0HQFhMnz9vFXVVXXFKo2Alc/iMjlIkN23h01Yt9eg itFhAFsJxp7krARkzexYDWxd1yD78ql5n+GRDF9juodxXHuBwPsKk6dhXt8izQmuy7d/ 5qjZHNL3KBaTpMKuHDUbWhlLIKGHXG0prFGRDJzdDXkk/qS5t4U7uo6E9zGJJcEj8SHn mZ6SY25tsQblF8McZ4F+eBA2Jh2fGCpe4EAncQJhx2xiy/gAK7lM/sTvfTvDTJXmxoS6 fBAg== X-Gm-Message-State: AO0yUKWyGSAw4PkunXAQrTVrZIGL+jYpDoXMtOSFymAnDNHmkB7bLyzV njTHgsZLY52VGqt1pQ8yq3s6X/zkQUI3QIVUWe5xpi+b X-Google-Smtp-Source: AK7set8foDpQM+3z2BmG2VHsMPBJ2++eZ2s3AZqkso9dG/VAIAJ9gMURHJv+TJix0UpjyGbPetu7whWMwePhOtRZeN8= X-Received: by 2002:a54:4602:0:b0:384:3518:df33 with SMTP id p2-20020a544602000000b003843518df33mr13100347oip.4.1678812561996; Tue, 14 Mar 2023 09:49:21 -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 Received: by 2002:ac9:7087:0:b0:49c:b071:b1e3 with HTTP; Tue, 14 Mar 2023 09:49:21 -0700 (PDT) In-Reply-To: References: <202303140419.32E4Jtsd058392@gitrepo.freebsd.org> From: Mateusz Guzik Date: Tue, 14 Mar 2023 17:49:21 +0100 Message-ID: Subject: Re: git: adeca21464d2 - main - Add GNU glibc compatible secure_getenv To: Jessica Clarke Cc: Warner Losh , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4Pbfcz1Bnmz4MMj X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N 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=3Dadeca21464d25bc61f98968a5c1e76= ab3c808ae4 >> >> 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 thi= s >> 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.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?.. > 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. >> +.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 inactive. >> If an >> * older setting has enough room to store the new value, it will be >> reused. No > > --=20 Mateusz Guzik