From 8247d900182be6a3a5796142a33e19c64efb0099 Mon Sep 17 00:00:00 2001 From: Dhanraj E Date: Tue, 20 Oct 2026 18:12:27 +0530 Subject: [PATCH] fix stack overflow building absolute path from relative symlink target cf-agent reconstructs the absolute target of a relative symbolic link by appending the readlink() result onto the link's directory name in a CF_BUFSIZE stack buffer. KillGhostLink() in files_links.c did this with an unbounded strcat(linkpath, linkbuf), and LinkCopy() in verify_files_utils.c with strncat(vbuff, linkbuf, CF_BUFSIZE - 1), whose third argument is the maximum number of bytes to take from the source, not the space left in the destination. readlink() returns up to CF_BUFSIZE - 1 bytes, so a symlink whose relative target is near CF_BUFSIZE, left by an unprivileged user in a directory that a root files promise walks (depth_search, perms, link copy), overflows the stack of the agent. KillGhostLink() runs on every symlink met during a recursive descent, before the dead-link stat(). Bound both appends with strlcat(..., sizeof(buf)), the same way ExpandLinks() in files_links.c already does, and have KillGhostLink() bail out with an "Internal limit reached" message when the target does not fit. Normal links are unchanged. Changelog: Title Signed-off-by: Dhanraj E --- cf-agent/verify_files_utils.c | 4 ++-- libpromises/files_links.c | 8 +++++++- tests/unit/files_links_test.c | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/cf-agent/verify_files_utils.c b/cf-agent/verify_files_utils.c index 7931ae87544..f7685ff1954 100644 --- a/cf-agent/verify_files_utils.c +++ b/cf-agent/verify_files_utils.c @@ -1338,7 +1338,7 @@ static PromiseResult LinkCopy(EvalContext *ctx, char *sourcefile, char *destfile strlcpy(vbuff, changes_sourcefile, CF_BUFSIZE); ChopLastNode(vbuff); AddSlash(vbuff); - strncat(vbuff, chrooted_linkbuf, CF_BUFSIZE - 1); + strlcat(vbuff, chrooted_linkbuf, sizeof(vbuff)); strlcpy(chrooted_linkbuf, vbuff, CF_BUFSIZE - 1); } else @@ -1346,7 +1346,7 @@ static PromiseResult LinkCopy(EvalContext *ctx, char *sourcefile, char *destfile strlcpy(vbuff, sourcefile, CF_BUFSIZE); ChopLastNode(vbuff); AddSlash(vbuff); - strncat(vbuff, linkbuf, CF_BUFSIZE - 1); + strlcat(vbuff, linkbuf, sizeof(vbuff)); strlcpy(linkbuf, vbuff, CF_BUFSIZE - 1); } } diff --git a/libpromises/files_links.c b/libpromises/files_links.c index 62fd9b58a2a..33125e44c69 100644 --- a/libpromises/files_links.c +++ b/libpromises/files_links.c @@ -528,7 +528,13 @@ bool KillGhostLink(EvalContext *ctx, const char *name, const Attributes *attr, c } } - strcat(linkpath, linkbuf); + if (strlcat(linkpath, linkbuf, sizeof(linkpath)) >= sizeof(linkpath)) + { + Log(LOG_LEVEL_ERR, + "Internal limit reached in KillGhostLink()," + " path too long: '%s' + '%s'", linkpath, linkbuf); + return false; + } CompressPath(tmp, sizeof(tmp), linkpath); if (stat(tmp, &statbuf) == -1) /* link points nowhere */ diff --git a/tests/unit/files_links_test.c b/tests/unit/files_links_test.c index 6b2fae5739d..2a4a2cf0932 100644 --- a/tests/unit/files_links_test.c +++ b/tests/unit/files_links_test.c @@ -25,7 +25,9 @@ #include #include -#include /* ExpandLinks */ +#include /* ExpandLinks, KillGhostLink */ +#include /* Attributes */ +#include /* symlink, unlink */ /* A single path component longer than the internal node buffer * (CF_MAXLINKSIZE) overflowed the stack in ExpandLinks(): the parse into @@ -44,12 +46,41 @@ static void test_expand_links_long_component(void) assert_false(ret); } +/* KillGhostLink() rebuilds the absolute target of a relative symlink by + * appending the readlink() result onto the link's directory in a CF_BUFSIZE + * buffer. The append was an unbounded strcat(), so a symlink whose relative + * target is near CF_BUFSIZE overflowed the stack. It must bail out instead. */ +static void test_kill_ghost_link_long_target(void) +{ + char tmpl[] = "/tmp/cfghostXXXXXX"; + const char *dir = mkdtemp(tmpl); + assert_true(dir != NULL); + + char linkpath[256]; + xsnprintf(linkpath, sizeof(linkpath), "%s/l", dir); + + char target[CF_BUFSIZE]; + memset(target, 'x', sizeof(target) - 1); + target[sizeof(target) - 1] = '\0'; + assert_int_equal(symlink(target, linkpath), 0); + + Attributes attr = { 0 }; + attr.link.when_no_file = cfa_delete; + PromiseResult result = PROMISE_RESULT_NOOP; + + assert_false(KillGhostLink(NULL, linkpath, &attr, NULL, &result)); + + unlink(linkpath); + rmdir(dir); +} + int main() { PRINT_TEST_BANNER(); const UnitTest tests[] = { unit_test(test_expand_links_long_component), + unit_test(test_kill_ghost_link_long_target), }; return run_tests(tests);