193 lines
4.8 KiB
Diff
193 lines
4.8 KiB
Diff
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;
|