openssh bugs - Feb 2017 - [Bug 2681] New: postauth processes to log via monitor

https://bugzilla.mindrot.org/show_bug.cgi?id=2681

            Bug ID: 2681
           Summary: postauth processes to log via monitor
           Product: Portable OpenSSH
           Version: 7.4p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2945
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2945&action=edit
log in postauth via monitor (if there is no /dev/log)

There is a long standing problem with logging in chroots. Especially,
when you use %u in ChrootDirectory, it is nearly impossible to have
/dev/log in every possible chroot for all users.

It seems to be important mainly for sftp-internal session which are
simply configurable to be chrooted and where admins would like to log
sftp session commands.

Similar way as in the pre-authentication phase, we can log the events
in the postauth phase if we know the postauth process will not be able
to open its own /dev/log (generally in chroot).

How does it work?

We are trying to solve this problem on two fronts:
 - In do_child, we check if the /dev/log is available in the chroot and
if not, we "leak the FD" to the internal-sftp process. We also
postpone
the closefrom() call after the internal-sftp call.
 - In privsep_postauth(), we have the same check (it could be probably
written more nicely) which takes care of setting up log FDs going
through the monitor.

The idea is that this change should not modify behavior of the existing
setup in case the /dev/log is available in chroot.


Originally posted in on the mailing list over 2 years ago [1] and
discussed in Red Hat bugzilla years ago [2]

I am not sure if there are some other platforms without the /dev/log
concept, but if so, there is still the possibility to make it runtime
option. We are using the attached patch for 2 years in Fedora/RHEL7


[1]
https://lists.mindrot.org/pipermail/openssh-unix-dev/2014-October/033011.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1083482#c13

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2681

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2945
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2945
log in postauth via monitor (if there is no /dev/log)
> void
>-monitor_reinit(struct monitor *mon)
>+monitor_reinit(struct monitor *mon, const char *chroot_dir)
> {
>-	monitor_openfds(mon, 0);
>+	struct stat dev_log_stat;
>+	char *dev_log_path;
>+	int do_logfds = 0;
>+
>+	if (chroot_dir != NULL) {
>+		xasprintf(&dev_log_path, "%s/dev/log", chroot_dir);
>+
>+		if (stat(dev_log_path, &dev_log_stat) != 0) {
>+			debug("%s: /dev/log doesn't exist in %s chroot - will try to
log via monitor using [postauth] suffix", __func__, chroot_dir);
>+			do_logfds = 1;
I think it's simpler to log via the monitor unconditionally. There are
fewer paths to think about that way.
> static char *auth_sock_name = NULL;
>@@ -365,8 +366,8 @@ do_exec_no_pty(Session *s, const char *c
> 		is_child = 1;
> 
> 		/* Child.  Reinitialize the log since the pid has changed. */
>-		log_init(__progname, options.log_level,
>-		    options.log_facility, log_stderr);
>+		log_init_handler(__progname, options.log_level,
>+		    options.log_facility, log_stderr, have_dev_log);
I'm not sure whether this is needed anymore. It seems like a holdover
from when log_init() called openlog() itself, but it stopped doing that
in <checks> November 1999 :)
>-		log_init(__progname, options.log_level,
>-		    options.log_facility, log_stderr);
>+		log_init_handler(__progname, options.log_level,
>+		    options.log_facility, log_stderr, have_dev_log);
ditto
>@@ -619,6 +620,7 @@ do_exec(Session *s, const char *command)
> 	int ret;
> 	const char *forced = NULL, *tty = NULL;
> 	char session_type[1024];
>+	struct stat dev_log_stat;
> 
> 	if (options.adm_forced_command) {
> 		original_command = command;
>@@ -676,6 +678,10 @@ do_exec(Session *s, const char *command)
> 			tty += 5;
> 	}
> 
>+	if (lstat("/dev/log", &dev_log_stat) != 0) {
>+		have_dev_log = 0;
>+	}
>+
ditto re always logging via monitor
>-	/*
>-	 * Close any extra open file descriptors so that we don't have them
>-	 * hanging around in clients.  Note that we want to do this after
>-	 * initgroups, because at least on Solaris 2.3 it leaves file
>-	 * descriptors open.
>-	 */
>-	closefrom(STDERR_FILENO + 1);
If you remove this then I think you need to add an explicit closefrom()
before the do_pwchange() call in do_child().
>-	closefrom(STDERR_FILENO + 1);
I don't think this one should be removed. IMO it would be better
arrange for the log socket to be on fd=4 and closefrom(5) instead (with
a comment explaining why).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2681

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
(In reply to Damien Miller from comment #1)
> I think it's simpler to log via the monitor unconditionally. There
> are fewer paths to think about that way.
If we are logging using monitor, it significantly more complicated to
filter these logs afterward in syslog, because all of them will be
originating from the same process and from the same /dev/log.

On the other hand, logging using existing /dev/log in chroot makes it
very simple to filter the logs from different chroots for example to
different files (though it does not scale very well).
> >-	/*
> >-	 * Close any extra open file descriptors so that we don't have
them
> >-	 * hanging around in clients.  Note that we want to do this after
> >-	 * initgroups, because at least on Solaris 2.3 it leaves file
> >-	 * descriptors open.
> >-	 */
> >-	closefrom(STDERR_FILENO + 1);
> 
> If you remove this then I think you need to add an explicit
> closefrom() before the do_pwchange() call in do_child().
That would probably make sense. Good catch.
> >-	closefrom(STDERR_FILENO + 1);
> 
> I don't think this one should be removed. IMO it would be better
> arrange for the log socket to be on fd=4 and closefrom(5) instead
> (with a comment explaining why).
Well ... this was moved after the internal-sftp call so we can "leak"
that file descriptor into the internal-sftp. This is executed in all
the other code paths (though possibly later).
I am not sure if there is sane portable way to ensure the log fd will
be 4.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2681

--- Comment #3 from Jakub Jelen <jjelen at redhat.com>
---
> I'm not sure whether this is needed anymore. It seems like a holdover
from when log_init() called openlog() itself, but it stopped doing that in
<checks> November 1999 :)
Really? I still see that log_init() calls openlog() in current master
(at least portable).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.