hi again -
two more fixes for problems i noticed (patches against 1.0 / cvs):
1. make the change_user function more secure by aborting if setuid,
setgid, or chdir fail.
2. make `exec' commands fire even if the user is not logged into tty.
i'm pretty sure this was the intent, given the xmessage example, which
currently won't work if the user isn't also currently running an xterm
or something like that. more details in the patch preamble.
-damon
Submitted by: Damon Harper <[EMAIL PROTECTED]>
Date: 2006-08-28
Summary: Check for errors in comsatd change_user and exit if anything fails
In the change_user function of comsatd, no checks were performed on
the exit values of setgid, setuid and chdir. This could theoretically
lead to privilege escalation if setuid or setgid fails. With this
patch, comsatd will exit if one of these commands fails.
--- mailutils-1.0.orig/comsat/comsat.c 2005-09-30 03:41:07.000000000 -0700
+++ mailutils-1.0/comsat/comsat.c 2006-08-27 14:17:16.000000000 -0700
@@ -583,9 +583,21 @@
exit (1);
}
- setgid (pw->pw_gid);
- setuid (pw->pw_uid);
- chdir (pw->pw_dir);
+ if (setgid (pw->pw_gid))
+ {
+ syslog (LOG_CRIT, _("Cannot set GID %d for user %s: %m"), pw->pw_gid,
user);
+ exit (1);
+ }
+ if (setuid (pw->pw_uid))
+ {
+ syslog (LOG_CRIT, _("Cannot set UID %d for user %s: %m"), pw->pw_uid,
user);
+ exit (1);
+ }
+ if(chdir (pw->pw_dir))
+ {
+ syslog (LOG_CRIT, _("Cannot chdir to %s for user %s: %m"), pw->pw_dir,
user);
+ exit (1);
+ }
username = user;
}
Submitted by: Damon Harper <[EMAIL PROTECTED]>
Date: 2006-08-28
Summary: Run comsatd exec actions even if user is not logged in
Currently, comsatd's child process simply exits if it cannot find a
tty where the user to be notified is logged in. This is fine for the
default action, or echo and beep actions; however, this also prevents
exec actions from running unless the user is actively logged in at the
time of notification.
This should not be the desired behaviour. A user might use exec to
perform an action that should fire off even if that user is not logged
in to a tty. In the comsatd documentation, an example is given of an
exec command that will run xmessage to show a message in x-windows.
With the current behaviour, that example will only work if the user is
also logged in to a tty (perhaps via xterm) at the time of the
message.
This patch changes the semantics of find_user, so that the tty is set
to the device name if a usable tty is found, but is otherwise set to
NULL. The return value of find_user is still provided but is now
ignored. run_user_action, open_rc, and action_* functions are now
reworked to assume that tty could be NULL. action_beep and
action_echo both return immediately if this is the case, but
action_exec simply continues with all output directed to /dev/null.
This patch also fixes all functions to use the cr string instead of
hard-coded \r\n when writing to the tty.
diff -urN mailutils-1.0.orig/comsat/action.c mailutils-1.0/comsat/action.c
--- mailutils-1.0.orig/comsat/action.c 2005-08-29 03:21:54.000000000 -0700
+++ mailutils-1.0/comsat/action.c 2006-08-27 15:37:06.000000000 -0700
@@ -233,6 +233,8 @@
static void
action_beep (FILE *tty)
{
+ if (tty == NULL)
+ return;
fprintf (tty, "\a\a");
}
@@ -241,6 +243,8 @@
{
if (!str)
return;
+ if (tty == NULL)
+ return;
for (; *str; str++)
{
if (*str == '\n')
@@ -290,11 +294,14 @@
pid = fork ();
if (pid == 0)
{
- close (1);
- close (2);
- dup2 (fileno (tty), 1);
- dup2 (fileno (tty), 2);
- fclose (tty);
+ if (tty != NULL)
+ {
+ close (1);
+ close (2);
+ dup2 (fileno (tty), 1);
+ dup2 (fileno (tty), 2);
+ fclose (tty);
+ }
execv (argv[0], argv);
syslog (LOG_ERR, _("Cannot execute %s: %s"), argv[0], strerror (errno));
exit (0);
@@ -302,7 +309,7 @@
}
static FILE *
-open_rc (const char *filename, FILE *tty)
+open_rc (const char *filename, FILE *tty, char *cr)
{
struct stat stb;
struct passwd *pw = getpwnam (username);
@@ -320,8 +327,9 @@
}
if ((stb.st_mode & 0777) != 0600)
{
- fprintf (tty, "%s\r\n",
- _("Warning: your .biffrc has wrong permissions"));
+ if (tty != NULL)
+ fprintf (tty, "%s%s",
+ _("Warning: your .biffrc has wrong permissions"), cr);
syslog (LOG_NOTICE, _("%s's %s has wrong permissions"),
username, filename);
return NULL;
@@ -338,7 +346,7 @@
char *stmt = NULL;
size_t size = 0;
- fp = open_rc (BIFF_RC, tty);
+ fp = open_rc (BIFF_RC, tty, cr);
if (fp)
{
int line = 0;
@@ -379,8 +387,11 @@
}
else
{
- fprintf (tty, _(".biffrc:%d: unknown keyword"), line);
- fprintf (tty, "\r\n");
+ if (tty != NULL)
+ {
+ fprintf (tty, _(".biffrc:%d: unknown keyword"), line);
+ fprintf (tty, "%s", cr);
+ }
syslog (LOG_ERR, _("%s:.biffrc:%d: unknown keyword %s"),
username, line, argv[0]);
break;
diff -urN mailutils-1.0.orig/comsat/comsat.c mailutils-1.0/comsat/comsat.c
--- mailutils-1.0.orig/comsat/comsat.c 2005-09-30 03:41:07.000000000 -0700
+++ mailutils-1.0/comsat/comsat.c 2006-08-27 15:37:37.000000000 -0700
@@ -103,8 +103,8 @@
static void comsat_daemon_init (void);
static void comsat_daemon (int _port);
static int comsat_main (int fd);
-static void notify_user (const char *user, const char *device, const char
*path, off_t offset);
-static int find_user (const char *name, char *tty);
+static void notify_user (const char *user, const char *path, off_t offset);
+static int find_user (const char *name, char **tty);
static char *mailbox_path (const char *user);
static void change_user (const char *user);
@@ -322,7 +322,6 @@
struct sockaddr_in sin_from;
char buffer[216]; /*FIXME: Arbitrary size */
pid_t pid;
- char tty[MAX_TTY_SIZE];
char *p, *endp;
size_t offset;
char *path = NULL;
@@ -374,9 +373,6 @@
syslog (LOG_ERR, _("Malformed input: [EMAIL PROTECTED] (near %s)"),
buffer, p, endp);
}
- if (find_user (buffer, tty) != SUCCESS)
- return 0;
-
/* All I/O is done by child process. This is to avoid various blocking
problems. */
@@ -399,7 +395,7 @@
}
/* Child: do actual I/O */
- notify_user (buffer, tty, path, offset);
+ notify_user (buffer, path, offset);
exit (0);
}
@@ -422,9 +418,10 @@
/* NOTE: Do not bother to free allocated memory, as the program exits
immediately after executing this */
static void
-notify_user (const char *user, const char *device, const char *path, off_t
offset)
+notify_user (const char *user, const char *path, off_t offset)
{
- FILE *fp;
+ char *tty;
+ FILE *fp = NULL;
const char *cr;
char *blurb;
mu_mailbox_t mbox = NULL, tmp = NULL;
@@ -435,14 +432,17 @@
size_t count, n;
change_user (user);
- if ((fp = fopen (device, "w")) == NULL)
+ find_user (user, &tty);
+ if (tty)
{
- syslog (LOG_ERR, _("Cannot open device %s: %m"), device);
- exit (0);
+ if ((fp = fopen (tty, "w")) == NULL)
+ {
+ syslog (LOG_ERR, _("Cannot open tty device %s: %m"), tty);
+ exit (0);
+ }
+ cr = get_newline_str (fp);
}
- cr = get_newline_str (fp);
-
if (!path)
{
path = mailbox_path (user);
@@ -502,21 +502,27 @@
mu_mailbox_get_message (tmp, 1, &msg);
run_user_action (fp, cr, msg);
- fclose (fp);
+ if (tty)
+ {
+ fclose (fp);
+ free (tty);
+ }
}
/* Search utmp for the local user */
static int
-find_user (const char *name, char *tty)
+find_user (const char *name, char **tty)
{
UTMP *uptr;
int status;
struct stat statb;
- char ftty[MAX_TTY_SIZE];
+ char trytty[MAX_TTY_SIZE];
+ char *ftty;
time_t last_time = 0;
status = NOT_HERE;
- sprintf (ftty, "%s/", PATH_TTY_PFX);
+ *tty = NULL;
+ sprintf (trytty, "%s/", PATH_TTY_PFX);
SETUTENT ();
@@ -529,24 +535,24 @@
if (!strncmp (uptr->ut_name, name, sizeof(uptr->ut_name)))
{
/* no particular tty was requested */
- strncpy (ftty + sizeof(PATH_DEV),
+ strncpy (trytty + sizeof(PATH_DEV),
uptr->ut_line,
- sizeof (ftty) - sizeof (PATH_DEV) - 2);
- ftty[sizeof (ftty) - 1] = 0;
+ sizeof (trytty) - sizeof (PATH_DEV) - 2);
+ trytty[sizeof (trytty) - 1] = 0;
- mu_normalize_path (ftty, "/");
- if (strncmp (ftty, PATH_TTY_PFX, strlen (PATH_TTY_PFX)))
+ mu_normalize_path (trytty, "/");
+ if (strncmp (trytty, PATH_TTY_PFX, strlen (PATH_TTY_PFX)))
{
/* An attempt to break security... */
- syslog (LOG_ALERT, _("Bad line name in utmp record: %s"), ftty);
+ syslog (LOG_ALERT, _("Bad line name in utmp record: %s"), trytty);
return NOT_HERE;
}
- if (stat (ftty, &statb) == 0)
+ if (stat (trytty, &statb) == 0)
{
if (!S_ISCHR (statb.st_mode))
{
- syslog (LOG_ALERT, _("Not a character device: %s"), ftty);
+ syslog (LOG_ALERT, _("Not a character device: %s"), trytty);
return NOT_HERE;
}
@@ -559,8 +565,11 @@
if (statb.st_atime > last_time)
{
last_time = statb.st_atime;
- strcpy(tty, ftty);
- status = SUCCESS;
+ if ((ftty = malloc (strlen (trytty) + 1)) != NULL)
+ {
+ strcpy (ftty, trytty);
+ status = SUCCESS;
+ }
}
continue;
}
@@ -568,6 +577,8 @@
}
ENDUTENT ();
+ if (status == SUCCESS)
+ *tty = ftty;
return status;
}
_______________________________________________
Bug-mailutils mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/bug-mailutils
