rpctest: filter tar members on extraction; document two API contracts - #409
Open
xanimo wants to merge 1 commit into
Open
rpctest: filter tar members on extraction; document two API contracts#409xanimo wants to merge 1 commit into
xanimo wants to merge 1 commit into
Conversation
rpctest/fetch.py downloaded a dogecoind release and ran
tar.extractall(os.getcwd())
with no member filtering, so a crafted archive can write outside the
destination through '../' members, absolute paths, or symlinks pointing
out of the tree. Demonstrated rather than assumed: an archive whose only
member is '../escaped.txt' places that file one level above the
destination, contents intact. With filter='data' the same archive raises
OutsideDestinationError and nothing is written.
The checksum check above the extraction narrows who can supply such an
archive, but it does not make extraction safe on its own, and the two are
independent concerns. Note the .zip branch was never exposed: Python's
zipfile.extractall already sanitises member paths.
Uses filter='data' where available, and where it is not -- PEP 706 landed
in 3.12 and was backported to security releases of 3.8+ -- checks members
by hand rather than silently extracting unfiltered. Both the member path
and any link target must resolve inside the destination.
Also documents two contracts that were only discoverable by reading the
implementation:
dogecoin_generate_mnemonic's entropy_out takes a hex string and needs
MAX_ENTROPY_STRING_SIZE bytes; the type is really HEX_ENTROPY, but the
signature says char*, so the requirement is invisible at the call site.
The write itself is bounded -- entropy_size is validated to 128..256 bits
before use, capping output at 64 hex characters plus a terminator -- so
this is a documentation gap rather than an overflow, which is worth
stating plainly since static analysis flags the strcpy.
dogecoin_cheap_random_bytes is exported under a name that invites misuse.
It is seeded from the wall clock and is not cryptographic. In-tree it is
used only for P2P nonces, which is appropriate; the comment says so and
points at dogecoin_random_bytes for anything else.
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
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.
rpctest/fetch.pyextracts a downloaded archive unfilteredNo member filtering, so a crafted archive writes outside the destination via
../members, absolute paths, or symlinks pointing out of the tree.Demonstrated, not assumed. An archive whose only member is
../escaped.txt:With
filter='data', the same archive:The checksum check above the extraction narrows who can supply such an archive, but it doesn't make extraction safe — those are independent concerns, and the fix costs nothing.
Note the
.zipbranch was never exposed: Python'szipfile.extractallalready sanitises member paths. Only tar needed this.Uses
filter='data'where available. Where it isn't — PEP 706 landed in 3.12 and was backported to security releases of 3.8+ — it checks members by hand rather than silently extracting unfiltered; both the member path and any link target must resolve inside the destination.Two contracts that were only discoverable by reading the implementation
dogecoin_generate_mnemonic'sentropy_outneedsMAX_ENTROPY_STRING_SIZEbytes — the type is reallyHEX_ENTROPY, but the signature sayschar*, so the requirement is invisible at the call site.Worth being precise about severity here, because static analysis flags the
strcpyas an unbounded write and it isn't one:entropy_sizeis validated to 128–256 bits before use, which caps the output at 64 hex characters plus a terminator. It's a documentation gap, not an overflow. I'd rather say that plainly than let it get filed as a buffer overflow.dogecoin_cheap_random_bytesis exported under a name that invites misuse. It's seeded from the wall clock and is not cryptographic. In-tree use is appropriate — P2P nonces only — and the comment now says so, and points atdogecoin_random_bytesfor anything else.No functional change to the library; the only behaviour change is in a test helper script.
This will show red on cppcheck until #403 lands — that job fails on
0.1.5-devitself, not on this branch.