From e61649242afc42213e7fd3bb8b3dbea33be96761 Mon Sep 17 00:00:00 2001 From: Alex Naumov Date: Wed, 7 May 2025 10:49:24 +0200 Subject: [PATCH 3/6] attacher.c: fix bad strncpy() which can lead to a buffer overflow `strncpy()` always pads the destination buffer with zeroes, regardless of the length of the input string. Passing `MAXPATHLEN` in every `for` loop iteration will cause a buffer write overflow past the end of the `m.m.command.cmd` buffer. This becomes visible on systems that compile Screen with the `_FORTIFY_SOURCE` macro enabled when passing more than one parameter, for example like this: ``` screen -S myinstance -X blankerprg /path/to/blanker *** buffer overflow detected ***: terminated Aborted (core dumped) ``` This is not security relevant, since only zeroes are written past the end of the buffer and only other message buffer fields can be reached, no internal state of Screen can be changed. Committed-By: Matthias Gerstner --- attacher.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/attacher.c b/attacher.c index d8de9d4..4e1a77e 100644 --- a/attacher.c +++ b/attacher.c @@ -457,13 +457,16 @@ void SendCmdMessage(char *sty, char *match, char **av, int query) } p = m.m.command.cmd; n = 0; + size_t space_left = ARRAY_SIZE(m.m.command.cmd); + for (; *av && n < MAXARGS - 1; ++av, ++n) { - size_t len; - len = strlen(*av) + 1; - if (p + len >= m.m.command.cmd + ARRAY_SIZE(m.m.command.cmd) - 1) - break; - strncpy(p, *av, MAXPATHLEN); - p += len; + int printed = snprintf(p, space_left, "%s", *av); + if (printed < 0 || (size_t)printed >= space_left) + Panic(0, "Total length of the command to send too large.\n"); + + printed += 1; // add null terminator + p += printed; + space_left -= printed; } *p = 0; m.m.command.nargs = n; -- 2.49.0