This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [Patch] Define secure_getenv
On 7/13/2012 9:35 AM, Florian Weimer wrote:
> On 07/13/2012 11:52 AM, Andreas Schwab wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> I don't want to use secure_getenv for the public symbol because I think I
>>> need three symbols: __secure_getenv (public, but without a default
>>> version) for backwards compatibility, getenv_secure (public) for the new
>>> interface, and __getenv_secure (GLIBC_PRIVATE) for the internal
>>> cross-references, both within libc itself and from libnss_hesiod and
>>> others.
>>
>> You don't need the third name, a symbol can have more than one version.
>> Also, libc internal references are resolved internally via the hidden
>> mechanism.
>
> I figured out what I was doing wrong. I didn't include <shlib-compat.h> and
>
> compat_symbol (libc, secure_getenv, __secure_getenv, GLIBC_2_0);
>
> was parsed as a function declaration.
>
> Anyway, I couldn't get a GLIBC_PRIVATE symbol for __secure_getenv. So I've renamed all the internal calls and ended up with:
You don't need GLIBC_PRIVATE since you have a public interface now and libc hidden takes care of any internal non-plt references.
> $ readelf -s --wide libc.so | grep secure_getenv
> 851: 00000000000378c0 27 FUNC GLOBAL DEFAULT 12 __secure_getenv@GLIBC_2.2.5
> 1702: 00000000000378c0 27 FUNC GLOBAL DEFAULT 12 secure_getenv@@GLIBC_2.16
> 4649: 00000000000378c0 27 FUNC LOCAL DEFAULT 12 __GI_secure_getenv
> 6819: 00000000000378c0 27 FUNC GLOBAL DEFAULT 12 __secure_getenv@GLIBC_2.2.5
> 6843: 00000000000378c0 27 FUNC GLOBAL DEFAULT 12 secure_getenv
> $ readelf -s --wide hesiod/libnss_hesiod.so | grep secure_getenv
> 25: 0000000000000000 0 FUNC GLOBAL DEFAULT UND secure_getenv@GLIBC_2.16 (7)
> 119: 0000000000000000 0 FUNC GLOBAL DEFAULT UND secure_getenv@@GLIBC_2.16
>
> Is this acceptable?
Thanks for doing this work!
> (I'm using GLIBC_2.16 for testing because there isn't a GLIBC_2.17 or GLIBC_2.18 on the trunk. Eventually, it will be GLIBC_2.18.)
Please test with trunk and repost the final patch along with the changes below.
> 2012-07-13 Florian Weimer <fweimer@redhat.com>
>
> * stdlib/stdlib.h: Rename __secure_getenv to secure_getenv.
> * include/stdlib.h: Likewise.
> * stdlib/secure-getenv.c: Likewise. Add compatibility symbol.
> * stdlib/Versions: Export secure_getenv.
> * manual/startup.texi (Environment Access): Document
> secure_getenv.
> * hesiod/hesiod.c (hesiod_init): Switch to secure_getenv.
> * malloc/mtrace.c (mtrace): Likewise.
> * inet/ruserpass.c (ruserpass): Likewise.
> * sysdeps/mach/hurd/tmpfile.c (__tmpfile): Likewise.
> * sysdeps/posix/libc_fatal.c (__libc_message): Likewise.
> * sysdeps/posix/sysconf.c (__sysconf_check_spec): Likewise.
> * sysdeps/unix/sysv/linux/libc_fatal.c (__libc_message): Likewise.
> * sysdeps/posix/tempname.c (secure_getenv): Likewise.
> Check for HAVE_SECURE_GETENV and HAVE___SECURE_GETENV.
>
> diff --git a/hesiod/hesiod.c b/hesiod/hesiod.c
> index a3f22e5..75d83be 100644
> --- a/hesiod/hesiod.c
> +++ b/hesiod/hesiod.c
> @@ -87,7 +87,7 @@ hesiod_init(void **context) {
> ctx->classes[0] = C_IN;
> ctx->classes[1] = C_HS;
>
> - configname = __secure_getenv("HESIOD_CONFIG");
> + configname = secure_getenv("HESIOD_CONFIG");
> if (!configname)
> configname = _PATH_HESIOD_CONF;
> if (parse_config_file(ctx, configname) < 0) {
> @@ -109,7 +109,7 @@ hesiod_init(void **context) {
> * The default RHS can be overridden by an environment
> * variable.
> */
> - if ((cp = __secure_getenv("HES_DOMAIN")) != NULL) {
> + if ((cp = secure_getenv("HES_DOMAIN")) != NULL) {
> free(ctx->RHS);
> ctx->RHS = malloc(strlen(cp)+2);
> if (!ctx->RHS)
> diff --git a/include/stdlib.h b/include/stdlib.h
> index de0b414..c3315ce 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -33,12 +33,12 @@ libc_hidden_proto (__strtold_l)
> libc_hidden_proto (exit)
> libc_hidden_proto (abort)
> libc_hidden_proto (getenv)
> +libc_hidden_proto (secure_getenv)
> libc_hidden_proto (bsearch)
> libc_hidden_proto (qsort)
> libc_hidden_proto (qsort_r)
> libc_hidden_proto (lrand48_r)
> libc_hidden_proto (wctomb)
> -libc_hidden_proto (__secure_getenv)
>
> extern long int __random (void);
> extern void __srandom (unsigned int __seed);
> diff --git a/inet/ruserpass.c b/inet/ruserpass.c
> index df423ba..9c5c03f 100644
> --- a/inet/ruserpass.c
> +++ b/inet/ruserpass.c
> @@ -102,7 +102,7 @@ ruserpass(host, aname, apass)
> int t, usedefault = 0;
> struct stat64 stb;
>
> - hdir = __secure_getenv("HOME");
> + hdir = secure_getenv("HOME");
> if (hdir == NULL) {
> /* If we can't get HOME, fail instead of trying ".",
> which is no improvement. This really should call
> diff --git a/malloc/mtrace.c b/malloc/mtrace.c
> index d1049c9..bd2ce09 100644
> --- a/malloc/mtrace.c
> +++ b/malloc/mtrace.c
> @@ -309,7 +309,7 @@ mtrace ()
> /* When compiling the GNU libc we use the secure getenv function
> which prevents the misuse in case of SUID or SGID enabled
> programs. */
> - mallfile = __secure_getenv (mallenv);
> + mallfile = secure_getenv (mallenv);
> #else
> mallfile = getenv (mallenv);
> #endif
> diff --git a/manual/startup.texi b/manual/startup.texi
> index 0420e93..9be6df8 100644
> --- a/manual/startup.texi
> +++ b/manual/startup.texi
> @@ -310,11 +310,15 @@ character, since this is assumed to terminate the string.
>
> The value of an environment variable can be accessed with the
> @code{getenv} function. This is declared in the header file
> -@file{stdlib.h}. Modifications of enviroment variables are not
> -allowed in Multi-threaded programs. The @code{getenv} function
> -can be safely used in multi-threaded programs
> +@file{stdlib.h}.
> @pindex stdlib.h
>
> +Libraries should use @code{secure_getenv} instead of @code{getenv},
> +so that they do not accidentally use entrusted environment variables.
Same typo already pointed out Chris Metcalf: "entrusted"=>"unstructed"
> +Modifications of environment variables are not allowed in
> +multi-threaded programs. The @code{getenv} function can be safely
> +used in multi-threaded programs.
> +
Can "seccure_getenv" be safely used in an m-t context?
> @comment stdlib.h
> @comment ISO
> @deftypefun {char *} getenv (const char *@var{name})
> @@ -326,6 +330,18 @@ environment variable @var{name} is not defined, the value is a null
> pointer.
> @end deftypefun
>
> +@comment stdlib.h
> +@comment GNU
> +@deftypefun {char *} secure_getenv (const char *@var{name})
> +This function is similar to @code{getenv}, but it returns a null
> +pointer if the environment is untrusted. This happens when the
> +program file has SUID or SGID bits set. General-purpose libraries
> +should always prefer this function over @code{getenv}, to avoid
No comma.
> +vulnerabilities if the library is referenced from a SUID/SGID program.
> +
> +This function is a GNU extension.
> +@end deftypefun
> +
> @comment stdlib.h
> @comment SVID
> diff --git a/stdlib/Versions b/stdlib/Versions
> index 2aa396e..d817d10 100644
> --- a/stdlib/Versions
> +++ b/stdlib/Versions
> @@ -6,7 +6,7 @@ libc {
> # functions used in inline functions or macros
> __strto*_internal;
>
> - # functions used in other libraries
> + # compatibility symbol
> __secure_getenv;
>
> # a*
> @@ -103,6 +103,9 @@ libc {
> GLIBC_2.13 {
> __fentry__;
> }
> + GLIBC_2.16 {
> + secure_getenv;
> + }
You need to fix this to work with trunk.
> GLIBC_PRIVATE {
> # functions which have an additional interface since they are
> # are cancelable.
> diff --git a/stdlib/secure-getenv.c b/stdlib/secure-getenv.c
> index f64759f..bedc5be 100644
> --- a/stdlib/secure-getenv.c
> +++ b/stdlib/secure-getenv.c
Update copyright years.
> @@ -18,13 +18,19 @@
> #include <stdlib.h>
> #include <unistd.h>
>
> +#include <shlib-compat.h>
> +
> /* Some programs and especially the libc itself have to be careful
> what values to accept from the environment. This special version
> checks for SUID or SGID first before doing any work. */
> char *
> -__secure_getenv (name)
> +secure_getenv (name)
> const char *name;
> {
> return __libc_enable_secure ? NULL : getenv (name);
> }
> -libc_hidden_def (__secure_getenv)
> +libc_hidden_def (secure_getenv)
> +
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_16)
> +compat_symbol (libc, secure_getenv, __secure_getenv, GLIBC_2_0);
> +#endif
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index f652eda..cf3f39c 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -568,10 +568,12 @@ __BEGIN_NAMESPACE_STD
> extern char *getenv (const char *__name) __THROW __nonnull ((1)) __wur;
> __END_NAMESPACE_STD
>
> +#ifdef __USE_GNU
> /* This function is similar to the above but returns NULL if the
> programs is running with SUID or SGID enabled. */
> -extern char *__secure_getenv (const char *__name)
> +extern char *secure_getenv (const char *__name)
> __THROW __nonnull ((1)) __wur;
> +#endif
Perfect :-)
This meets Roland's requirement for "proper public declaration."
> #if defined __USE_SVID || defined __USE_XOPEN
> /* The SVID says this is in <stdio.h>, but this seems a better place. */
> diff --git a/sysdeps/mach/hurd/tmpfile.c b/sysdeps/mach/hurd/tmpfile.c
> index 94b1380..9ae138b 100644
> --- a/sysdeps/mach/hurd/tmpfile.c
> +++ b/sysdeps/mach/hurd/tmpfile.c
> @@ -37,7 +37,7 @@ __tmpfile (void)
> FILE *f;
>
> /* Get a port to the directory that will contain the file. */
> - const char *dirname = __secure_getenv ("TMPDIR") ?: P_tmpdir;
> + const char *dirname = secure_getenv ("TMPDIR") ?: P_tmpdir;
> file_t dir = __file_name_lookup (dirname, 0, 0);
> if (dir == MACH_PORT_NULL)
> return NULL;
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index 62acb9b..d4a36fe 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
Update copyright.
> @@ -61,7 +61,7 @@ __libc_message (int do_abort, const char *fmt, ...)
>
> /* Open a descriptor for /dev/tty unless the user explicitly
> requests errors on standard error. */
> - const char *on_2 = __secure_getenv ("LIBC_FATAL_STDERR_");
> + const char *on_2 = secure_getenv ("LIBC_FATAL_STDERR_");
> if (on_2 == NULL || *on_2 == '\0')
> fd = open_not_cancel_2 (_PATH_TTY, O_RDWR | O_NOCTTY | O_NDELAY);
>
> diff --git a/sysdeps/posix/sysconf.c b/sysdeps/posix/sysconf.c
> index 1f988d5..d3da56d 100644
> --- a/sysdeps/posix/sysconf.c
> +++ b/sysdeps/posix/sysconf.c
Update copyright.
> @@ -1259,7 +1259,7 @@ __sysconf_check_spec (const char *spec)
> {
> int save_errno = errno;
>
> - const char *getconf_dir = __secure_getenv ("GETCONF_DIR") ?: GETCONF_DIR;
> + const char *getconf_dir = secure_getenv ("GETCONF_DIR") ?: GETCONF_DIR;
> size_t getconf_dirlen = strlen (getconf_dir);
> size_t speclen = strlen (spec);
>
> diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
> index a98f1d6..0a8270e 100644
> --- a/sysdeps/posix/tempname.c
> +++ b/sysdeps/posix/tempname.c
Update copyright.
> @@ -101,8 +101,12 @@
> # define __xstat64(version, path, buf) stat (path, buf)
> #endif
>
> -#if ! (HAVE___SECURE_GETENV || _LIBC)
> -# define __secure_getenv getenv
> +#if ! (HAVE_SECURE_GETENV || _LIBC)
> +# ifdef HAVE___SECURE_GETENV
> +# define secure_getenv __secure_getenv
> +# else
> +# define secure_getenv getenv
> +# endif
> #endif
>
> #ifdef _LIBC
> @@ -168,7 +172,7 @@ __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
>
> if (try_tmpdir)
> {
> - d = __secure_getenv ("TMPDIR");
> + d = secure_getenv ("TMPDIR");
> if (d != NULL && direxists (d))
> dir = d;
> else if (dir != NULL && direxists (dir))
> diff --git a/sysdeps/unix/sysv/linux/libc_fatal.c b/sysdeps/unix/sysv/linux/libc_fatal.c
> index 58a5a7f8..3cb40f0 100644
> --- a/sysdeps/unix/sysv/linux/libc_fatal.c
> +++ b/sysdeps/unix/sysv/linux/libc_fatal.c
Update copyright.
> @@ -64,7 +64,7 @@ __libc_message (int do_abort, const char *fmt, ...)
>
> /* Open a descriptor for /dev/tty unless the user explicitly
> requests errors on standard error. */
> - const char *on_2 = __secure_getenv ("LIBC_FATAL_STDERR_");
> + const char *on_2 = secure_getenv ("LIBC_FATAL_STDERR_");
> if (on_2 == NULL || *on_2 == '\0')
> fd = open_not_cancel_2 (_PATH_TTY, O_RDWR | O_NOCTTY | O_NDELAY);
>
Cheers,
Carlos.
--
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026