Repository navigation
fix leak of uninitialized stack memory in CfGetFile refusal replies - #6408
Merged
larsewi merged 1 commit intoOct 15, 2026
Merged
Conversation
CfGetFile() in cf-serverd/server_common.c has a local reply buffer:
char sendbuffer[CF_BUFSIZE + 256], filename[CF_BUFSIZE - 128];
On its two early-return paths, access refused by TransferRights() and
open() failure, it writes only the short CF_FAILEDSTR status string with
snprintf() and then sends args->buf_size bytes of the buffer back to the
client:
snprintf(sendbuffer, CF_BUFSIZE, "%s", CF_FAILEDSTR);
...
SendSocketStream(ConnectionInfoSocket(conn_info), sendbuffer, args->buf_size);
... (or TLSSend() on the TLS protocol)
args->buf_size is the block size from the client's "GET <buf_size> <file>"
request, validated only to be within 1..CF_BUFSIZE. CF_FAILEDSTR is 59
bytes, so a client that asks for a file it is not granted, or one that
cannot be opened, with buf_size near CF_BUFSIZE makes the server, running
as root, transmit up to about 4 KB of uninitialized stack per request. The
bytes past the status string are whatever the frame held from earlier
request handling.
The successful-transfer loop already memsets the buffer before each read,
and CfEncryptGetFile() reports refusals with FailedTransfer() rather than
sending from the buffer, so only these GET refusal/error replies leak.
Zero-initialize sendbuffer at declaration. Valid transfers are unchanged;
the refusal and error replies now pad with zeros instead of stack contents.
Changelog: Title
Signed-off-by: Dhanraj E <dhanraj@labs.digiscrypt.com>
Contributor
|
@cf-bottom Jenkins please :) |
|
Alright, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14803/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14803/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CfGetFile() in cf-serverd/server_common.c has a local reply buffer:
On its two early-return paths, access refused by TransferRights() and open() failure, it writes only the short CF_FAILEDSTR status string with snprintf() and then sends args->buf_size bytes of the buffer back to the client:
args->buf_size is the block size from the client's "GET <buf_size> " request, validated only to be within 1..CF_BUFSIZE. CF_FAILEDSTR is 59 bytes, so a client that asks for a file it is not granted, or one that cannot be opened, with buf_size near CF_BUFSIZE makes the server, running as root, transmit up to about 4 KB of uninitialized stack per request. The bytes past the status string are whatever the frame held from earlier request handling.
The successful-transfer loop already memsets the buffer before each read, and CfEncryptGetFile() reports refusals with FailedTransfer() rather than sending from the buffer, so only these GET refusal/error replies leak.
Zero-initialize sendbuffer at declaration. Valid transfers are unchanged; the refusal and error replies now pad with zeros instead of stack contents.