Skip to content

fix stack buffer overflow in SaveAsFile backup path construction - #6420

Open
Dhanu-dynamic wants to merge 1 commit into
cfengine:masterfrom
Dhanu-dynamic:saveasfile-backup-path-overflow
Open

Dhanu-dynamic wants to merge 1 commit into
cfengine:masterfrom
Dhanu-dynamic:saveasfile-backup-path-overflow

Conversation

@Dhanu-dynamic

Copy link
Copy Markdown
Contributor

Repro: run an edit_line (or edit_xml) files promise whose promiser is a symlink pointing to a target path near CF_BUFSIZE long; cf-agent crashes (ASan: stack-buffer-overflow in SaveAsFile).

Cause: after SaveAsFile() dereferences the symlink chain into deref_file (a Buffer, so up to CF_BUFSIZE), it builds the .cf-before-edit backup name and .cf-after-edit temp name with strcpy()/strcat() into the fixed backup[CF_BUFSIZE] and new[CF_BUFSIZE] stack buffers, plus a timestamp. A dereferenced path close to CF_BUFSIZE overruns both buffers. Reached from any files promise with edit_line/edit_xml via FinishEditContext -> SaveItemListAsFile -> SaveAsFile.

Fix: use strlcpy/strlcat and refuse the edit (as the function already does on other failures) when the backup/temp name would not fit, matching the bounded copies already used a few lines above in this file.

Verified with AddressSanitizer: a unit test in tests/unit/files_operators_test.c (driving SaveItemListAsFile over a long symlink target) and a real cf-agent edit_line run both trip the overflow before the change and are clean after. Full unit suite stays green under -fsanitize=address.

@larsewi larsewi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the test. The fix is easy enough to verify by reading it. And the test tests a side effect, not the behavior. Otherwise LGTM, thanks 🚀

Signed-off-by: Dhanraj E <dhanraj@labs.digiscrypt.com>
@Dhanu-dynamic
Dhanu-dynamic force-pushed the saveasfile-backup-path-overflow branch from 714cda2 to e1d058e Compare October 20, 2026 08:21
@Dhanu-dynamic

Copy link
Copy Markdown
Contributor Author

Dropped the unit test and the Makefile.am hunk, so it's just the strlcpy/strlcat change in SaveAsFile now.

@larsewi

larsewi commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

@cf-bottom Jenkins please :)

@cf-bottom

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants