config file line length limit

[Don Fong]

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.
>>
>>
>>
>>
>>
>>
>>
>>