lightningd: log entry allocation and getlog output (v26.06.7 security, 5/7) - #9511
Merged
Merged
Conversation
nGoline
force-pushed
the
port-26.06.7-logging
branch
from
September 15, 2026 12:52
3d63a24 to
aa46443
Compare
The io level of the log book holds raw JSON-RPC and plugin traffic, which can contain secrets such as runes. Marked xfail until the next commit. (cherry picked from commit 03be4d7)
The io level of the log book holds the raw JSON-RPC and plugin traffic, which can contain secrets such as runes, and getlog returns the whole log book to its caller. Refuse level=io there: io logging remains available in the log file, via --log-level=io. Changelog-Changed: JSON-RPC: `getlog` no longer accepts `level=io`; io logs are only available in the log file (`--log-level=io`). (cherry picked from commit e05c572)
test_commando needs a reply over 65535 bytes to exercise commando's multi-message split, and used getlog level=io to get one. getlog no longer serves that level, so read back a 100k datastore entry instead. (cherry picked from commit 9373b5f)
log_to_files() sized a stack VLA by the log string's length. Callers can log attacker-controlled strings of arbitrary size - e.g. clnrest logging unauthenticated request parameters on rune failure - so a single ~10MB HTTP request overflowed the stack and crashed lightningd (SIGSEGV). Use a fixed 1kB stack buffer for normal entries and a heap allocation for oversized ones. Changelog-Fixed: lightningd: logging a very large message no longer crashes the node (stack overflow). Reported-by: Vincenzo Palazzo (Bitcoin Security Council finding 2026-08-10) (cherry picked from commit cc90f6b)
Drive log_to_files() directly with an entry far larger than any stack buffer, using a plugin which emits one on demand. Nothing bounds a log entry, so testing this through clnrest is the wrong shape: requests are now capped at 2MiB before any parameter is parsed, so an oversized request is rejected long before it can reach the logger, and a test written that way passes whether or not the daemon is fixed. A plugin can hand us an entry of any size, which is what we actually want to exercise. (cherry picked from commit 50910c9)
nGoline
force-pushed
the
port-26.06.7-logging
branch
from
September 15, 2026 21:30
aa46443 to
bfe3a06
Compare
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.
Fifth of seven PRs forward-porting the v26.06.7 security point release onto master.
Log entry allocation, and what
getlogis allowed to return.5 commits:
tests: check getlog does not return io logslightningd: don't return io logs from getlogtests: don't use io logs for commando's oversized replylightningd: don't allocate huge log entries on the stacktests: huge log entries don't crash lightningdOne
Changelog-Fixed, logging a very large message no longer crashes the node, and oneChangelog-Changed,getlogno longer acceptslevel=io; io logs remain available in the log file via--log-level=io.For reviewers
This is 5 commits, not the 6 in the release.
lightningd: don't free() a tal pointer when a log entry is truncatedis not ported, because master already has the property it restores, by a different implementation. On the release linelogv()built its message withvasprintf()and calledfree()on the pointercap_header()had already replaced with tal memory. Master'slogv()usestal_vfmt()throughout and passescap_header(tmpctx, &l, take(logmsg)), whose signature isTAKESand which always returns a fresh tal allocation, and the function ends intal_free(logmsg)with nofree()anywhere. There is no malloc pointer to lose. Its regression test is ported and passes.contrib/pyln-grpc-proto/pyln/grpc/node_pb2.pyis regenerated rather than merged, since removingiofrom thegetlogrequest enum changes the serialised descriptor. The regeneration is folded into the commit that changescln-grpc/proto/node.proto, so no commit leaves the generated file inconsistent with its source.