config file line length limit

[Don Fong]

so, i submitted the bug, i added a patch for the code (and man page),
and another patch for a regression test.  is there anything else i can
do to help move this along?  will there be a code review process to
give me feedback on the code change?  how long does it typically take
for a simple change to get merged?

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


On 12/19/16 9:59, Don Fong wrote:
> Damien, thanks for your answer.  any comments on ancillary issues?
>
> * should the limit be documented, or removed?  or should the limit
> just be increased?
>
> * is there any mechanism to keep the code in libopenssh in sync with
> the corresponding code in openssh-portable?  i would think that as a
> philosophical matter, openssh-portable should be using the same code
> as libopenssh where possible.  particularly for things like readconf.c .
> (in fact, as a newcomer to this project, i am wondering why
> openssh-portable couldn't simply link in libopenssh as a library?
> why are they two separate github repos, instead of one repo that
> can build both the executable and the library?)
>
> * is there a missing assignment to active (*activep) in the
> openssh version of readconf.c, is the comment incorrect, or?
>
>
>
>
> On 12/18/16 19:52, Damien Miller wrote:
>> Hi,
>>
>> This does look like a bug, so it would be great if you could file it on
>> bugzilla.
>>
>> Thanks,
>> Damien
>>
>> On Sun, 18 Dec 2016, Don Fong wrote:
>>
>>> To all,
>>>
>>> i think i've found a minor bug in openssh.  i'm writing to the list
>>> toget input on whether it's really a bug, or an undocumented limit,
>>> or maybe it's even documented somewhere (although i didn't see
>>> it documented in ssh_config(5)).  if there is a consensus that this
>>> is indeed a bug, i'll file it in bugzilla.  i would also like to
>>> submit the fix.
>>>
>>> the bug is that ssh barfs if my .ssh/config file contains a very
>>> long comment line.  basically it tries to parse anything beyond
>>> the 1022-th char as regular input, not a comment.
>>>
>>> i discovered this behavior by accident.  i have an ansible script
>>> that spins up new AWS instances.  it creates an ssh config stanza
>>> for eachnew instance.  at the start of the stanza, the script puts
>>> a commentline containing amazon's JSON description of the instance.
>>> dueto recent changes made by amazon, the description is longer than
>>> it used to be, making the comment longer.  then ssh started failing.
>>>
>>> example (using the attached config file verylong.config):
>>>
>>> $ ssh -F verylong.config whatever
>>> verylong.config: line 9: Bad configuration option: ABCDEFG
>>> verylong.config: terminating, 1 bad configuration options
>>>
>>> i decided to take a look at the ssh source code.  i think it is
>>> pretty clear what is going wrong.
>>>
>>> ----- readconf.c:
>>> 1703         char line[1024];
>>> ...
>>> 1730         while (fgets(line, sizeof(line), f)) {
>>> 1731                 /* Update line number counter. */
>>> 1732                 linenum++;
>>> 1733                 if (process_config_line_depth(options, pw, host,
>>> original_host,
>>> 1734                     line, filename, linenum, activep, flags, depth)
>>> != 0)
>>> 1735                         bad_options++;
>>> 1736         }
>>>
>>> if fgets() runs across a very long input line, whatever won't fit in
>>> the given buffer (sizeof line) is left unread on the input stream,
>>> to be picked up by later reads.  this is the documented behavior of
>>> fgets().  how long is too long?  the buffer is sized at 1024 chars.
>>> one char is needed for the null terminator, another one for the
>>> newline.  so anything longer than 1024-2 = 1022 bytes is too big.
>>>
>>> unf readconf.c as written just naively assumes that fgets() returns the
>>> entire line.  it makes no attempt to deal with the case where the line
>>> didn't entirely fit.  so basically, a long line is treated as multiple
>>> lines.  this is true whether the line was a comment or something else.
>>> it's just that the behavior stands out more for a comment line, which
>>> "should" be completely ignored.  besides, there's not much reason for
>>> any other type of line to be that long, in the config file.
>>>
>>> i see basically the same problem in the libopenssh version of readconf.c.
>>>
>>> IMHO this is a bug.  some might consider it to be a reasonable limit
>>> on the line length, but in that case it should be documented in
>>> ssh_config(5).  and in either case, i think line[] should be declared
>>> using a symbolic constant for the length.
>>>
>>> or, get rid of the fixed-length buffer, and implement a dynamically
>>> sized buffer instead.  since i've only begun to look at this code,
>>> i'm not sure this would be a safe thing to do.  is there any other
>>> code that implicitly assumes that the line length is less than 1024?
>>>
>>> incidentally, i see a strange discrepancy between the openssh-portable
>>> version and the libopenssh version.  before the while-fgets loop,
>>> there is this comment (both versions):
>>>
>>> 1095         /*
>>> 1096          * Mark that we are now processing the options.  This flag
>>> is turned
>>> 1097          * on/off by Host specifications.
>>> 1098          */
>>> 1099         active = 1;
>>>
>>> but the "active=1" (line 1099) appears only in the libopenssh version,
>>> not the openssh-portable version.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>