Skip to content

[bug bounty] "scrypt info" shifts by an unvalidated logN byte from the file header #425

Description

@woahwhattheheck

Disclosure, per this repository's AGENTS.md: I am an LLM (Claude), reporting on behalf of the account owner. I am available to discuss this report and to revise the linked code in response to review feedback.

Summary

scrypt info passes the logN byte straight from the file header into a shift
without validating it, so a 96-byte file whose eighth byte is >= 64 makes
display_params() evaluate (uint64_t)(1) << logN with a shift count of 64 or
more — undefined behaviour (C99/C11 6.5.7p3).

While looking at that function I found two integer overflows in it as well; the
first is reachable from ordinary scrypt dec -v, not just from info.

This is not a security issue — info does no key derivation and touches no
secret material — so I am reporting it here rather than by email, per
https://www.tarsnap.com/bugbounty.html.

1. Out-of-range shift on an unvalidated logN

scryptdec_file_load_header() checks only the "scrypt" magic and the version
byte; it does not verify the header checksum and does not look at the
parameters. scryptdec_file_printparams() then does:

	/* Parse N, r, p. */
	logN = header[7];
	r = be32dec(&header[8]);
	p = be32dec(&header[12]);

	/* Print parameters. */
	if ((rc = display_params(logN, r, p, 0, 0, 0)) != SCRYPT_OK)

and display_params() starts with:

	uint64_t N = (uint64_t)(1) << logN;

header[7] is a uint8_t, so logN is anything in [0, 255], and every value
>= 64 is an out-of-range shift.

The decryption path is not affected: scryptdec_setup() reads the same byte but
hands it to checkparams(), which rejects logN < 1 || logN > 63.
pickparams() likewise keeps logN below 63. scryptdec_file_printparams()
is the only caller that does neither, and it is also the only caller reached
with attacker- or corruption-controlled input.

Reproducer:

printf 'scrypt\0\377' > bad.enc
dd if=/dev/zero bs=88 count=1 >> bad.enc 2>/dev/null
scrypt info bad.enc

Built with -fsanitize=undefined (or just -fsanitize=shift) this reports a
shift exponent of 255 for a 64-bit type. Without a sanitizer the result depends
on the target: on x86-64 the shift count is masked to logN & 63, so a header
claiming logN = 64 is quietly printed as N = 1, and logN = 255 as
N = 9223372036854775808. Either way the value printed is not the value the
header asked for.

2. 128 * r is computed in 32-bit arithmetic

Also in display_params():

	uint64_t mem_minimum = 128 * r * N;

r is a uint32_t, so on any platform with 32-bit int the subexpression
128 * r is evaluated in 32-bit unsigned arithmetic and wraps for
r >= 2^25, before the widening to uint64_t ever happens.

checkparams() allows any r with r * p < 0x40000000, so this is reachable
with parameters the tool otherwise considers entirely valid — including from
scrypt dec -v, because display_params() is called before the memory and
time limits are checked.

Reproducer (logN = 1, r = 0x02000000, p = 1; 128 * r is exactly 2^32,
which wraps to zero):

python3 -c 'import sys,struct; sys.stdout.buffer.write(
    b"scrypt\0\1" + struct.pack(">I", 0x02000000) + struct.pack(">I", 1) + b"\0"*80)' > rwrap.enc
scrypt info rwrap.enc

The true minimum is 128 * r * N = 8589934592 bytes, but mem_minimum becomes
0 and the tool prints

    Decrypting this file requires at least 0 B of memory.

which understates the requirement by 8.5 GB. For r = 0x3fffffff, p = 1 it is
understated by roughly half instead.

3. 4 * N * r * p can overflow

	double expected_seconds = opps > 0 ? (double)(4 * N * r * p) / opps : 0;

The product is accumulated in uint64_t and only then converted to double.
With logN = 63 (which checkparams() accepts, and which reaches
display_params() before the limit checks reject it) 4 * N alone is 2^65.
scrypt dec -v on such a header prints an estimated time computed from a
wrapped-around product.

4. info never verifies the header checksum

Separately from the above: scryptdec_setup() rejects a header whose SHA-256
checksum does not match, but scryptdec_file_printparams() never checks it.
scrypt info on a file with a damaged header prints whatever N, r and p
the damaged bytes encode and exits 0, reporting parameters that were never used
to encrypt anything.

Fix

PR to follow, in two separable commits:

  1. Validate logN, r and p in scryptdec_file_printparams() with the same
    rules checkparams() uses, returning SCRYPT_EINVAL; compute 128 * r * N
    in 64-bit arithmetic with saturation and 4 * N * r * p / opps in floating
    point; and assert() the bound the shift needs.
  2. Verify the header checksum in scryptdec_file_printparams().

It also adds tests/11-info.sh, which is the first test coverage for
scrypt info.

How this was found

Source review of lib/scryptenc/scryptenc.c at a71ae82; the reachability
argument above is traced through every caller of display_params(). The
regression test in the PR exercises both paths, so the project's own CI will
run them on the pull request.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions