michaelrsweet commented 18 years ago

Version: 1.2-current CUPS.org User: jlovell

check_permissions() is susceptable to a "time of checkâtime of use" race condition by using stat() and chown()/chmod().

Hopefully the attached patch fixes it -- but please double check it.

This is radr://4697828

Thanks!

michaelrsweet commented 18 years ago

CUPS.org User: mike

The patch opens directories as regular files which does not work on all of the operating systems we support. Even if "open" works, the mkdir + open strategy still has a race condition...

Also, we generally don't want to abort cupsd (return 0 from cupsdReadConfiguration) if check_permissions fails, since that will prevent cupsd from running as a non-root user, for example.

Finally, check_permissions() was never designed to provide absolute protection of the checked files or directories - we just want to correct common install/config errors so that printing works. Since we don't check before every file/directory operation, any user with sufficient permissions can change things after cupsd.conf is loaded and check_permissions is run...

....

It looks like the issue is that a user might have created a file called /var/tmp/cups, which would prevent cupsd from creating its directory? Would it be better to just put CacheDir in /var/spool/cups/cache on OSX instead?

michaelrsweet commented 18 years ago

CUPS.org User: jlovell

Yes, the main issue is the window of time between stat/mkdir/chown in the public directory /var/tmp. Moving the cache dir to /var/spool/cups should be good enough. Still, havingÂ check_permissions() avoid the insecure pattern of file operations seems like a good thing...

FWIW: http://developer.apple.com/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html

Thanks.

michaelrsweet commented 18 years ago

CUPS.org User: mike

Again, the whole point of check_permissions() is not to provide security but to correct simple configuration or installation errors.

Changing from chmod/chown/stat to open/fchmod/fchown/fstat/lstat will not make the scheduler any more secure, and in fact will introduce new problems such as the OS-dependent "open a directory" code and the fact that your changes to use fstat and lstat will prevent otherwise valid use of symlinks.

Even if I remove the symlink prevention code, any changes made by check_permissions() are not protected against later changes made after the check. In order to truly prevent this kind of race condition, we would need to modify the scheduler to keep all files and directories open in exclusive mode, however that would prevent valid access by child processes and would not work with most network file systems.

In short, there is little benefit to changing check_permissions() as you propose.

....

That said, I'm OK with updating things so that a check_permissions() failure (vs. a warning) will prevent the scheduler from running. That would be consistent with our "help but don't hang the user" policy... :)

michaelrsweet commented 18 years ago

CUPS.org User: mike

Fixed in Subversion repository.

The attached patch aborts cupsd if we find any hard config file/directory errors. I've also changed the default CacheDir to /var/spool/cups/cache when compiling on OSX...

michaelrsweet commented 18 years ago

CUPS.org User: jlovell

The changes mostly look good... wouldn't it be better to use lstat in place of stat? If Linux's /var/cache is world writable then a symbolic link of /var/cache/cups would allow any directory on the system to be chown'd to root.

Thanks.

michaelrsweet commented 18 years ago

CUPS.org User: mike

Again, we have to support symlinks - many of our customers use them to relocate some CUPS files to separate filesystems. If we used lstat() then all of those customers' installations would break.

In any case, /var/cache is a FHS-defined system directory and is never shipped world-writable.

michaelrsweet commented 18 years ago

"str1941.patch":

Index: conf.c

--- conf.c (revision 5856) +++ conf.c (working copy) @@ -636,22 +636,24 @@

writable by the user and group in the cupsd.conf file... */
check_permissions(CacheDir, NULL, 0775, RunUser, Group, 1, 1); -/* check_permissions(CacheDir, "ppd", 0755, RunUser, Group, 1, 1);*/
if (check_permissions(CacheDir, NULL, 0775, RunUser, Group, 1, 1) ||
check_permissions(StateDir, NULL, 0755, RunUser, Group, 1, 1) ||
check_permissions(StateDir, "certs", RunUser ? 0711 : 0511, User,
SystemGroupIDs[0], 1, 1) ||
check_permissions(ServerRoot, NULL, 0755, RunUser, Group, 1, 0) ||
check_permissions(ServerRoot, "ppd", 0755, RunUser, Group, 1, 1) ||
check_permissions(ServerRoot, "cupsd.conf", ConfigFilePerm,
RunUser, Group, 0, 0))
return (0);
check_permissions(StateDir, NULL, 0755, RunUser, Group, 1, 1);
check_permissions(StateDir, "certs", RunUser ? 0711 : 0511, User,

- SystemGroupIDs[0], 1, 1);

check_permissions(ServerRoot, NULL, 0755, RunUser, Group, 1, 0);
check_permissions(ServerRoot, "ppd", 0755, RunUser, Group, 1, 1);
check_permissions(ServerRoot, "ssl", 0700, RunUser, Group, 1, 0);
check_permissions(ServerRoot, "cupsd.conf", ConfigFilePerm, RunUser, Group,
0, 0); check_permissions(ServerRoot, "classes.conf", 0600, RunUser, Group, 0, 0); check_permissions(ServerRoot, "printers.conf", 0600, RunUser, Group, 0, 0); check_permissions(ServerRoot, "passwd.md5", 0600, User, Group, 0, 0);

+#if defined(HAVE_LIBSSL) || defined(HAVE_GNUTLS)

check_permissions(ServerRoot, "ssl", 0700, RunUser, Group, 1, 0); +#endif /* HAVE_LIBSSL || HAVEGNUTLS / + /_
- Update TempDir to the default if it hasn't been set already... */ @@ -692,7 +694,8 @@
- permissions... */
check_permissions(RequestRoot, NULL, 0710, RunUser, Group, 1, 1);
if (check_permissions(RequestRoot, NULL, 0710, RunUser, Group, 1, 1))
return (0);

if (!strncmp(TempDir, RequestRoot, strlen(RequestRoot)) || access(TempDir, 0)) @@ -702,7 +705,8 @@
- is under the spool directory or does not exist... */
check_permissions(TempDir, NULL, 01770, RunUser, Group, 1, 1);
if (check_permissions(TempDir, NULL, 01770, RunUser, Group, 1, 1))
```
return (0);
```
}

if (!strncmp(TempDir, RequestRoot, strlen(RequestRoot))) @@ -1121,9 +1125,11 @@ int isdir, /* I - 1 = directory, 0 = file / int createdir)/ I - 1 = create directory, 0 = not */ {
int dir_created = 0; /* Did we create a directory? */
int fd, /* File descriptor */
dircreated = 0; /* Did we create a directory? / char pathname[1024]; /_ File name with prefix */
struct stat fileinfo; /* Stat buffer */
struct stat lstatbuf, /* lstat buffer */
```
fstatbuf;       /* fstat buffer */
```
/* @@ -1137,10 +1143,10 @@ }

/*
* See if we can stat the file/directory...
- See if we can open the file/directory... */
if (stat(filename, &fileinfo))
if ((fd = open(filename, O_RDONLY)) == -1) { if (errno == ENOENT && create_dir) { @@ -1156,24 +1162,64 @@ }

dir_created = 1; +
if ((fd = open(filename, O_RDONLY)) == -1)
{
cupsdLogMessage(CUPSD_LOG_ERROR,
"Unable to open new directory \"%s\" - %s", filename,
strerror(errno));
return (-1);
} } else
{
cupsdLogMessage(CUPSD_LOG_ERROR,
"Unable to open \"%s\" - %s", filename, strerror(errno)); return (-1);
} }
/*
* See if we can stat the file/directory...
*/
if (fstat(fd, &fstatbuf) == -1 || lstat(filename, &lstatbuf) == -1)
{
cupsdLogMessage(CUPSD_LOG_ERROR,
"Unable to stat \"%s\" - %s", filename, strerror(errno));
close(fd);
return (-1);
} + /*
* Make sure the file we have open is the same as the lstat info
* (i.e. it's not a symbolic link)...
*/ +
if (fstatbuf.st_dev != lstatbuf.st_dev ||
fstatbuf.st_ino != lstatbuf.st_ino)
{
cupsdLogMessage(CUPSD_LOG_ERROR,
"\"%s\" is a symbolic link!", filename);
close(fd);
return (-1);
} +
/*
- Make sure it's a regular file... */
if (!dir_created && !is_dir && !S_ISREG(fileinfo.st_mode))
if (!is_dir && !S_ISREG(fstatbuf.st_mode)) { cupsdLogMessage(CUPSD_LOG_ERROR, "\"%s\" is not a regular file!", filename);
close(fd); return (-1); }
if (!dir_created && is_dir && !S_ISDIR(fileinfo.st_mode))
if (is_dir && !S_ISDIR(fstatbuf.st_mode)) { cupsdLogMessage(CUPSD_LOG_ERROR, "\"%s\" is not a directory!", filename);
close(fd); return (-1); }

@@ -1181,28 +1227,47 @@

Fix owner, group, and mode as needed... */
if (dir_created || fileinfo.st_uid != user || fileinfo.st_gid != group)
if (fstatbuf.st_uid != user || fstatbuf.st_gid != group) {
if (S_ISREG(fstatbuf.st_mode) && fstatbuf.st_nlink > 1)
{
cupsdLogMessage(CUPSD_LOG_ERROR,
"File \"%s\" has hard links, cannot repair ownership!",
filename);
close(fd);
return (-1);
}
cupsdLogMessage(CUPSD_LOG_WARN, "Repairing ownership of \"%s\"", filename);
if (chown(filename, user, group))
if (fchown(fd, user, group)) { cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to change ownership of \"%s\" - %s", filename, strerror(errno));
```
close(fd);
```
return (-1); } }
if (dir_created || (fileinfo.st_mode & 07777) != mode)
if ((fstatbuf.st_mode & 07777) != mode) {
if (dir_created)
{
cupsdLogMessage(CUPSD_LOG_WARN, "Access permissions of \"%s\" changed!",
filename);
close(fd);
return (-1);
} + cupsdLogMessage(CUPSD_LOG_WARN, "Repairing access permissions of \"%s\"", filename);
if (chmod(filename, mode))
if (fchmod(fd, mode)) { cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to change permissions of \"%s\" - %s", filename, strerror(errno));
```
close(fd);
```
return (-1); } } @@ -1211,6 +1276,7 @@
- Everything is OK... */
close(fd); return (0); }

michaelrsweet commented 18 years ago

"str1941esp.patch":

Index: conf.c

--- conf.c (revision 5902) +++ conf.c (working copy) @@ -636,22 +636,22 @@

writable by the user and group in the cupsd.conf file... */
check_permissions(CacheDir, NULL, 0775, RunUser, Group, 1, 1); -/* check_permissions(CacheDir, "ppd", 0755, RunUser, Group, 1, 1);*/
if (check_permissions(CacheDir, NULL, 0775, RunUser, Group, 1, 1) < 0 ||
check_permissions(StateDir, NULL, 0755, RunUser, Group, 1, 1) < 0 ||
check_permissions(StateDir, "certs", RunUser ? 0711 : 0511, User,
SystemGroupIDs[0], 1, 1) < 0 ||
check_permissions(ServerRoot, NULL, 0755, RunUser, Group, 1, 0) < 0 ||
check_permissions(ServerRoot, "ppd", 0755, RunUser, Group, 1, 1) < 0 ||
check_permissions(ServerRoot, "ssl", 0700, RunUser, Group, 1, 0) < 0 ||
check_permissions(ServerRoot, "cupsd.conf", ConfigFilePerm, RunUser,
Group, 0, 0) < 0 ||
check_permissions(ServerRoot, "classes.conf", 0600, RunUser, Group,
0, 0) < 0 ||
check_permissions(ServerRoot, "printers.conf", 0600, RunUser, Group,
0, 0) < 0 ||
check_permissions(ServerRoot, "passwd.md5", 0600, User, Group, 0, 0) < 0)
return (0);
check_permissions(StateDir, NULL, 0755, RunUser, Group, 1, 1);
check_permissions(StateDir, "certs", RunUser ? 0711 : 0511, User,

- SystemGroupIDs[0], 1, 1);

check_permissions(ServerRoot, NULL, 0755, RunUser, Group, 1, 0);
check_permissions(ServerRoot, "ppd", 0755, RunUser, Group, 1, 1);
check_permissions(ServerRoot, "ssl", 0700, RunUser, Group, 1, 0);
check_permissions(ServerRoot, "cupsd.conf", ConfigFilePerm, RunUser, Group,
0, 0);
check_permissions(ServerRoot, "classes.conf", 0600, RunUser, Group, 0, 0);
check_permissions(ServerRoot, "printers.conf", 0600, RunUser, Group, 0, 0);

- check_permissions(ServerRoot, "passwd.md5", 0600, User, Group, 0, 0);

/*

Update TempDir to the default if it hasn't been set already... */ @@ -677,14 +677,14 @@ "TMPDIR (%s) has the wrong permissions!", tmpdir); else

cupsdSetString(&TempDir, tmpdir);
- if (!TempDir)
- cupsdLogMessage(CUPSD_LOG_INFO, "Using default TempDir of %s/tmp...",
- ```
          RequestRoot);
```
}

if (!TempDir)
- {
- cupsdLogMessage(CUPSD_LOG_INFO, "Using default TempDir of %s/tmp...",
- RequestRoot); cupsdSetStringf(&TempDir, "%s/tmp", RequestRoot);
- } }
/* @@ -692,7 +692,8 @@
permissions... */
check_permissions(RequestRoot, NULL, 0710, RunUser, Group, 1, 1);
if (check_permissions(RequestRoot, NULL, 0710, RunUser, Group, 1, 1) < 0)
return (0);

if (!strncmp(TempDir, RequestRoot, strlen(RequestRoot)) || access(TempDir, 0)) @@ -702,7 +703,8 @@
- is under the spool directory or does not exist... */
check_permissions(TempDir, NULL, 01770, RunUser, Group, 1, 1);
if (check_permissions(TempDir, NULL, 01770, RunUser, Group, 1, 1) < 0)
```
return (0);
```
}

if (!strncmp(TempDir, RequestRoot, strlen(RequestRoot))) @@ -772,7 +774,7 @@ if (MaxActiveJobs > (MaxFDs / 3)) MaxActiveJobs = MaxFDs / 3;
if (Classification && strcasecmp(Classification, "none") == 0)
if (Classification && !strcasecmp(Classification, "none")) cupsdClearString(&Classification);

if (Classification) @@ -1112,7 +1114,7 @@
- 'check_permissions()' - Fix the mode and ownership of a file or directory. */

-static int /* O - 0 on success, -1 on error / +static int / O - 0 on success, -1 on error, 1 on warning _/ checkpermissions(const char *filename, / I - File/directory name / const char *suffix, / I - Additional file/directory name / int mode, / I - Permissions */ @@ -1158,7 +1160,7 @@ dir_created = 1; } else

return (-1);
```
return (create_dir ? -1 : 1);
```
}

/* @@ -1190,7 +1192,7 @@ cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to change ownership of \"%s\" - %s", filename, strerror(errno));
return (-1);
return (1); } }

@@ -1203,7 +1205,7 @@ cupsdLogMessage(CUPSD_LOG_ERROR, "Unable to change permissions of \"%s\" - %s", filename, strerror(errno));

return (-1);
return (1); } }

apple / cups

cups cachedir of /var/tmp/cups means check_permissions() could own an arbitrary file to root #1941

Index: conf.c

- SystemGroupIDs[0], 1, 1);

Index: conf.c

- SystemGroupIDs[0], 1, 1);

- check_permissions(ServerRoot, "passwd.md5", 0600, User, Group, 0, 0);

cupsdSetString(&TempDir, tmpdir);