Created attachment 2945 [details] 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
Comment on attachment 2945 [details] 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).
(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.
> 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).