Incomplete fix for CVE-2014-3537 (CVE-2014-5029/5030/5031)

[michaelrsweet]

Version: 1.7.4
CUPS.org User: carnil

Hi

I noticed the fix for CVE-2014-3537 might be incomplete. From http://www.cups.org/str.php?L4450 the intention was to dissalow symlinks. But the code in 1.7.4 (similarly in current development version) looks like:

3313 if ((status = stat(filename, filestats)) != 0 && language[0] &&
3314 strncmp(con->uri, "/icons/", 7) &&
3315 strncmp(con->uri, "/ppd/", 5) &&
3316 strncmp(con->uri, "/rss/", 5) &&
3317 strncmp(con->uri, "/admin/conf/", 12) &&
3318 strncmp(con->uri, "/admin/log/", 11))
3319 {
3320 /*
3321 * Drop the country code...
3322 /
3323
3324 language[3] = '\0';
3325 snprintf(filename, len, "%s%s%s", DocumentRoot, language, con->uri);
3326
3327 if ((ptr = strchr(filename, '?')) != NULL)
3328 *ptr = '\0';
3329
3330 if ((status = lstat(filename, filestats)) != 0)
3331 {
3332 /
3333 * Drop the language prefix and try the root directory...
3334 */
3335
3336 language[0] = '\0';
3337 snprintf(filename, len, "%s%s", DocumentRoot, con->uri);
3338
3339 if ((ptr = strchr(filename, '?')) != NULL)
3340 *ptr = '\0';
3341
3342 status = lstat(filename, filestats);
3343 }
3344 }

if language[0] is null, we do not reach the lstat calls for filename and afterwards

3346 /*
3347 * If we've found a symlink, 404 the sucker to avoid disclosing information.
3348 */
3349
3350 if (!status && S_ISLNK(filestats->st_mode))
3351 {
3352 cupsdLogMessage(CUPSD_LOG_INFO, "[Client %d] Symlinks such as "%s" are not allowed.", c on->http.fd, filename);
3353 return (NULL);
3354 }

will not do what was intended.

Additionally (from Michael Sweet):

Yes, it looks like this needs to be an lstat as well, and we should
probably add similar protections to the directory index files (which
are also using stat).

Regards,
Salvatore

[michaelrsweet]

CUPS.org User: mike

Proposed fix attached, with a tentative release date of July 30.

[michaelrsweet]

CUPS.org User: mike

Updated patch for 2.0 (forgot to include world-readable check)

[michaelrsweet]

CUPS.org User: mike

And the patch for 1.7 and earlier (which don't have the cupsdLogClient function)

[michaelrsweet]

CUPS.org User: mike

Now assigned to CVE-2014-5029/5030/5031.

[michaelrsweet]

CUPS.org User: twaugh.redhat

The world-readability test breaks authenticated 'GET /admin/log/error_log' requests. Is that intentional?

[michaelrsweet]

CUPS.org User: jsmeix.suse

For me it still works to view the error log
via the web interface.

I think this is because (at least for my cups-1.7.4)
by default the files are
-rw-r--r-- 1 root lp ... access_log
-rw-r--r-- 1 root lp ... error_log
-rw-r--r-- 1 root lp ... page_log

I vaguely remember there was a longer time ago some thread
about whether or not world readable error_log is o.k.
from a secutity point of view and - as far as I remember -
Michael Sweet's response was that with default cupsd LogLevel
no security relevant information is logged in error_log.

[michaelrsweet]

CUPS.org User: mike

Tim, please file a separate bug for the log file regression; the default log file permissions in CUPS are 0644, but we can probably special-case the log files when authentication is enabled.

[michaelrsweet]

"str4455.patch":

Index: scheduler/client.c

--- scheduler/client.c (revision 12054)
+++ scheduler/client.c (working copy)
@@ -2959,7 +2959,7 @@

• then fallback to the default one...
  */

• if ((status = stat(filename, filestats)) != 0 && language[0] &&

• if ((status = lstat(filename, filestats)) != 0 && language[0] &&
  strncmp(con->uri, "/icons/", 7) &&
  strncmp(con->uri, "/ppd/", 5) &&
  strncmp(con->uri, "/rss/", 5) &&
  @@ -3057,13 +3057,13 @@
  plen = len - (size_t)(ptr - filename);

  strlcpy(ptr, "index.html", plen);

•  status = stat(filename, filestats);
  

•  status = lstat(filename, filestats);
  

  #ifdef HAVE_JAVA
  if (status)
  {
  strlcpy(ptr, "index.class", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_JAVA */

@@ -3071,7 +3071,7 @@
if (status)
{
strlcpy(ptr, "index.pl", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PERL */

@@ -3079,7 +3079,7 @@
if (status)
{
strlcpy(ptr, "index.php", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PHP */

@@ -3087,18 +3087,28 @@
if (status)
{
strlcpy(ptr, "index.pyc", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }

  if (status)
  {
  strlcpy(ptr, "index.py", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PYTHON */

  }
  while (status && language[0]);

• /*
• * If we've found a symlink, 404 the sucker to avoid disclosing information.
• */

• if (!status && S_ISLNK(filestats->st_mode))
• {

•  cupsdLogClient(con, CUPSD_LOG_INFO, "Symlinks such as \"%s\" are not allowed.", filename);
  

•  return (NULL);
  

• }
  }

cupsdLogClient(con, CUPSD_LOG_DEBUG2, "get_file filestats=%p, filename=%p, len=" CUPS_LLFMT ", returning "%s".", filestats, filename, CUPS_LLCAST len, status ? "(null)" : filename);

[michaelrsweet]

"str4455_v2.patch":

Index: scheduler/client.c

--- scheduler/client.c (revision 12054)
+++ scheduler/client.c (working copy)
@@ -2959,7 +2959,7 @@

• then fallback to the default one...
  */

• if ((status = stat(filename, filestats)) != 0 && language[0] &&

• if ((status = lstat(filename, filestats)) != 0 && language[0] &&
  strncmp(con->uri, "/icons/", 7) &&
  strncmp(con->uri, "/ppd/", 5) &&
  strncmp(con->uri, "/rss/", 5) &&
  @@ -3057,13 +3057,13 @@
  plen = len - (size_t)(ptr - filename);

  strlcpy(ptr, "index.html", plen);

•  status = stat(filename, filestats);
  

•  status = lstat(filename, filestats);
  

  #ifdef HAVE_JAVA
  if (status)
  {
  strlcpy(ptr, "index.class", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_JAVA */

@@ -3071,7 +3071,7 @@
if (status)
{
strlcpy(ptr, "index.pl", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PERL */

@@ -3079,7 +3079,7 @@
if (status)
{
strlcpy(ptr, "index.php", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PHP */

@@ -3087,18 +3087,39 @@
if (status)
{
strlcpy(ptr, "index.pyc", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }

  if (status)
  {
  strlcpy(ptr, "index.py", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PYTHON */

  }
  while (status && language[0]);

• /*
• * If we've found a symlink, 404 the sucker to avoid disclosing information.
• */

• if (!status && S_ISLNK(filestats->st_mode))
• {

•  cupsdLogClient(con, CUPSD_LOG_INFO, "Symlinks such as \"%s\" are not allowed.", filename);
  

•  return (NULL);
  

• }

• /*
• * Similarly, if the file/directory does not have world read permissions, do
• * not allow access...
• */

• if (!status && !(filestats->st_mode & S_IROTH))
• {

•  cupsdLogClient(con, CUPSD_LOG_INFO, "Files/directories such as \"%s\" must be world-readable.", filename);
  

•  return (NULL);
  

• }
  }

cupsdLogClient(con, CUPSD_LOG_DEBUG2, "get_file filestats=%p, filename=%p, len=" CUPS_LLFMT ", returning "%s".", filestats, filename, CUPS_LLCAST len, status ? "(null)" : filename);

[michaelrsweet]

"str4455-1.7.patch":

Index: scheduler/client.c

--- scheduler/client.c (revision 12054)
+++ scheduler/client.c (working copy)
@@ -3310,7 +3310,7 @@

• then fallback to the default one...
  */

• if ((status = stat(filename, filestats)) != 0 && language[0] &&

• if ((status = lstat(filename, filestats)) != 0 && language[0] &&
  strncmp(con->uri, "/icons/", 7) &&
  strncmp(con->uri, "/ppd/", 5) &&
  strncmp(con->uri, "/rss/", 5) &&
  @@ -3408,13 +3408,13 @@
  plen = len - (ptr - filename);

  strlcpy(ptr, "index.html", plen);

•  status = stat(filename, filestats);
  

•  status = lstat(filename, filestats);
  

  #ifdef HAVE_JAVA
  if (status)
  {
  strlcpy(ptr, "index.class", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_JAVA */

@@ -3422,7 +3422,7 @@
if (status)
{
strlcpy(ptr, "index.pl", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PERL */

@@ -3430,7 +3430,7 @@
if (status)
{
strlcpy(ptr, "index.php", plen);

• status = stat(filename, filestats);
• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PHP */

@@ -3438,18 +3438,39 @@
if (status)
{
strlcpy(ptr, "index.pyc", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }

  if (status)
  {
  strlcpy(ptr, "index.py", plen);

• status = stat(filename, filestats);

• status = lstat(filename, filestats);
  }
  #endif /* HAVE_PYTHON */

  }
  while (status && language[0]);

• /*
• * If we've found a symlink, 404 the sucker to avoid disclosing information.
• */

• if (!status && S_ISLNK(filestats->st_mode))
• {

•  cupsdLogMessage(CUPSD_LOG_INFO, "[Client %d] Symlinks such as \"%s\" are not allowed.", con->http.fd, filename);
  

•  return (NULL);
  

• }

• /*
• * Similarly, if the file/directory does not have world read permissions, do
• * not allow access...
• */

• if (!status && !(filestats->st_mode & S_IROTH))
• {

•  cupsdLogMessage(CUPSD_LOG_INFO, "[Client %d] Files/directories such as \"%s\" must be world-readable.", con->http.fd, filename);
  

•  return (NULL);
  

• }
  }

cupsdLogMessage(CUPSD_LOG_DEBUG2,

[michaelrsweet]

CUPS.org User: jsmeix.suse

Lame self-reply because of my own lossy mind:
http://www.cups.org/pipermail/cups/2011-July/023248.html

[michaelrsweet]

CUPS.org User: twaugh.redhat

Filed as STR #4461.

[michaelrsweet]

CUPS.org User: twaugh.redhat

Note that STR #4461 also affects cupsd.conf, effectively breaking cupsctl for all situations except the special case of cupsctl being able to read the file directly.

[galaktipus]

Is there a commit for the issue?

[michaelrsweet]

@galaktipus Yes, 6 years ago in the old Subversion repository. Searching the git log output I found commit 9e92ab1 which contained this change.