scryptenc: validate the header parameters used by "scrypt info" - #426
woahwhattheheck wants to merge 4 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.
Graph-only rejoin of the isolated Tarsnap#426 integration candidate with current master after a path-disjoint main.c advance. #28 remains the master-target carrier.
Integrate Tarsnap#426 header validation and overflow-safe `scrypt info` estimates onto the current fork, with expanded reviewed regression coverage. Preserves all later fork fixes; no new upstream bounty, acceptance, or payment claim.
|
Maintainer review requested for #426 (fixes first-reporter report #425). Current head |
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().