[8575] in bugtraq
Re: Xinetd /tmp race? (long)
daemon@ATHENA.MIT.EDU (Marc Heuse)
Mon Nov 16 22:47:52 1998
Date: Sun, 15 Nov 1998 19:12:37 +0100
Reply-To: Marc Heuse <marc@SUSE.DE>
From: Marc Heuse <marc@SUSE.DE>
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;
}
