oss-sec mailing list archives
Re: Re: ecryptfs headsup
From: "Jason A. Donenfeld" <Jason () zx2c4 com>
Date: Sat, 14 Jul 2012 07:41:44 +0200
Hi, There's another almost-hole (almost, like the umount env clearing) that the patch in that bug report fixes: At line 538, this code is added: if (strstr(alias, "..")) { fputs("Invalid alias", stderr); exit(1); } Ostensibly this is to prevent the subsequent call to lock_counter(pwd->pw_name, uid, alias); from crafting the path name like this: asprintf(&f, "%s/%s-%s-%s", TMP, FSTYPE, u, alias) where alias might have ".." in it, and then making a file like this: open(f, O_RDWR | O_CREAT | O_NOFOLLOW, 0600) which takes special care to specify O_NOFOLLOW too, and then afterwards, it checks to see if it's owned by the correct user and makes certain it's a regular file. Fortunately, the call at the beginning of the program to seteuid(uid) prevents this from being too traumatic. But, the strstr check was added apparently as a security consideration; correct me if I'm mistaken. There's actually one use of the alias variable before such sanitation that is almost a big hole, but isn't, due again to the seteuid(uid) at the beginning of the program. At line 525 we have: if (read_config(pwd->pw_dir, uid, alias, &src, &dest, &opts2) < 0) { read_config forms a file name like this: if (asprintf(&fnam, "%s/.ecryptfs/%s.conf", pw_dir, alias) < 0) { It then makes a smart security check to ensure that it's not a symlink and that it's owned by uid: if (stat(fnam, &mstat)!=0 || (!S_ISREG(mstat.st_mode) || mstat.st_uid!=uid)) { fputs("Bad file\n", stderr); free(fnam); return -1; } After it does this, it opens the file for reading with a standard fopen: fin = fopen(fnam, "r"); And then it reads the various settings out of fin. There's a race condition on fnam, since you could make fnam a valid file, but immediately after the stat(2) call, swap it for a symlink. I'm not sure this is really an issue, though, since geteuid(uid) has already been called, so it's not as if the subsequent fopen is allowed to read just any file. It does pose the question, though, of why that stat check is there in the first place, if it can be subverted like that. The solution is just to open the fd first, and then check the permissions and whatnot with fstat(2). Jason Donenfeld
Current thread:
- Re: ecryptfs headsup, (continued)
- Re: ecryptfs headsup Marcus Meissner (Jul 10)
- Re: ecryptfs headsup Dan Rosenberg (Jul 10)
- Re: ecryptfs headsup Tyler Hicks (Jul 10)
- Re: ecryptfs headsup Tyler Hicks (Jul 10)
- Re: ecryptfs headsup Dustin Kirkland (Jul 11)
- Re: ecryptfs headsup Kurt Seifried (Jul 11)
- Re: Re: ecryptfs headsup Tyler Hicks (Jul 11)
- Re: Re: ecryptfs headsup Kurt Seifried (Jul 11)
- Re: Re: ecryptfs headsup Tyler Hicks (Jul 11)
- Re: Re: ecryptfs headsup Dustin Kirkland (Jul 13)
- Re: Re: ecryptfs headsup Jason A. Donenfeld (Jul 13)
- Re: Re: ecryptfs headsup Jason A. Donenfeld (Jul 14)
- Re: Re: ecryptfs headsup Sebastian Krahmer (Jul 16)
- Re: Re: ecryptfs headsup Justin Ossevoort (Jul 16)