Date: Sun, 15 Nov 1998 19:12:37 +0100 From: Marc Heuse <marc@SUSE.DE> Subject: Re: Xinetd /tmp race? (long) To: BUGTRAQ@NETSPACE.ORG Hi folks, I have to apologize for my previous posting with the patch for the /tmp problem of xinetd. It does not close the whole security problem, well I missed the possibility of hardlinking the file. Well, I can only say, I tested that on my machine, but obviously with my brain in power saving mode. marc@master:/tmp > ln /etc/passwd /tmp/xinetd.dump ln: cannot create hard link `/tmp/xinetd.dump' to `/etc/passwd': Operation not permitted I thought "okay, thats not a problem", but that was because I had installed solar designer's secure-linux patch :-( such a single minded thing should not happen to someone doing that as a job :-(( Some people wrote me an email and pointed that out, (even a bigmouth claiming that secure appending to files is easy), but the fix they proposed, checking the device and inode number, is NOT secure! This shows that my claim was correct, it's not that easy. I'll show below why and will also present my new general solution to this problem. But first here's the updated security fix. I exchanged the getuid() call to geteuid() call, which changes not the functionality but will not confuse the reader ;) . 2nd I added the hardlink count check, so this hole is closed (<feeling ashame>). Some people also commented about moving the file to another location. I didn't do that because this should be by the author of the package. A security fix should *not* change the behaviour of a program (except fixing the holes ;-) This patch provides not the whole security like the generic solution I present at the end, I wanted to keep the fix simple. --- internals.c.orig Wed Jan 24 20:32:46 1996 +++ internals.c Thu Nov 12 11:18:39 1998 @@ -8,6 +8,7 @@ #include <sys/types.h> #include <sys/stat.h> +#include <unistd.h> #ifdef linux #include <sys/time.h> #endif @@ -54,9 +55,29 @@ time_t current_time ; register int fd ; register unsigned u ; + struct stat stat ; char *func = "dump_internal_state" ; - dump_fd = open( dump_file, O_WRONLY + O_CREAT + O_APPEND, DUMP_FILE_MODE ) ; + dump_fd = open( dump_file, O_WRONLY + O_CREAT + O_EXCL, DUMP_FILE_MODE ) ; + if ( dump_fd == -1 ) + { + if ( lstat( dump_file, &stat) != 0) + { + msg( LOG_ERR, func, "failed to open %s: %m", dump_file ) ; + return ; + } + if (stat.st_uid != geteuid()) + { + msg( LOG_ERR, func, "security: I'm not owning %s: %m", dump_file ) ; + return ; + } + if (stat.st_st.st_nlink != 1) + { + msg( LOG_ERR, func, "security: %s is hardlinked: %m", dump_file ) ; + return ; + } + dump_fd = open( dump_file, O_WRONLY + O_APPEND) ; + } if ( dump_fd == -1 ) { msg( LOG_ERR, func, "failed to open %s: %m", dump_file ) ; This fix is open to a denial-of-service attack, just put a hard/soft-link there in /tmp and xinetd refuses to dump. One can argue if it's better to fail close due a security problem or to remove the problem (unlink/rename etc.) and go on. This should be decided by the author of the program. But now let's get to the "fix" proposed by some guys about checking the device number and inode number before opening the file (lstat) and afterwards (fstat). Try out this small program and you'll see that this won't help and would leave xinetd open for a race condition: master:/data/new/fuck # cat > inode-test.c #include <stdio.h> #include <sys/stat.h> int main () { struct stat st; char name[20]="/tmp/foo"; int fd; printf("Creating file %s.\n", name); if ((fd = creat(name, 0644)) < 0) { perror("creat"); exit(1); } lstat(name, &st); printf("Name: %s Device: %d Inode: %ld\n",name,st.st_dev,st.st_ino); close(fd); unlink(name); printf("Removing %s + placing a symlink there.\n", name); symlink("../etc/passwd", name); lstat(name, &st); printf("Name: %s Device: %d Inode: %ld\n",name,st.st_dev,st.st_ino); unlink(name); } ^D master:/data/new/fuck # cc -o inode-test inode-test.c master:/data/new/fuck # ./inode-number Creating file /tmp/foo. Name: /tmp/foo Device: 839 Inode: 13 Removing /tmp/foo + placing a symlink there. Name: /tmp/foo Device: 839 Inode: 13 as you can see, checking these doesn't help anything. However, what does help is doing all the relevant checks for a {sym|hard}link, owner etc. before opening the file (lstat), and checking the {acm}time between the stat prior opening and afterwards (fstat). In my proposed generic solution below I go even further, checking the whole stat structure for differences and printing an error when found. Please take a look at the source below and comment on it for holes I missed, silly stuff etc. ;-) Greets, Marc -- Marc Heuse, S.u.S.E. GmbH, Schanzaeckerstr. 10, 90443 Nuernberg E@mail: marc@suse.de Function: Security Support & Auditing issue a "finger marc@suse.de | pgp -fka" for my public pgp key /* * Generic example for "How to do a secure appending on a file" * Comment to marc@suse.de */ #include <stdio.h> #include <fcntl.h> #include <unistd.h> #include <sys/stat.h> #include <sys/types.h> #define permissions 0644 int main () { struct stat st; struct stat saved_st; FILE *f; int fd; char file[20] = "/tmp/xinetd.dump"; /* O_EXCL is not secure when used on an NFS mounted filesystem */ if ((fd=open(file, O_RDWR | O_CREAT | O_EXCL, permissions)) < 0) { memset(&st, '\0', sizeof(st)); /* both stat structure filled */ memset(&saved_st, '\0', sizeof(saved_st)); /* with null-byte */ lstat(file, &saved_st); /* get lstat and perform checks */ if ((saved_st.st_mode & S_IFLNK) == S_IFLNK) { fprintf(stderr, "Security: %s is a symlink, aborting.\n", file); return -1; } if (saved_st.st_nlink != 1) { fprintf(stderr, "Security: %s is hardlinked, aborting.\n", file); return -1; } /* // if it's important that the file belongs to the current euid if (saved_st.st_uid != geteuid()) { fprintf(stderr, "Security: I'm not owning %s, aborting.\n", file); return -1; } */ if ((fd=open(file, O_RDWR | O_APPEND)) < 0) { /* lstat data was safe */ perror("open"); return -1; } fstat(fd, &st); /* get the stat data from the filedescriptio */ if (memcmp(&st, &saved_st, sizeof(st)) != 0) { fprintf(stderr, "Security: %s was raced, aborting.\n", file); close(fd); /* if both stat datas differ we've got a security */ return -1; /* problem. complain and return with an error */ } } /* if we arrive here, we've got a safely aquired fd */ close(fd); return 0; }