oysteikt/heimdal
0
0
Fork 0
Code Issues 5 Pull Requests Releases Activity

Make Refuses to open symlinks msg less spurious

Browse Source 
If we're racing enough we could complain about symlinks where there were
none.  This was very surprising.  Make it surprise less.

We should really #ifndef O_NOFOLLOW that code chunk too, for the obvious
reason that we don't need to worry about symlinks if we have and use
O_NOFOLLOW.

Also, since all uses of fcc_open() use O_NOFOLLOW we should move that
into fcc_open().  Ditto O_BINARY and O_CLOEXEC.
This commit is contained in:
Nicolas Williams
2013-08-16 23:46:06 -05:00
parent 0866ee2615
commit e3eb1305f5
1 changed files with 35 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
lib/krb5/fcache.c
View File

@@ -396,9 +396,10 @@ fcc_open(krb5_context context,
(flags | O_RDWR) == flags);
krb5_error_code ret;
const char *filename;
struct stat sb1, sb2;
struct stat sb1, sb2, sb3;
int strict_checking;
int fd;
size_t tries = 3;
*fd_ret = -1;
@@ -410,16 +411,23 @@ fcc_open(krb5_context context,
strict_checking = (flags & O_CREAT) == 0 &&
(context->flags & KRB5_CTX_F_FCACHE_STRICT_CHECKING) != 0;
again:
if (strict_checking) {
ret = lstat(filename, &sb1);
if (ret < 0) {
krb5_set_error_message(context, ret, N_("%s lstat(%s)", "file, error"),
operation, filename);
return errno;
}
}
if (!S_ISREG(sb1.st_mode)) {
krb5_set_error_message(context, EPERM
N_("Refuses to open symlinks for "
"caches FILE:%s", ""), filename);
return EPERM;
}
fd = open(filename, flags, mode);
if(fd < 0) {
char buf[128];
@@ -446,11 +454,28 @@ fcc_open(krb5_context context,
}
#ifndef _WIN32
/* All of the following could be #ifndef O_NOFOLLOW, FYI */
if (sb1.st_dev != sb2.st_dev || sb1.st_ino != sb2.st_ino) {
krb5_set_error_message(context, EPERM, N_("Refuses to open symlinks for caches FILE:%s", ""), filename);
/*
* Perhaps we raced with a rename(). To complain about
* symlinks in that case would cause unnecessary concern, so
* we check for that possibility and loop. This has no
* TOCTOU problems because we redo the open() (and if we
* have O_NOFOLLOW we could even avoid that too).
*/
close(fd);
ret = lstat(filename, &sb3);
if (ret || sb1.st_dev != sb2.st_dev ||
sb3.st_dev != sb2.st_dev || sb3.st_ino != sb2.st_ino) {
krb5_set_error_message(context, EPERM, N_("Refuses to open possible symlink for caches: FILE:%s", ""), filename);
return EPERM;
}
if (--tries == 0) {
krb5_set_error_message(context, EPERM, N_("Raced too many times with renames of FILE:%s", ""), filename);
return EPERM;
}
goto again;
}
#endif
if (sb2.st_nlink != 1) {
@@ -459,6 +484,12 @@ fcc_open(krb5_context context,
return EPERM;
}
#ifndef _WIN32
/*
* XXX Should probably add options to improve control over this
* check. We might want strict checking of everything except
* this, and we might want st_uid == getuid() || st_uid == geteuid()
* to be OK.
*/
if (sb2.st_uid != getuid()) {
krb5_set_error_message(context, EPERM, N_("Refuses to open cache files not own by myself FILE:%s (owned by %d)", ""), filename, (int)sb2.st_uid);
close(fd);