coolkey/coolkey-cache-dir-move.patch

CVE-2007-4129 coolkey file and directory permission flaw

Steve Grubb reported: "It looks like coolkey creates /tmp/.pk11ipc1 as a 
world writable directory without the sticky bit. And...it creates the files 
under that potentially as world writable with the execute bit turned on or 
uses the file without any sanity check. coolkey runs as root sometimes and 
that makes it susceptible to doing symlink attacks."

I know some folks ship coolkey here, so we've set an embargo of 20070904, 
but as it's low severity are happy to extend if anyone wishes.

CVE-2007-4129 for this issue.

Proposed patch from Bob Relyea attached.
===================================================================
Index: src/coolkey/machdep.cpp
===================================================================
RCS file: /cvs/dirsec/coolkey/src/coolkey/machdep.cpp,v
retrieving revision 1.4
diff -u -r1.4 machdep.cpp
--- src/coolkey/machdep.cpp	14 Feb 2007 00:46:28 -0000	1.4
+++ src/coolkey/machdep.cpp	15 Aug 2007 01:41:11 -0000
@@ -185,12 +185,20 @@
 #define MAP_INHERIT 0
 #endif
 
+#ifndef BASEPATH
+#ifdef MAC
+#define BASEPATH "/var"
+#else
+#define BASEPATH "/var/cache"
+#endif
+#endif
+
 #ifdef FULL_CLEANUP
 #define RESERVED_OFFSET 256
-#define MEMSEGPATH "/tmp/.pk11ipc"
+#define MEMSEGPATH BASEPATH"/coolkey-lock"
 #else 
 #define RESERVED_OFFSET 0
-#define MEMSEGPATH "/tmp/.pk11ipc1"
+#define MEMSEGPATH BASEPATH"/coolkey"
 #endif
 
 struct SHMemData {
@@ -208,11 +216,6 @@
 #ifdef FULL_CLEANUP
 	flock(fd,LOCK_EX);
 	unsigned long ref = --(*(unsigned long *)addr); 
-#ifdef notdef
-	if (ref == 0) {
-	    unlink(path);
-	}
-#endif
 	flock(fd, LOCK_UN);
 #endif
 	munmap(addr,size+RESERVED_OFFSET);
@@ -225,6 +228,73 @@
     }
 }
 
+/*
+ * The cache directory is shared and accessible by anyone, make
+ * sure the cache file we are opening is really a valid cache file.
+ */
+int safe_open(char *path, int flags, int mode, int size)
+{
+    struct stat buf;
+    int fd, ret;
+
+    fd = open (path, flags|O_NOFOLLOW, mode);
+
+    if (fd < 0) {
+	return fd;
+    }
+
+    ret = fstat(fd, &buf);
+    if (ret < 0) {
+	close (fd);
+	return ret;
+    }
+
+    /* our cache files are pretty specific, make sure we are looking
+     * at the correct one */
+
+    /* first, we should own the file ourselves, don't open a file
+     * that someone else wanted us to see. */
+    if (buf.st_uid != getuid()) {
+	close(fd);
+	errno = EACCES;
+	return -1;
+    }
+
+    /* next, there should only be one link in this file. Don't
+     * use this code to trash another file */
+    if (buf.st_nlink != 1) {
+	close(fd);
+	errno = EMLINK;
+	return -1;
+    }
+
+    /* next, This better be a regular file */
+    if (!S_ISREG(buf.st_mode)) {
+	close(fd);
+	errno = EACCES;
+	return -1;
+    }
+
+    /* if the permissions don't match, something is wrong */
+    if ((buf.st_mode & 03777) != mode) {
+	close(fd);
+	errno = EACCES;
+	return -1;
+    }
+
+    /* finally the file should be the correct size. This 
+     * check isn't so much to protect from an attack, as it is to
+     * detect a corrupted cache file */
+    if (buf.st_size != size) {
+	close(fd);
+	errno = EACCES;
+	return -1;
+    }
+
+    /* OK, the file checked out, ok to continue */
+    return fd;
+}
+
 SHMem::SHMem(): shmemData(0) {}
 
 SHMem *
@@ -248,7 +318,7 @@
 	return NULL;
     }
     int mask = umask(0);
-    int ret = mkdir (MEMSEGPATH, 0777);
+    int ret = mkdir (MEMSEGPATH, 01777);
     umask(mask);
     if ((ret == -1) && (errno != EEXIST)) {
 	delete shmemData;
@@ -264,21 +334,16 @@
     shmemData->path[sizeof(MEMSEGPATH)-1] = '/';
     strcpy(&shmemData->path[sizeof(MEMSEGPATH)],name);
 
-    int mode = 0777;
-    if (strcmp(name,"token_names") != 0) {
-	/* each user gets his own uid array */
-    	sprintf(uid_str, "-%u",getuid());
-    	strcat(shmemData->path,uid_str);
-	mode = 0700;
-    } 
+    sprintf(uid_str, "-%u",getuid());
+    strcat(shmemData->path,uid_str);
+    int mode = 0600;
+
     shmemData->fd = open(shmemData->path, 
 		O_CREAT|O_RDWR|O_EXCL|O_APPEND|O_EXLOCK, mode);
-    if (shmemData->fd  < 0) {
-	needInit = false;
-	shmemData->fd = open(shmemData->path,O_RDWR|O_EXLOCK, mode);
-    }  else {
+    if (shmemData->fd >= 0) {
 	char *buf;
 	int len = size+RESERVED_OFFSET;
+	int ret;
 
 	buf = (char *)calloc(1,len);
 	if (!buf) {
@@ -289,8 +354,22 @@
 	    delete shmemData;
 	    return NULL;
 	}
-	write(shmemData->fd,buf,len);
+	ret = write(shmemData->fd,buf,len);
+	if (ret != len) {
+	    unlink(shmemData->path);
+#ifdef FULL_CLEANUP
+	    flock(shmemData->fd, LOCK_UN);
+#endif
+	    delete shmemData;
+	    return NULL;
+	}
+	
 	free(buf);
+    } else if (errno == EEXIST) {
+	needInit = false;
+
+	shmemData->fd = safe_open(shmemData->path,O_RDWR|O_EXLOCK, mode,
+				  size+RESERVED_OFFSET);
     }
     if (shmemData->fd < 0) {
 	delete shmemData;