[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHmME9ryCHeJZX3pJFTbaO-A=rhp_zx8wAo15ifMmvCLCMaynw@mail.gmail.com>
Date: Sat, 14 Jul 2012 07:41:44 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: oss-security@...ts.openwall.com
Cc: Tyler Hicks <tyhicks@...onical.com>, Kurt Seifried <kseifried@...hat.com>,
Marcus Meissner <meissner@...e.de>, Dan Rosenberg <dan.j.rosenberg@...il.com>
Subject: Re: Re: ecryptfs headsup
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
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
