| Submitter | Theodore Ts'o |
|---|---|
| Date | Jan. 22, 2013, 12:09 a.m. |
| Message ID | <1358813390-26466-1-git-send-email-tytso@mit.edu> |
| Download | mbox | patch |
| Permalink | /patch/214295/ |
| State | Accepted |
| Headers | show |
Comments
* Theodore Ts'o: > +#if HAVE___SECURE_GETENV > + return __secure_getenv(arg); > +#else > + return getenv(arg); > +#endif glibc 2.17 has secure_getenv, but not __secure_getenv. Unfortuantely, this was the only way to turn this into an official interface. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 23, 2013 at 01:17:25PM +0100, Florian Weimer wrote: > > glibc 2.17 has secure_getenv, but not __secure_getenv. Unfortuantely, > this was the only way to turn this into an official interface. Thanks for pointing this out. I'm using Debian testing which is still using glibc 2.13. The bigger issue is that it's not just spd_readdir.c (which is in contrib and so it's not compiled from the makefile), but we are using __secure_getenv() in libext2fs and other libraries in e2fprogs. Use of __secure_getenv() is protected with a configure.in test, so we won't break when we compile under glibc 2.17, but we won't have the full benefit of using secure_getenv(), either. We use a similar safe_getenv() code which will skip the getenv if the process is running setuid, or PR_GET_DUMPABLE returns 0, so hopefully that prevents us against the worst of the security exposure, but as [1] states, "such emulation is necessarily brittle". [1] http://sourceware.org/glibc/wiki/Tips_and_Tricks/secure_getenv Can someone send me a patch, please? Or I'll put it on my todo list.... - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
diff --git a/contrib/spd_readdir.c b/contrib/spd_readdir.c index f89832c..30c01b3 100644 --- a/contrib/spd_readdir.c +++ b/contrib/spd_readdir.c @@ -27,6 +27,10 @@ #define MAX_DIRSIZE 0 #define DEBUG +/* Util we autoconfiscate spd_readdir... */ +#define HAVE___SECURE_GETENV 1 +#define HAVE_PRCTL 1 +#define HAVE_SYS_PRCTL_H 1 #ifdef DEBUG #define DEBUG_DIR(x) {if (do_debug) { x; }} @@ -46,6 +50,11 @@ #include <dirent.h> #include <errno.h> #include <dlfcn.h> +#ifdef HAVE_SYS_PRCTL_H +#include <sys/prctl.h> +#else +#define PR_GET_DUMPABLE 3 +#endif struct dirent_s { unsigned long long d_ino; @@ -83,6 +92,27 @@ static int num_open = 0; static int do_debug = 0; #endif +static char *safe_getenv(const char *arg) +{ + if ((getuid() != geteuid()) || (getgid() != getegid())) + return NULL; +#if HAVE_PRCTL + if (prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) == 0) + return NULL; +#else +#if (defined(linux) && defined(SYS_prctl)) + if (syscall(SYS_prctl, PR_GET_DUMPABLE, 0, 0, 0, 0) == 0) + return NULL; +#endif +#endif + +#if HAVE___SECURE_GETENV + return __secure_getenv(arg); +#else + return getenv(arg); +#endif +} + static void setup_ptr() { char *cp; @@ -97,11 +127,11 @@ static void setup_ptr() real_telldir = dlsym(RTLD_NEXT, "telldir"); real_seekdir = dlsym(RTLD_NEXT, "seekdir"); real_dirfd = dlsym(RTLD_NEXT, "dirfd"); - if ((cp = getenv("SPD_READDIR_MAX_SIZE")) != NULL) { + if ((cp = safe_getenv("SPD_READDIR_MAX_SIZE")) != NULL) { max_dirsize = atol(cp); } #ifdef DEBUG - if (getenv("SPD_READDIR_DEBUG")) { + if (safe_getenv("SPD_READDIR_DEBUG")) { printf("initialized!\n"); do_debug++; }
This is part of a series of improvements from a 2008 version of spd_readdir.c that somehow didn't make it into the version which we checked into e2fsprogs git tree. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- contrib/spd_readdir.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)