Skip to content

fix: reject key files with invalid or excessive scrypt parameters - #592

Open
Huge wants to merge 1 commit into
rustic-rs:mainfrom
Huge:fix/keyfile-scrypt-limits
Open

Huge wants to merge 1 commit into
rustic-rs:mainfrom
Huge:fix/keyfile-scrypt-limits

Conversation

@Huge

@Huge Huge commented Oct 7, 2026

Copy link
Copy Markdown

KeyFile::kdf_key passes the scrypt parameters N, r and p from the key file to scrypt without checking them. Key files are read from the backend, and find_key_in_backend tries every key in keys/ before the password is verified. A corrupt or crafted key file therefore decides how much memory and time opening the repository takes:

key file on main
N = 0 panics: assertion failed: x > 0 in log_2 (keyfile.rs:288)
N = 2^31, r = 8, p = 1 memory allocation of 2199023255552 bytes failed, the process aborts
N = 2^15, r = 8, p = 2^20 runs for about 14 hours (about 0.05 s per p)

This PR checks the parameters in kdf_key before running scrypt. It returns an ErrorKind::Key error if N is zero, if the memory scrypt allocates (128 * r * (N + p) bytes) exceeds 1 GiB, or if the work (128 * r * N * p) exceeds 2^32.

The limits are far above what rustic and restic create. Timings are from a release build on an AMD Zen 4 machine:

  • rustic (N = 2^17, r = 8, p = 1): 128 MiB, work 2^27, about 0.26 s
  • restic (calibrated to 0.5 s and at most 60 MiB; the key1 fixture has N = 2^15, r = 8, p = 6): 32 MiB, work 2^27.6, about 0.29 s
  • the largest accepted parameters: 1 GiB takes about 2.1 s, and work 2^32 takes about 6 s

r = 0 and p = 0 are still rejected by scrypt::Params::new, as before.

Tests

tests/keys.rs gets test_keys_with_excessive_scrypt_params_fail with the three cases above. Each case starts from the key1 fixture and changes only N, r and p. On main the three cases panic, abort and time out. With this change they return the new error immediately. test_working_keys_passes still opens the existing restic and rustic keys.

cargo fmt --check is clean, and clippy reports nothing for the touched files. cargo test --workspace --all-targets --all-features passes, apart from the five snapshot tests in tests/integration (backup, chunker, rewrite). Those fail the same way on an unmodified main.

A key file rejected this way still stops find_key_in_backend with an error, as it already does for parameters that scrypt::Params::new rejects. Skipping such keys and trying the remaining ones would change behaviour, so I left it out.

Disclaimer: Found the issue by test-driving an LM-powered vulnerability scanner I work on heavily.

`KeyFile::kdf_key` passed `N`, `r` and `p` from the key file straight to
scrypt. Key files are read from the backend and every key is tried
before the password is verified, so a corrupt or crafted key file in
`keys/` could make opening the repository
- panic for `N = 0` (`assert!(x > 0)` in `log_2`),
- abort on allocation failure for a huge `N * r` (e.g. `N = 2^31`,
  `r = 8` tries to allocate 2 TiB), or
- run for hours for a huge `p` (e.g. `N = 2^15`, `r = 8`, `p = 2^20`).

Reject such parameters with an error before running scrypt: `N` must
not be zero, the memory scrypt allocates (`128 * r * (N + p)` bytes)
must not exceed 1 GiB and the work (`128 * r * N * p`) must not exceed
2^32. Keys created by rustic (`N = 2^17`, `r = 8`, `p = 1`) use 128 MiB
and keys created by restic at most 60 MiB; both need a work of about
2^27, so the limits leave a wide margin for existing keys.

Add tests opening a repository with key files using these parameters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant