Skip to content

fix off-by-one in ProtocolGet and ProtocolOpenDir recv buffers#6171

Closed
aizu-m wants to merge 1 commit into
cfengine:masterfrom
aizu-m:protocol-recv-buf-off-by-one
Closed

fix off-by-one in ProtocolGet and ProtocolOpenDir recv buffers#6171
aizu-m wants to merge 1 commit into
cfengine:masterfrom
aizu-m:protocol-recv-buf-off-by-one

Conversation

@aizu-m

@aizu-m aizu-m commented Jun 9, 2026

Copy link
Copy Markdown

Tracing the file-receive path in libcfnet/protocol.c.

TLSRecv() NUL-terminates what it reads:

tls_generic.c:  buffer[received] = '\0';   // received can equal toget

So the buffer must hold toget + 1 bytes. net.c does exactly that already
with char proto[CF_INBAND_OFFSET + 1] for a CF_INBAND_OFFSET read.

ProtocolGet() and ProtocolOpenDir() declare buf[CF_MSGSIZE] yet receive
up to CF_MSGSIZE bytes. GET goes straight through TLSRecv(ssl, buf, CF_MSGSIZE). OPENDIR goes through ReceiveTransaction(), whose length cap is
CF_BUFSIZE - CF_INBAND_OFFSET, i.e. CF_MSGSIZE. A peer that fills the
record makes received reach CF_MSGSIZE, so the terminating NUL is written
to buf[CF_MSGSIZE], one byte past the array. The byte count is server
controlled, so the reach crosses the trust boundary.

Confirmed by modelling the terminating write with a guard byte placed right
after the buffer: the guard is clobbered at CF_MSGSIZE, intact once the
buffer is CF_BUFSIZE.

ProtocolStat() in the same file already sizes its buffer CF_BUFSIZE. The
other two now match it.

@cf-bottom

Copy link
Copy Markdown

Thank you for submitting a pull request! Maybe @craigcomstock can review this?

nickanderson added a commit to nickanderson/core that referenced this pull request Jun 12, 2026
ProtocolOpenDir()/ProtocolGet() receive up to CF_MSGSIZE bytes into a
char buf[CF_MSGSIZE], but the receive primitives NUL-terminate at
buf[received] where received can equal CF_MSGSIZE -- one past the array
(TLSRecv/RecvSocketStream both write toget+1 bytes).

Drives the real ProtocolOpenDir() over a classic-protocol socketpair with
a record-filling reply. Aborts under AddressSanitizer on current code,
passes once the buffers are sized CF_BUFSIZE (cfengine#6171).

Ref: CFE-4687, cfengine#6171

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nickanderson

Copy link
Copy Markdown
Member

Thanks for the contribution. I have a PR with a test that exposes it and shows the test passes with your commit over in #6177

Closing this one in favor of that one

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