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. >>> >>> >>> >>> >>> >>> >>> >>> > _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
