From a324a7f13fcb0dcae6706921f209dd0bfa6a6f94 Mon Sep 17 00:00:00 2001 From: nekral-guest Date: Sat, 22 Nov 2008 23:56:11 +0000 Subject: [PATCH] * NEWS, libmisc/chowntty.c, libmisc/utmp.c: is_my_tty() moved from utmp.c to chowntty.c. checkutmp() now only uses an existing utmp entry if the pid matches and ut_line matches with the current tty. This fixes a possible DOS when entries can be forged in the utmp file. * libmisc/chowntty.c, src/login.c, lib/prototypes.h: Remove the tty argument from chown_tty. chown_tty always changes stdin and does not need this argument anymore. --- ChangeLog | 8 ++++++++ NEWS | 2 ++ libmisc/chowntty.c | 32 +------------------------------- libmisc/utmp.c | 41 +++++++++++++++++++++++++++++++++++++---- src/login.c | 2 +- 5 files changed, 49 insertions(+), 36 deletions(-) diff --git a/ChangeLog b/ChangeLog index 31d5597f..7c551c5c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,14 @@ * NEWS, libmisc/chowntty.c: Fix a race condition that could lead to gaining ownership or changing mode of arbitrary files. + * NEWS, libmisc/chowntty.c, libmisc/utmp.c: is_my_tty() moved from + utmp.c to chowntty.c. checkutmp() now only uses an existing utmp + entry if the pid matches and ut_line matches with the current tty. + This fixes a possible DOS when entries can be forged in the utmp + file. + * libmisc/chowntty.c, src/login.c, lib/prototypes.h: Remove the + tty argument from chown_tty. chown_tty always changes stdin and + does not need this argument anymore. 2008-10-11 Nicolas François diff --git a/NEWS b/NEWS index b91ec42e..7aab4d40 100644 --- a/NEWS +++ b/NEWS @@ -64,6 +64,8 @@ shadow-4.1.2.1 -> shadow-4.1.2.2 23-11-2008 *** security - Fix a race condition in login that could lead to gaining ownership or changing mode of arbitrary files. +- Fix a possible login DOS, which could be caused by injecting forged + entries in utmp. shadow-4.1.2 -> shadow-4.1.2.1 26-06-2008 diff --git a/libmisc/chowntty.c b/libmisc/chowntty.c index 4a4d0aeb..89ebb0e8 100644 --- a/libmisc/chowntty.c +++ b/libmisc/chowntty.c @@ -43,32 +43,14 @@ #include "defines.h" #include #include "getdef.h" -/* - * is_my_tty -- determine if "tty" is the same as TTY stdin is using - */ -static bool is_my_tty (const char *tty) -{ - struct stat by_name, by_fd; - - if ((stat (tty, &by_name) != 0) || (fstat (0, &by_fd) != 0)) { - return false; - } - - if (by_name.st_rdev != by_fd.st_rdev) { - return false; - } else { - return true; - } -} /* * chown_tty() sets the login tty to be owned by the new user ID * with TTYPERM modes */ -void chown_tty (const char *tty, const struct passwd *info) +void chown_tty (const struct passwd *info) { - char buf[200], full_tty[200]; char *group; /* TTY group name or number */ struct group *grent; gid_t gid; @@ -97,18 +79,6 @@ void chown_tty (const char *tty, const struct passwd *info) * the group as determined above. */ - if ('/' != *tty) { - snprintf (full_tty, sizeof full_tty, "/dev/%s", tty); - tty = full_tty; - } - - if (!is_my_tty (tty)) { - SYSLOG ((LOG_WARN, - "unable to determine TTY name, got %s\n", tty)); - closelog (); - exit (1); - } - if ( (fchown (STDIN_FILENO, info->pw_uid, gid) != 0) || (fchmod (STDIN_FILENO, getdef_num ("TTYPERM", 0600)) != 0)) { int err = errno; diff --git a/libmisc/utmp.c b/libmisc/utmp.c index c9169030..5218a1e3 100644 --- a/libmisc/utmp.c +++ b/libmisc/utmp.c @@ -56,6 +56,31 @@ struct utmp utent; #define NO_TTY \ _("Unable to determine your tty name.") +/* + * is_my_tty -- determine if "tty" is the same TTY stdin is using + */ +static bool is_my_tty (const char *tty) +{ + char full_tty[200]; + struct stat by_name, by_fd; + + if ('/' != *tty) { + snprintf (full_tty, sizeof full_tty, "/dev/%s", tty); + tty = full_tty; + } + + if ( (stat (tty, &by_name) != 0) + || (fstat (STDIN_FILENO, &by_fd) != 0)) { + return false; + } + + if (by_name.st_rdev != by_fd.st_rdev) { + return false; + } else { + return true; + } +} + /* * checkutmp - see if utmp file is correct for this process * @@ -81,16 +106,20 @@ void checkutmp (bool picky) setutent (); /* First, try to find a valid utmp entry for this process. */ - while ((ut = getutent ()) != NULL) + while ((ut = getutent ()) != NULL) { if ( (ut->ut_pid == pid) && ('\0' != ut->ut_line[0]) && ('\0' != ut->ut_id[0]) && ( (LOGIN_PROCESS == ut->ut_type) - || (USER_PROCESS == ut->ut_type))) { + || (USER_PROCESS == ut->ut_type)) + /* A process may have failed to close an entry + * Check if this entry refers to this tty */ + && is_my_tty (ut->ut_line)) { break; } + } - /* If there is one, just use it, otherwise create a new one. */ + /* If there is one, just use it, otherwise create a new one. */ if (NULL != ut) { utent = *ut; } else { @@ -110,7 +139,7 @@ void checkutmp (bool picky) utent.ut_type = LOGIN_PROCESS; utent.ut_pid = pid; strncpy (utent.ut_line, line, sizeof utent.ut_line); - /* XXX - assumes /dev/tty?? */ + /* XXX - assumes /dev/tty?? or /dev/pts/?? */ strncpy (utent.ut_id, utent.ut_line + 3, sizeof utent.ut_id); strcpy (utent.ut_user, "LOGIN"); utent.ut_time = time (NULL); @@ -173,6 +202,10 @@ void checkutmp (bool picky) * the structure. The UNIX/PC is broken in this regard * and needs help ... */ + /* XXX: The ut_line may not match with the current tty. + * ut_line will be set by setutmp anyway, but ut_id + * will not be set, and the wrong utmp entry may be + * updated. -- nekral */ if (utent.ut_line[0] == '\0') #endif /* !UNIXPC */ diff --git a/src/login.c b/src/login.c index 5cde73ab..5f305874 100644 --- a/src/login.c +++ b/src/login.c @@ -1094,7 +1094,7 @@ int main (int argc, char **argv) } setup_limits (&pwent); /* nice, ulimit etc. */ #endif /* ! USE_PAM */ - chown_tty (tty, &pwent); + chown_tty (&pwent); #ifdef USE_PAM /*