This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[Patch] Define secure_getenv (was: Re: [Patch] Document __secure_getenv)
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:
$ 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?
(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.)
--
Florian Weimer / Red Hat Product Security Team
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.
+Modifications of environment variables are not allowed in
+multi-threaded programs. The @code{getenv} function can be safely
+used in multi-threaded programs.
+
@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
+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;
+ }
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
@@ -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
#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
@@ -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
@@ -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
@@ -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
@@ -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);