[Pkg-shadow-devel] Bug#505071: Bug#505071: closed ... fixed in shadow 1:4.1.3-1
Paul Szabo
psz at maths.usyd.edu.au
Sat Apr 18 23:06:38 UTC 2009
Dear Nicolas,
> I changed src/login.c
> in libmisc/utmp.c, I only sanitized ut_line.
Thanks. New patches (replacing my previous ones) below, including the
move of endpwent(), and more verbose comments. The patch for login.c
is essential. The patch for utmp.c is mostly "as you wish"; there is a
missed endutent() there, am not sure whether correct or how important.
> utent.ut_host is only used to ...
> A forged ut_host does not seems critical.
Thanks, I agree.
Further comments:
Seems to me that su.c needs endgrent() for when SU_WHEEL_ONLY (this is
not a security issue).
In newgrp.c also, endpwent() needs to be moved to before the UID/GID
change (as was in login.c); I do not give patches. - Do these moves
warrant a DSA?
Cheers, Paul
Paul Szabo psz at maths.usyd.edu.au http://www.maths.usyd.edu.au/u/psz/
School of Mathematics and Statistics University of Sydney Australia
--- src/login.c.old 2009-04-17 07:00:50.000000000 +1000
+++ src/login.c 2009-04-19 08:54:00.000000000 +1000
@@ -449,6 +449,7 @@
#else
int delay;
struct spwd *spwd = NULL;
+ struct spwd spent;
#endif
/*
* Some quick initialization.
@@ -464,6 +465,16 @@
amroot = (getuid () == 0);
Prog = Basename (argv[0]);
+ /*
+ * PSz 18 Apr 09 Will this "work" unless we are root?
+ * Should we test
+ if (geteuid() != 0) {
+ fprintf (stderr, _("%s: Cannot possibly work without effective root\n"), Prog);
+ exit (1);
+ }
+ * ?
+ * Debian /bin/login is not setuid, cannot work without amroot.
+ */
process_flags (argc, argv);
@@ -477,9 +488,22 @@
* even if your getty is broken, or if something corrupts utmp,
* but users must "exec login" which will use the existing utmp
* entry (will not overwrite remote hostname). --marekm
+ * PSz 18 Apr 09 As seen in
+ * http://bugs.debian.org/505071
+ * http://bugs.debian.org/505271
+ * we cannot trust utmp: should set our own, new entry.
+ * On Debian at least, we will never succeed without amroot
+ * (should have exited already?), should never be picky...
*/
checkutmp (!amroot);
- STRFCPY (tty, utent.ut_line);
+ /*
+ * PSz 17 Apr 09 Though we may handle ut_line correctly (for Linux),
+ * we should not trust PAM_TTY to its vagaries...
+ *STRFCPY (tty, utent.ut_line);
+ */
+ tmp = ttyname (0);
+ if (NULL == tmp) { tmp = "UNKNOWN"; }
+ STRFCPY (tty, tmp);
#ifndef USE_PAM
is_console = console (tty);
#endif
@@ -496,6 +520,12 @@
* create the utmp entry and fill in ut_addr.
* gethostbyname() is not 100% reliable (the remote host may
* be unknown, etc.). --marekm
+ * PSz 18 Apr 09 As commented above we cannot trust
+ * utmp, should have set our new entry: should now fill
+ * in ut_addr and ut_host. Even if gethostbyname() fails
+ * because forward lookup is not provided, and whether
+ * this lookup will get back to the original IP...
+ * (should telnetd have refused for such hosts?).
*/
he = gethostbyname (hostname);
if (NULL != he) {
@@ -648,6 +678,11 @@
/*
* hostname & tty are either set to NULL or their correct values,
* depending on how much we know.
+ * PSz 18 Apr 09 PAM_RHOST and PAM_TTY must be kept safe:
+ * PAM_RHOST is used e.g. in pam_rhosts_auth. We only ever set
+ * hostname from rflg or hflg not from ut_host, so is safe.
+ * PAM_TTY is used e.g. in pam_time and pam_group. We set tty
+ * from ttyname not from ut_line, so is safe.
*/
retcode = pam_set_item (pamh, PAM_RHOST, hostname);
PAM_FAIL_CHECK;
@@ -1176,6 +1211,19 @@
}
}
+/* PSz 18 Apr 09 Must do this before changing UID, before user access */
+/*
+ * Copy spwd so can use in agecheck later, though I do not think that
+ * endspent() would invalidate the previously obtained reference
+ */
+ spent = *spwd;
+ endpwent (); /* stop access to password file */
+ endgrent (); /* stop access to group file */
+ endspent (); /* stop access to shadow passwd file */
+#ifdef SHADOWGRP
+ endsgent (); /* stop access to shadow group file */
+#endif
+
/* We call set_groups() above because this clobbers pam_groups.so */
#ifndef USE_PAM
if (setup_uid_gid (&pwent, is_console))
@@ -1250,7 +1298,8 @@
#endif
printf (".\n");
}
- agecheck (spwd);
+ /* agecheck (spwd); */
+ agecheck (&spent);
mailcheck (); /* report on the status of mail */
#endif /* !USE_PAM */
@@ -1269,12 +1318,15 @@
(void) signal (SIGHUP, SIG_DFL); /* added this. --marekm */
(void) signal (SIGINT, SIG_DFL); /* default interrupt signal */
- endpwent (); /* stop access to password file */
- endgrent (); /* stop access to group file */
- endspent (); /* stop access to shadow passwd file */
-#ifdef SHADOWGRP
- endsgent (); /* stop access to shadow group file */
-#endif
+/*
+ * PSz 18 Apr 09 Must do this before changing UID, before user access
+ * endpwent (); * stop access to password file *
+ * endgrent (); * stop access to group file *
+ * endspent (); * stop access to shadow passwd file *
+ * #ifdef SHADOWGRP
+ * endsgent (); * stop access to shadow group file *
+ * #endif
+ */
if (0 == pwent.pw_uid) {
SYSLOG ((LOG_NOTICE, "ROOT LOGIN %s", fromhost));
} else if (getdef_bool ("LOG_OK_LOGINS")) {
--- libmisc/utmp.c.old 2008-11-23 10:56:10.000000000 +1100
+++ libmisc/utmp.c 2009-04-19 08:09:22.000000000 +1000
@@ -68,17 +68,27 @@
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) {
+ /*
+ * PSz 19 Apr 09 Might as well just compare to ttyname():
+ * code below may be too complex (or unsafe/raceable).
+ */
+ if (strcmp(tty,ttyname(0))) {
return false;
} else {
return true;
}
+/*
+ * 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;
+ * }
+ */
}
/*
@@ -101,10 +111,29 @@
{
char *line;
struct utmp *ut;
+ char reuseid[32];
pid_t pid = getpid ();
setutent ();
+ /*
+ * PSz 19 Apr 09 I would like login.c to do its own utmp
+ * handling, setting up utmp/wtmp from scratch and un-setting
+ * them at exit. Did not do so because:
+ * - do not have "working examples" in front of me, there are
+ * too many different systems to support (am lazy);
+ * - change would require cooperation from telnetd etc, there
+ * would be duplication of entries until they stopped doing
+ * utmp.
+ * We re-use ut_id (if found below), but set everything else
+ * ourselves. Re-using ut_id avoids duplication of entries,
+ * and is "safe". By re-writing utmp completely, we may lose
+ * "important" data like login time or remote host: but only
+ * when these were set already, and user done a new login;
+ * this cannot happen when /bin/login is not setuid, cannot
+ * happen on Debian.
+ */
+
/* First, try to find a valid utmp entry for this process. */
while ((ut = getutent ()) != NULL) {
if ( (ut->ut_pid == pid)
@@ -121,29 +150,56 @@
/* If there is one, just use it, otherwise create a new one. */
if (NULL != ut) {
- utent = *ut;
+ strncpy (reuseid, utent.ut_id, sizeof reuseid);
} else {
if (picky) {
(void) puts (NO_UTENT);
exit (EXIT_FAILURE);
}
- line = ttyname (0);
- if (NULL == line) {
- (void) puts (NO_TTY);
- exit (EXIT_FAILURE);
- }
- if (strncmp (line, "/dev/", 5) == 0) {
- line += 5;
- }
- memset ((void *) &utent, 0, sizeof utent);
- utent.ut_type = LOGIN_PROCESS;
- utent.ut_pid = pid;
- strncpy (utent.ut_line, line, sizeof utent.ut_line);
- /* XXX - assumes /dev/tty?? or /dev/pts/?? */
+ reuseid[0] = '\0';
+/*
+ * line = ttyname (0);
+ * if (NULL == line) {
+ * (void) puts (NO_TTY);
+ * exit (EXIT_FAILURE);
+ * }
+ * if (strncmp (line, "/dev/", 5) == 0) {
+ * line += 5;
+ * }
+ * memset ((void *) &utent, 0, sizeof utent);
+ * utent.ut_type = LOGIN_PROCESS;
+ * utent.ut_pid = pid;
+ * strncpy (utent.ut_line, line, sizeof utent.ut_line);
+ * * 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);
+ */
+ }
+ line = ttyname (0);
+ if (NULL == line) {
+ (void) puts (NO_TTY);
+ exit (EXIT_FAILURE);
+ }
+ if (strncmp (line, "/dev/", 5) == 0) {
+ line += 5;
+ }
+ memset ((void *) &utent, 0, sizeof utent);
+ utent.ut_type = LOGIN_PROCESS;
+ utent.ut_pid = pid;
+ strncpy (utent.ut_line, line, sizeof utent.ut_line);
+ /* XXX - assumes /dev/tty?? or /dev/pts/?? */
+ if ('\0' != reuseid[0]) {
+ strncpy (utent.ut_id, reuseid, sizeof utent.ut_id);
+ } else {
strncpy (utent.ut_id, utent.ut_line + 3, sizeof utent.ut_id);
- strcpy (utent.ut_user, "LOGIN");
- utent.ut_time = time (NULL);
}
+ strcpy (utent.ut_user, "LOGIN");
+ utent.ut_time = time (NULL);
+
+ /* PSz 19 Apr 09 Seems we missed endutent()...
+ * not important, would be done later in setutmp */
+ endutent ();
}
#elif defined(LOGIN_PROCESS)
@@ -438,6 +494,10 @@
utxent = utxline;
utent = utline;
+ /* PSz 19 Apr 09 Seems we missed endutent()... */
+ endutxent ();
+ endutent ();
+
return err;
}
More information about the Pkg-shadow-devel
mailing list