Bug 2651 - ssh prints bogus error message if config file has very long lines
Summary: ssh prints bogus error message if config file has very long lines
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 7.4p1
Hardware: All All
: P5 minor
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_5
  Show dependency treegraph
 
Reported: 2016-12-24 13:17 AEDT by don fong
Modified: 2018-04-06 12:26 AEST (History)
2 users (show)

See Also:


Attachments
an example config file to demonstrate the bug (3.04 KB, application/octet-stream)
2016-12-24 13:17 AEDT, don fong
no flags Details
proposed patch, untested (2.31 KB, patch)
2016-12-24 13:21 AEDT, don fong
no flags Details | Diff
slight improvement to patch (2.32 KB, patch)
2016-12-28 18:30 AEDT, don fong
no flags Details | Diff
regression test (2.88 KB, patch)
2017-01-01 18:51 AEDT, don fong
no flags Details | Diff
fatal if line is at limit (868 bytes, patch)
2017-03-10 14:33 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 don fong 2016-12-24 13:17:19 AEDT
Created attachment 2918 [details]
an example config file to demonstrate the bug

for example, if a config file contains a comment line that is 1023+ chars long, the characters at position 1023 and beyond are treated as a separate line - not ignored as they should be.

in this example, longline.config has a comment line that is longer than 1023 chars.

$ ssh -F longline.config whatever
longline.config: line 5: Bad configuration option: ABCDEFG
longline.config: terminating, 1 bad configuration options

readconf.c uses a buffer of size 1024.  one char is needed for the null terminator, another char is needed for the newline.  thus the effective limit is 1022 (excluding newline).

very similar code exists in libopenssh.  it probably needs the fix too.
Comment 1 don fong 2016-12-24 13:21:17 AEDT
Created attachment 2919 [details]
proposed patch, untested
Comment 2 don fong 2016-12-28 18:30:23 AEDT
Created attachment 2922 [details]
slight improvement to patch
Comment 3 don fong 2017-01-01 18:51:06 AEDT
Created attachment 2923 [details]
regression test

regression test for long config lines
Comment 4 Damien Miller 2017-03-10 14:33:55 AEDT
Created attachment 2958 [details]
fatal if line is at limit

Here's a simpler patch that makes ssh match sshd's behaviour: fatal if the line completely fills the buffer.

To make sure that this doesn't create problems for users who had configuration files that contained lines this long, this also cranks the line buffer size to match sshd's.
Comment 5 Damien Miller 2017-03-10 15:27:50 AEDT
Patch committed. This will be in OpenSSH 7.5
Comment 6 don fong 2017-03-10 16:53:36 AEDT
the patch(es) that i submitted have some advantages over this fix.

* this fix errors out when the line length is exactly 4095 including newline.  in this case, the line is not "too long" to fit in the buffer, so the error message is somewhat misleading.  my patch correctly handles the case when the line exactly fits in the buffer.

* when the error happens, my patch prints a more helpful error message, telling the user what the maximum line length is.

* my patch has a regression test.

* my patch also documents (in the man page) the fact that there is a limit on the line length.

* my patch uses a symbolic constant for the maximum line length.  this is a better practice than a hard-coded constant.  it is also needed to tie together the code, the regression test, and the documentation.
Comment 7 Damien Miller 2017-03-10 17:37:17 AEDT
I appreciate your point, but I don't believe those are compelling enough reasons to justify a significantly more complex solution.
Comment 8 don fong 2017-03-10 18:01:18 AEDT
it is not significantly more complicated.
Comment 9 Damien Miller 2018-04-06 12:26:40 AEST
Close all resolved bugs after release of OpenSSH 7.7.