scryptenc: validate the header parameters used by "scrypt info" - #426
Open
woahwhattheheck wants to merge 3 commits into
Open
scryptenc: validate the header parameters used by "scrypt info"#426woahwhattheheck wants to merge 3 commits into
woahwhattheheck wants to merge 3 commits into
Conversation
scryptdec_file_printparams() passed header[7] straight to display_params(), which computes N as (uint64_t)(1) << logN. Because scryptdec_file_load_header() only checks the magic and the version byte, logN can be anything from 0 to 255, and any value >= 64 makes that an out-of-range shift (C99 6.5.7p3). A 96-byte file beginning "scrypt\0" followed by a 0xff byte is enough to reach it via "scrypt info". Validate logN, r and p with the same rules checkparams() uses before handing them to display_params(), and return SCRYPT_EINVAL otherwise. Also fix two overflows in display_params() itself: "128 * r" has type uint32_t and wraps for r >= 2^25 (so a header with r = 0x02000000 was reported as needing "0 B" of memory), and 4 * N * r * p can exceed UINT64_MAX. Compute the first in 64-bit arithmetic with saturation and the second in floating point, and assert the logN bound the shift needs. Add tests/11-info.sh, which covers "scrypt info" for the first time.
scryptdec_setup() rejects a header whose SHA-256 checksum does not match, but scryptdec_file_printparams() never checked it. "scrypt info" on a file with a corrupt header therefore printed whatever N, r and p the damaged bytes happened to encode and exited 0, reporting parameters that were never used to encrypt anything. Perform the same check that scryptdec_setup() does, so that "info" reports "Input is not valid scrypt-encrypted block" instead.
The previous commit was uploaded through the GitHub API by a helper which dropped the final newline of every text file it sent. Rewrite those files with their trailing newline intact; no other change.
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.
Fixes #425.
scryptdec_file_printparams()— thescrypt infopath — readslogNfromheader[7]and passes it todisplay_params(), which computes(uint64_t)(1) << logN.scryptdec_file_load_header()validates only themagic and the version byte, so
logNcan be any value in[0, 255]andanything
>= 64is an out-of-range shift. Every other caller ofdisplay_params()boundslogNfirst (checkparams()rejectslogN < 1 || logN > 63;pickparams()never exceeds 63), soinfois theonly way in — and the only one reached with untrusted input.
Commit 1 — reject out-of-range parameters in
scrypt infologN,randpinscryptdec_file_printparams()using the samerules
checkparams()uses, returningSCRYPT_EINVAL("Input is not validscrypt-encrypted block") otherwise.
assert()thelogNbound indisplay_params(), so the precondition theshift relies on is stated where the shift is.
128 * r * Nin 64-bit arithmetic.128 * ris auint32_texpression and wraps for
r >= 2^25, whichcheckparams()permits — aheader with
r = 0x02000000is currently reported as needing "0 B" ofmemory rather than 8.5 GB, and that one is reachable from
scrypt dec -vas well, since
display_params()runs before the limit checks. The productcan still exceed
UINT64_MAX, so it saturates rather than wrapping.4 * N * r * p / oppsin floating point; accumulated inuint64_tit overflows for
logN = 63.tests/11-info.sh—scrypt infohad no test coverage at all — with a96-byte fixture that has a valid checksum and an out-of-range
logN.Commit 2 — verify the header checksum in
scrypt infoscryptdec_setup()rejects a header whose SHA-256 checksum does not match;scryptdec_file_printparams()never checked it, soscrypt infoon a filewith a damaged header printed whatever
N,randpthe damaged bytesencoded and exited 0. This performs the same check.
This commit is separable from the first — the first fixes the undefined
behaviour on its own — so it can be dropped if you would rather
infostaypurely descriptive. The test fixture's checksum is valid either way, so the
logNtest still exercises the parameter check with this commit applied.Notes
scrypt infoontests/verify-strings/test_scrypt_good.encstill printsN = 262144; r = 8; p = 1;and exits 0.SCRYPT_EINVALon a header that was already invalid.assert()matches the existing one inscryptenc_setup().