Bug 111

Summary: sshd syslogs raw untrusted data
Product: Portable OpenSSH Reporter: Pavel Kankovsky <peak>
Component: sshdAssignee: OpenSSH Bugzilla mailing list <openssh-bugs>
Status: CLOSED FIXED    
Severity: normal    
Priority: P2    
Version: -current   
Hardware: UltraSPARC   
OS: Solaris   
Attachments:
Description Flags
Process all syslog data through vis() none

Description Pavel Kankovsky 2002-02-13 10:20:29 AEDT 
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.
Comment 1 Markus Friedl 2002-02-13 10:30:06 AEDT 
that's strange, either syslog() or syslogd should do vis()
Comment 2 Pavel Kankovsky 2002-02-13 10:58:30 AEDT 
The OS was S-o-l-a-r-i-s. Not OpenBSD. :)
Comment 3 Kevin Steves 2002-03-31 05:13:27 AEST 
we should perhaps vis(3) wrap log calls.
Comment 4 Damien Miller 2002-04-17 12:03:29 AEST 
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()
Comment 5 Damien Miller 2002-04-17 13:04:18 AEST 
Created attachment 79 [details]
Process all syslog data through vis()
Comment 6 Tom Holroyd 2002-04-17 13:42:50 AEST 
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.
Comment 7 Damien Miller 2003-01-07 17:05:46 AEDT 
Patch applied. I don't think there should be header file clashes with graphviz,
but please file separate bug reports if there are.
Comment 8 Damien Miller 2004-04-14 12:24:18 AEST 
Mass change of RESOLVED bugs to CLOSED
Comment 9 Pavel Kankovsky 2004-07-21 09:59:42 AEST 
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);
Comment 10 Damien Miller 2004-07-21 10:53:18 AEST 
Yes. Similar fix committed - thanks!