cups overwrites files as root in a directory with non-root write permission

[michaelrsweet]

Version: 1.4.2
CUPS.org User: wietse

The CUPS server saves state and overwrites files as root in a
directory that is writable by unprivileged processes.

This is a latent privilege escalation vulnerability. It can be
exploited only in the presence of other CUPS vulnerabilities.

Why this is privilege escalation

This is privilege escalation, because an unprivileged process can
trick the CUPS server into overwriting arbitrary files as root.

Example:

drwxrwxr-x  4 root lp /var/cache/cups
-rw-r-----  1 root lp /var/cache/cups/remote.cache

This file is opened with cupsFileOpen() which simply opens the file
with open(filename, O_WRONLY | O_TRUNC | O_CREAT | O_LARGEFILE |
O_BINARY, 0666).

If a CUPS "external" program has a vulnerability, an attacker can
use the group=lp privileges to replace /var/cache/cups/remote.cache
with a symlink to a root-writable file. CUPS will then overwrite
that file as root. A similar latent vulnerability exists for the
state file /var/cache/cups/job.cache.

Why this is a latent vulnerability

This is a latent vulnerability, because there is no known exploit
for CUPS "external" programs that run as user=lp, group=lp.

Possible solutions

• Save the state to a file in a directory that is writable only by
  privileged processes. Alternatively, log an error warning when
  the directory has unprivileged write permission.
• Don't use root privileges when creating a file in a directory
  that unprivileged processes can write to. This is an option
  when the information is not used for privileged decisions.
• Exercise appropriate paranoia when overwriting or creating files
  in an directory with unprivileged write access. To create a file
  use O_CREAT|O_EXCL. To open an existing file use open/fstat/lstat
  and verify that fstat results match lstat results. An example of
  such code can be found in the Postfix safe_open() function.

[michaelrsweet]

CUPS.org User: mike

OK, IMHO we should implement something like safe_open in cupsFileOpen (i.e. cupsFileOpen would only support a "safe open mode") so that it applies to all of the CUPS software (not just cupsd) and files (not just files in CacheDir).

[michaelrsweet]

CUPS.org User: mike

OK, can you review the attached patch? Basically, I've updated cupsFileOpen to not allow creating or appending to a file that is a symlink, much like you've done in Postfix.

[michaelrsweet]

CUPS.org User: wietse

Thanks. I haven't tested the code, but have a few comments.

• You look up file status information with fstat/lstat, and compare
  the st_gen structure members. These structure members were introduced
  with 4.4BSD and likely exist on MacOS X. They are not implemented
  on all platforms, so the test will need to be compiled conditionally.
• cups_open invokes lstat to avoid following symlinks on non-Windows
  platforms, but Windows also has a symlink mechanism. Some info is
  at: http://msdn.microsoft.com/en-us/library/aa365680(VS.85).aspx.
  As on UNIX, Windows symlinks are reparse points.
• The open/fstat/lstat code pattern defends against untrusted write
  permission in the final directory. This is sufficient in the case
  of /var/mail/username and /var/cache/cups. It is not sufficient
  when the final directory has a parent directory with untrusted
  write permission. I do not know if CUPS uses such directories
  internally. If such directories do exist, a more elaborate safe
  open function would be needed, along the lines of what we published
  last week at the NDSS 2010 conference.
• cups_open disallows symlinks and multiple hardlinks even when a
  directory is writable only by its owner. I do not know if CUPS uses
  symlinks or multiple hardlinks internally. If that is the case,
  this code would introduce a compatibility break that affects CUPS
  itself.

I do not know if you intend to use cups_open for non-CUPS files,
but if you do, this code would introduce a compatibility break that
affects users.

[michaelrsweet]

CUPS.org User: mike

Thanks for you feedback!

Re: st_gen, I have some pending autoconf stuff to add.

Re: Windows support, I already have a few changes pending but haven't started looking at symlink support. However, since we don't actually run cupsd on Windows I am not making that support a "must fix" for this bug.

Re: untrusted parent directories, this isn't an issue for directories used by cupsd in the default configuration, and we can add a check in cupsd (like we already do for the directories themselves) to make sure that world/group write permissions are not enabled for the parent directories.

Re: writing to hard links and symlinks, that's never been a supported configuration anyways, but I'll add documentation to cupsFileOpen to make this explicit...

Thanks again!

[michaelrsweet]

CUPS.org User: mike

Fixed in Subversion repository.

[michaelrsweet]

CUPS.org User: mdeslaur

Did this issue get a CVE number assigned?

[michaelrsweet]

CUPS.org User: wietse

I did not request a CVE, since this is latent. It depends on the existence of directly exploitable CUPS vulnerabilities.

[michaelrsweet]

CUPS.org User: kurtseifried

CVE-2010-2431 has been assigned for this.

[michaelrsweet]

"str3510.patch":

Index: cups/file.c

--- cups/file.c (revision 9034)
+++ cups/file.c (working copy)
@@ -8,7 +8,7 @@

• our own file functions allows us to provide transparent support of
• gzip'd print files, PPD files, etc.

• * Copyright 2007-2009 by Apple Inc.
• * Copyright 2007-2010 by Apple Inc.
• Copyright 1997-2007 by Easy Software Products, all rights reserved.

• These coded instructions, statements, and computer programs are the
  @@ -59,6 +59,7 @@
  */

#include "file-private.h"
+#include <sys/stat.h>

/*
@@ -69,6 +70,7 @@
static ssize_t cups_compress(cups_file_t fp, const char *buf, size_t bytes);
#endif / HAVE_LIBZ */
static ssize_t cups_fill(cups_file_t *fp);
+static int cups_open(const char *filename, int mode);
static ssize_t cups_read(cups_file_t *fp, char *buf, size_t bytes);
static ssize_t cups_write(cups_file_t *fp, const char *buf, size_t bytes);

@@ -827,7 +829,8 @@
switch (mode)
{
case 'a' : / Append file */

•    fd = open(filename, O_RDWR | O_CREAT | O_APPEND | O_LARGEFILE | O_BINARY, 0666);
  

•    fd = cups_open(filename,
  

•          O_RDWR | O_CREAT | O_APPEND | O_LARGEFILE | O_BINARY);
   break;
  

  case 'r' : /* Read file */
  @@ -835,7 +838,17 @@
  break;

  case 'w' : /* Write file */

•    fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT | O_LARGEFILE | O_BINARY, 0666);
  

•    fd = cups_open(filename, O_WRONLY | O_LARGEFILE | O_BINARY);
  

• if (fd < 0 && errno == ENOENT)

• {

• fd = cups_open(filename,
  

•                O_WRONLY | O_CREAT | O_EXCL | O_LARGEFILE | O_BINARY);
  

• if (fd < 0 && errno == EEXIST)
  

•   fd = cups_open(filename, O_WRONLY | O_LARGEFILE | O_BINARY);
  

• }

• if (fd >= 0)

• ftruncate(fd, 0);
   break;
  

  case 's' : /* Read/write socket */
  @@ -2209,6 +2222,88 @@

  /*

• * 'cups_open()' - Safely open a file for writing.

• * We don't allow appending to directories or files that are hard-linked or

• * symlinked.

• /
  +
  +static int / O - File descriptor or -1 otherwise /
  +cups_open(const char *filename, / I - Filename */

• int        mode)      /\* I - Open mode */
  

  +{

• int fd; /* File descriptor */

• struct stat fileinfo; /* File information */
  +#ifndef WIN32

• struct stat linkinfo; /* Link information /
  +#endif / !WIN32 */

• /*
• * Open the file...
• */

• if ((fd = open(filename, mode, 0666)) < 0)
• return (-1);

• /*
• * Then verify that the file descriptor doesn't point to a directory or hard-
• * linked file.
• */

• if (fstat(fd, &fileinfo))
• {
• close(fd);
• return (-1);
• }

• if (fileinfo.st_nlink != 1)
• {
• close(fd);
• errno = EPERM;
• return (-1);
• }

• if (S_ISDIR(fileinfo.st_mode))
• {
• close(fd);
• errno = EISDIR;
• return (-1);
• }

+#ifndef WIN32

• /*
• * Then use lstat to determine whether the filename is a symlink...
• */

• if (lstat(filename, &linkinfo))
• {
• close(fd);
• return (-1);
• }

• if (S_ISLNK(linkinfo.st_mode) ||

•  fileinfo.st_dev != linkinfo.st_dev ||
  

•  fileinfo.st_ino != linkinfo.st_ino ||
  

•  fileinfo.st_gen != linkinfo.st_gen ||
  

•  fileinfo.st_nlink != linkinfo.st_nlink ||
  

•  fileinfo.st_mode != linkinfo.st_mode)
  

• {
• /*
• * Yes, don't allow!
• */

• close(fd);
• errno = EPERM;
• return (-1);
• }
  +#endif /* !WIN32 */

• return (fd);
  +}

+/*

• 'cups_read()' - Read from a file descriptor.
  */