There are multiple occurences of log() et al using untrusted data (received from a client, read from ~/.ssh/*), including any special characters, as a part of the message. On some systems, namely Solaris (up to and including version 8), this is a bad thing because syslog() passes any character it gets to syslogd, including a newline that is interpreted as a message separator. For instance, when a single LF is sent to port 22, the following is logged: Feb 12 23:29:56 blah sshd[1234]: Bad protocol version identification ' Feb 12 23:29:56 blah ' from 127.0.0.1 This way, sshd could be abused to generate bogus syslog messages by remote (!) users. Yes, such an implementation of syslog()/syslogd is stupid but it is much easier to patch OpenSSH than Solaris. :) I made a patch against 3.0.2p1 processing untrusted data with vis() whenever they are passed to log() et al (I had to re-add vis.[ch]) but I am not sure whether I got all cases covered. It would probably be more efficient to add vis() to do_log()...as long as the potential ambiguity in cases where more than one piece of untrusted data is logged (e.g. method and authctx->user in auth_log()) is considered harmless.
that's strange, either syslog() or syslogd should do vis()
The OS was S-o-l-a-r-i-s. Not OpenBSD. :)
we should perhaps vis(3) wrap log calls.
I agree that it should be syslog's responsability to safely encode any untrusted data. I hope you filed a bug with Sun too :) Here's an untested patch which runs all syslog data through vis()
Created attachment 79 [details] Process all syslog data through vis()
A while ago vis.[ch] was removed because it wasn't used anywhere. This was helpful for another reason: AT&T's graphviz package defines a completely different vis() and has a vis.h. Since -I$(srcdir)/openbsd-compat was removed, it would always find the vis.h in /usr/local/include on systems with graphviz installed and die. If this patch goes in, it'd be nice if the prototype for vis() could be added someplace else, instead of relying on finding vis.h in the -I searchpath.
Patch applied. I don't think there should be header file clashes with graphviz, but please file separate bug reports if there are.
Mass change of RESOLVED bugs to CLOSED
The current code in log.c fails to address the problem. It does strnvis(fmtbuf, msgbuf, sizeof(fmtbuf), VIS_SAFE|VIS_OCTAL); but this leaves nasty characters like LFs (misinterpreted by Solaris syslog()...see above) alone. On the other hand, stricter vis() flags make debugging output (sshd -d) ugly because some debugging message include a "natural" newline. I think the code should read: strnvis(fmtbuf, msgbuf, sizeof(fmtbuf), log_on_stderr ? VIS_SAFE|VIS_OCTAL : VIS_CSTYLE|VIS_NL|VIS_TAB|VIS_OCTAL);
Yes. Similar fix committed - thanks!