Hi Guillem, On 03.09.22 20:08, Guillem Jover wrote:
[ Removed Erik from To, as last time my mail was rejected by the mail server, and might then not get delivered by mailman as duplicate. ]
:-( I have sent this email to bug-inetutils@gnu.org only to avoid this kind of problem.
You might want to take a look at: <https://git.hadrons.org/cgit/debian/pkgs/inetutils.git/tree/debian/patches/0004-telnet-Add-checks-for-option-reply-parsing-limits.patch>
Thanks for pointing out that patch. Without it telnet crashes when it starts the log in process: $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. realloc(): invalid next size Aborted (core dumped) With the patch, it does not crash: $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. [...] login: Cannot possibly work without effective root Connection closed by foreign host. This crash does not happen with the "tests/addrpeek" test binary: $ ./telnet/telnet -l`perl -e 'print "A"x5000'` localhost 4711 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. Your address is 127.0.0.1. Connection closed by foreign host. Thus it is not trivial to add a test to GNU Inetutils. That is no reason for not fixing the bug, of course. @Simon: if you think it is OK to add this patch to GNU Inetutils, feel free to just go ahead and do so. I have to admit that I have a hard time understanding the details of the patch. Thus I'll try to analyze it: The patch changes only one file, "telnet/telnet.c". The OPT_REPLY_SIZE is changed from 256 to depend on SUBBUFSIZE. Currently that sets OPT_REPLY_SIZE to 2 * 256 = 512: -#define OPT_REPLY_SIZE 256 +#define OPT_REPLY_SIZE (2 * SUBBUFSIZE) It seems to me as if this is not really required to fix the crash, but rather to support larger option values. Is there a specific reason to use twice the SUBBUFSIZE for OPT_REPLY_SIZE? The global pointer *opt_reply is changed from implicit initialization to 0 to explicit initialization to NULL: -unsigned char *opt_reply; +unsigned char *opt_reply = NULL; In line 1719, opt reply is compared with NULL, thus it seems correct to initialize it to NULL. But there seems to be quite a bit of code that assumes NULL to have the value 0 (or at least to be "false"). Thus I wonder if similar changes would be advisable all over the code…. Two bounds checks are introduced to env_opt_add(): + if (opt_replyp + (2 + 2) > opt_replyend) + return; The code guarded by this check adds two bytes to the contents of opt_reply[OPT_REPLY_SIZE]. That could be one of the "2" values. + if (opt_replyp + (1 + 2 + 2) > opt_replyend) + return; The code guarded by this check adds one byte to the contents of opt_reply[OPT_REPLY_SIZE]. That could be the "1" value. A bounds check is introduced to env_opt_end(): + if (opt_replyp + 2 > opt_replyend) + return; The code guarded by this check adds two bytes to the contents of opt_reply[OPT_REPLY_SIZE]. That fits with the "2" value. This might be the other "2" value from the two checks above in env_opt_add(). I do not understand the second "2" value in the second bounds check in env_opt_add(), though. What did I miss? Of course, I may have made mistakes reading the code. Then there is the nagging issue that I did not see how these changes prevent the 5000 A bytes from overflowing the now 512 byte buffer. Could it be that there are other bounds checks that should be adjusted as well to account for this overhead of up to five bytes? In addition to, not as a replacement of, the checks from the patch. Anyway, the patch seems OK to me after looking more closely at the details.
The implementations in the BSDs are probably worth checking for similar old fixes.
I have to concur… any volunteers? ;-) Thanks, Erik