openssh bugs - Dec 2016 - [Bug 2651] New: ssh prints bogus error message if config file has very long lines

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

            Bug ID: 2651
           Summary: ssh prints bogus error message if config file has very
                    long lines
           Product: Portable OpenSSH
           Version: 7.4p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: minor
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: dfong at dfong.com

Created attachment 2918
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2918&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

--- Comment #1 from don fong <dfong at dfong.com> ---
Created attachment 2919
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2919&action=edit
proposed patch, untested

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

don fong <dfong at dfong.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2919|0                           |1
        is obsolete|                            |

--- Comment #2 from don fong <dfong at dfong.com> ---
Created attachment 2922
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2922&action=edit
slight improvement to patch

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

--- Comment #3 from don fong <dfong at dfong.com> ---
Created attachment 2923
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2923&action=edit
regression test

regression test for long config lines

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at zip.com.au
   Attachment #2958|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Created attachment 2958
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2958&action=edit
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2647


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2958|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Patch committed. This will be in OpenSSH 7.5

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

--- Comment #6 from don fong <dfong at dfong.com> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
I appreciate your point, but I don't believe those are compelling
enough reasons to justify a significantly more complex solution.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

--- Comment #8 from don fong <dfong at dfong.com> ---
it is not significantly more complicated.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2651

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.