Skip to content

Validate reference scrypt parameters#405

Closed
achimala wants to merge 1 commit into
Tarsnap:masterfrom
achimala:codex/ref-param-validation
Closed

Validate reference scrypt parameters#405
achimala wants to merge 1 commit into
Tarsnap:masterfrom
achimala:codex/ref-param-validation

Conversation

@achimala

Copy link
Copy Markdown

The reference crypto_scrypt() implementation was less strict than the primary implementation for invalid KDF parameters:

  • r == 0 and p == 0 reached later allocation/math checks and failed with ENOMEM in my local harness rather than EINVAL.
  • N == 1 was accepted even though the public API documents N as a power of two greater than 1 and the primary implementation rejects N < 2.

This mirrors the existing guards in lib-platform/crypto/crypto_scrypt.c, updates the reference comments/docs to state 0 < r * p < 2^30, and adds a small regression binary for the reference implementation.

Verification on macOS/Apple Silicon from a no-space build path (/tmp/scrypt-verify):

PATH="/opt/homebrew/opt/libtool/libexec/gnubin:$PATH" autoreconf -i
CPPFLAGS="-I/opt/homebrew/opt/openssl@3/include" \
  LDFLAGS="-L/opt/homebrew/opt/openssl@3/lib" \
  ./configure --enable-libscrypt-kdf
make -j4
make test SMALLMEM=1

Result: the new tests/verify-strings/test_scrypt_ref_params binary passed, and the existing test scenarios passed; 05-system-scrypt-encrypt-decrypt skipped because no suitable system scrypt was installed.

I used Codex assistance to find, patch, and test this change.

@gperciva

Copy link
Copy Markdown
Member

Could you please split this up into 3 separate PRs:

  • fix r or p being zero; including changes to FORMAT and README.md. The commit message should mention 686a628 as well.
  • fix N == 1
  • add a test case. This should go into its own directory, not put into verify-strings since it has nothing to do with verifying strings.

@achimala

Copy link
Copy Markdown
Author

Thanks, split as requested:\n\n- #406 for the zero r/p fix and docs, including the 686a628 reference in the commit message\n- #407 for the N == 1 fix\n- #408 as a draft test-only PR under tests/invalid-params/, outside verify-strings\n\nClosing this combined PR as superseded by those smaller PRs.

@achimala achimala closed this May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants