Bug 2688 - Long log messages to stderr missing newlines
Summary: Long log messages to stderr missing newlines
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 7.4p1
Hardware: All Linux
: P5 minor
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_5
  Show dependency treegraph
 
Reported: 2017-03-05 07:52 AEDT by brian.dyson
Modified: 2018-04-06 12:26 AEST (History)
3 users (show)

See Also:


Attachments
include room for \r\n in sprintf (543 bytes, patch)
2017-03-10 14:05 AEDT, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description brian.dyson 2017-03-05 07:52:14 AEDT
When logging to standard error (via -e) or to log file (via -E <logfile>), long log messages do not end with a newline.

Problem occurs in log.c, line 456 in OpenSSH 7.4p1.

The snprintf() call attempts to copy fmtbuf and "\r\n" into msgbuf. However, fmtbuf and msgbuf are the same size (MSGBUFSIZ, nominally 1024 bytes). When fmtbuf is completely filled (due to long log message), then the snprintf() simply copies fmtbuf and ignores the "\r\n".

This was observed when testing certificate-based logins at LogLevel DEBUG3.

For example, 3 logs messages appear on one line like this (with ... replacing long OpenSSH certificate public key):

debug2: user_key_allowed: check options: 'ssh-rsa-cert-v01@openssh.com AAAA...debug2: user_key_allowed: advance: 'AAAA...debug2: key not found

Notice multiple debug2 messages all on the same line. Each log line should with with a newline character.

Suggested Fix

Since the intent is to append "\r\n" to fmtbuf before writing to stderr, maybe it would be better to make that intent clearer using strlcat() rather than snprintf().

msgbuf[0] = '\0';
(void)strlcat(msgbuf, fmtbuf, sizeof msgbuf  - 2);
(void)strlcat(msgbuf, "\r\n", sizeof msgbuf );
Comment 1 Damien Miller 2017-03-10 14:05:54 AEDT
Created attachment 2957 [details]
include room for \r\n in sprintf

This is a slightly different fix, that sets an explicit length in the snprintf call for the log line.
Comment 2 Damien Miller 2017-03-10 14:16:35 AEDT
Patch applied. This will be in openssh-7.5, due soon.
Comment 3 brian.dyson 2017-03-13 04:29:46 AEDT
Ah, yes. Much more elegant fix than my proposal. Thanks!
Comment 4 Damien Miller 2018-04-06 12:26:36 AEST
Close all resolved bugs after release of OpenSSH 7.7.