Setuid programs should not be allowed to override default dirs #3482

Closed
michaelrsweet opened this Issue Jan 22, 2010 · 5 comments

Comments

Projects
None yet
1 participant
Collaborator

michaelrsweet commented Jan 22, 2010

Version: 1.4-current
CUPS.org User: mike

[Received email directly from reporter...]

From: Ronald Volgers r.c.volgers@student.utwente.nl
Subject: [CUPS] Security vulnerability in lppasswd
Date: January 22, 2010 11:22:42 AM PST
To: Michael Sweet msweet@apple.com
Cc: security@debian.org

Hi Michael,

I recently found a security vulnerability in the lppasswd utility which
can lead to system compromise. Attached is a text file with the details.

I'm CC'ing debian security on this as well since I run Debian and have
disclosed bugs through them in the past.

I will not be publishing this until a fix is released. I'd appreciate it
if you could let me know when you have a timeline for the fix.

Please note that I do not have access to an OSX system, so all of my
information is about CUPS on Linux. I'd very much appreciate it if you
could tell me something about the impact on OSX.

Hope you find my information of use.

Greetings,
Ronald.

Collaborator

michaelrsweet commented Jan 22, 2010

CUPS.org User: mike

[My response to reporter]

FWIW, lppasswd is no longer installed setuid in CUPS 1.4.x and later making this sort of attack fail out-of-the-box.

For 1.3.x and earlier, plus users that enable the setuid bit on later releases, we can add setuid detection in the cups_env_init function (cups/globals.c) so that the environment variables are not used when the program is run setuid.

Index: cups/globals.c

--- cups/globals.c (revision 8960)
+++ cups/globals.c (working copy)
@@ -38,6 +38,20 @@
static void
cups_env_init(_cups_globals_t g) / I - Global data */
{

  • if (getuid() != geteuid())
  • {
  • /*
  • * Running setuid, use default directories...
  • */
  • g->cups_datadir = CUPS_DATADIR;
  • g->cups_serverbin = CUPS_SERVERBIN;
  • g->cups_serverroot = CUPS_SERVERROOT;
  • g->cups_statedir = CUPS_STATEDIR;
  • g->localedir = CUPS_LOCALEDIR;
  • return;
  • }

if ((g->cups_datadir = getenv("CUPS_DATADIR")) == NULL)
g->cups_datadir = CUPS_DATADIR;

Collaborator

michaelrsweet commented Jan 22, 2010

CUPS.org User: mike

An additional change would be to add the format string validator from the checkpo utility to _cupsMessageLoad so that we could invalidate (and not add) messages that did not match what we expect.

Collaborator

michaelrsweet commented Feb 2, 2010

CUPS.org User: mike

Updated patch attached that adds a setgid check and makes lppasswd use _cupsGlobals() to get the CUPS_SERVERROOT value (so all of the setuid/setgid code is in one place in libcups...)

Collaborator

michaelrsweet commented Mar 30, 2010

"str3482.patch":

Index: cups/globals.c

--- cups/globals.c (revision 8973)
+++ cups/globals.c (working copy)
@@ -38,20 +38,44 @@
static void
cups_env_init(_cups_globals_t g) / I - Global data */
{

  • if ((g->cups_datadir = getenv("CUPS_DATADIR")) == NULL)
  • g->cups_datadir = CUPS_DATADIR;
    +#ifdef HAVE_GETEUID
  • if ((geteuid() != getuid() && getuid()) || getegid() != getgid())
    +#else
  • if (!getuid())
    +#endif /* HAVE_GETEUID */
  • {
  • /*
  • * When running setuid/setgid, don't allow environment variables to override
  • * the directories...
  • */
  • if ((g->cups_serverbin = getenv("CUPS_SERVERBIN")) == NULL)

- g->cups_serverbin = CUPS_SERVERBIN;

  • if ((g->cups_serverroot = getenv("CUPS_SERVERROOT")) == NULL)
  • g->cups_datadir = CUPS_DATADIR;
  • g->cups_serverbin = CUPS_SERVERBIN;
    g->cups_serverroot = CUPS_SERVERROOT;
  • g->cups_statedir = CUPS_STATEDIR;
  • g->localedir = CUPS_LOCALEDIR;
  • }
  • else
  • {
  • /*
  • * Allow directories to be overridden by environment variables.
  • */
  • if ((g->cups_statedir = getenv("CUPS_STATEDIR")) == NULL)
  • g->cups_statedir = CUPS_STATEDIR;
  • if ((g->cups_datadir = getenv("CUPS_DATADIR")) == NULL)
  •  g->cups_datadir = CUPS_DATADIR;
    
  • if ((g->localedir = getenv("LOCALEDIR")) == NULL)
  • g->localedir = CUPS_LOCALEDIR;
  • if ((g->cups_serverbin = getenv("CUPS_SERVERBIN")) == NULL)
  •  g->cups_serverbin = CUPS_SERVERBIN;
    
  • if ((g->cups_serverroot = getenv("CUPS_SERVERROOT")) == NULL)
  •  g->cups_serverroot = CUPS_SERVERROOT;
    
  • if ((g->cups_statedir = getenv("CUPS_STATEDIR")) == NULL)
  •  g->cups_statedir = CUPS_STATEDIR;
    
  • if ((g->localedir = getenv("LOCALEDIR")) == NULL)
  •  g->localedir = CUPS_LOCALEDIR;
    
  • }
    }

Index: systemv/lppasswd.c

--- systemv/lppasswd.c (revision 8973)
+++ systemv/lppasswd.c (working copy)
@@ -31,9 +31,7 @@
#include <sys/types.h>
#include <sys/stat.h>

-#include <cups/string.h>
-#include <cups/cups.h>
-#include <cups/i18n.h>
+#include <cups/globals.h>
#include <cups/md5.h>

#ifndef WIN32
@@ -79,7 +77,6 @@
groupline[17], /* Group from line /
md5line[33], /
MD5-sum from line /
md5new[33]; /
New MD5 sum */

  • const char root; / CUPS server root directory /
    char passwdmd5[1024], /
    passwd.md5 file /
    passwdold[1024], /
    passwd.old file /
    passwdnew[1024]; /
    passwd.tmp file /
    @@ -88,6 +85,7 @@
    int flag; /
    Password check flags... /
    int fd; /
    Password file descriptor /
    int error; /
    Write error */
  • _cups_globals_t _cg = cupsGlobals(); / Global data /
    #if defined(HAVE_SIGACTION) && !defined(HAVE_SIGSET)
    struct sigaction action; /
    Signal action /
    #endif /
    HAVE_SIGACTION && !HAVE_SIGSET*/
    @@ -113,19 +111,12 @@

/*

  • Find the server directory...
  • * We use the CUPS_SERVERROOT environment variable when we are running
    • as root or when lppasswd is not setuid...
      */
  • if ((root = getenv("CUPS_SERVERROOT")) == NULL ||
  •  (getuid() != geteuid() && getuid()))
    
  • root = CUPS_SERVERROOT;
  • snprintf(passwdmd5, sizeof(passwdmd5), "%s/passwd.md5", cg->cups_serverroot);
  • snprintf(passwdold, sizeof(passwdold), "%s/passwd.old", cg->cups_serverroot);
  • snprintf(passwdnew, sizeof(passwdnew), "%s/passwd.new", cg->cups_serverroot);
  • snprintf(passwdmd5, sizeof(passwdmd5), "%s/passwd.md5", root);
  • snprintf(passwdold, sizeof(passwdold), "%s/passwd.old", root);

- snprintf(passwdnew, sizeof(passwdnew), "%s/passwd.new", root);

/*

  • Find the default system group...
    */
Collaborator

michaelrsweet commented Mar 30, 2010

CUPS.org User: mike

Fixed in Subversion repository.

michaelrsweet added this to the Stable milestone Mar 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment