| Summary: | postauth processes to log via monitor | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | Jakub Jelen <jjelen> | ||||
| Component: | sshd | Assignee: | Assigned to nobody <unassigned-bugs> | ||||
| Status: | NEW --- | ||||||
| Severity: | enhancement | ||||||
| Priority: | P5 | ||||||
| Version: | 7.4p1 | ||||||
| Hardware: | Other | ||||||
| OS: | Linux | ||||||
| Attachments: |
|
||||||
|
Description
Jakub Jelen
2017-02-21 01:22:52 AEDT
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).
|