such: stop writing through a string literal in the -e path - #411
Open
xanimo wants to merge 1 commit into
Open
Conversation
`such -c generate_mnemonic -e <hex>` segfaults. Every time, on a
documented flag.
char* entropy_size = "256";
...
sprintf(entropy_size, "%zu", strlen(entropy) / HEX_CHARS_PER_BYTE * 8);
entropy_size points at a string literal, and string literals live in
read-only memory, so the sprintf faults before it writes anything.
Reproduced directly:
$ such -c generate_mnemonic -e 000102...1f
Segmentation fault (exit 139)
and with a four-line C program doing the same thing, which also takes
SIGSEGV, confirming the mechanism rather than inferring it.
There is a second defect underneath the first. "256" is four bytes
including its terminator, while the value derived here is
strlen(optarg) / 2 * 8 -- unbounded, since optarg is a command-line
argument -- so even against a writable four-byte buffer this overflows
for any -e argument longer than 128 hex characters. The crash simply
arrives first.
entropy_size cannot become an array: the -z case assigns optarg to it, so
it has to stay a pointer. Adds a local buffer for the -e case to derive
into, repoints entropy_size at it, and uses snprintf. 21 bytes holds any
size_t in decimal plus a terminator.
Verified across the paths that share this variable:
-e <32 hex chars> 12 words (was: segfault)
-e <64 hex chars> 24 words (was: segfault)
-z 128 12 words
no flags 24 words
The 128-bit and 256-bit runs agree on their first eleven words, which is
what should happen when the shorter entropy is a prefix of the longer --
evidence the value is being used, not merely that the crash stopped.
Not present in v0.1.2, v0.1.3 or v0.1.4. Present in v0.1.5-pre, which is
the current release on GitHub.
81/81.
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.
such -c generate_mnemonic -e <hex>segfaults. Every time, on a documented flag.entropy_sizepoints at a string literal. Literals live in read-only memory, so thesprintffaults before it writes anything.Reproduced directly rather than inferred:
and with a four-line C program doing the same thing, which also takes SIGSEGV — confirming the mechanism, not just the symptom.
There's a second defect underneath the first
"256"is four bytes including its terminator. The value derived here isstrlen(optarg) / 2 * 8— andoptargis a command-line argument, so it's unbounded. Even against a writable four-byte buffer this overflows for any-eargument longer than 128 hex characters:-elengthThe crash just arrives first, which is why the overflow was never observed.
The fix
entropy_sizecan't become an array — the-zcase assignsoptargto it, so it has to stay a pointer. So this adds a local buffer for the-ecase to derive into, repointsentropy_sizeat it, and usessnprintf. 21 bytes holds anysize_tin decimal plus a terminator.Verification
-e <32 hex>-e <64 hex>-z 128The 128-bit and 256-bit runs agree on their first eleven words, which is what should happen when the shorter entropy is a prefix of the longer — evidence the value is actually being used, not merely that the crash stopped.
Affected versions
Not in v0.1.2, v0.1.3 or v0.1.4. Present in v0.1.5-pre, which is the current release on GitHub.
81/81. Found via CodeQL's
cpp/overrunning-write, which flagged the buffer size — the read-only write turned out to be the more immediate problem.