Add upper limit to PBKDF iteration count#10050
Conversation
|
Jenkins retest this please. |
anhu
force-pushed
the
pbkdf_max
branch
2 times, most recently
from
1bcbddc to
fca6e23
Compare
|
jenkins retest this please |
|
jenkins retest this please. |
1 similar comment
|
jenkins retest this please. |
|
jenkins retest this please Aborted |
|
jenkins retest this please |
anhu
added
For This Release
douzzer
left a comment
douzzer
left a comment
There was a problem hiding this comment.
This limit is far too low.
Aside from that, the limit should not be imposed in the crypto layer, it should be imposed by the caller. Only the caller knows the provenance of the iteration count, so only the caller can impose limits that are appropriate.
Here is an AI summary of real world PBKDF iterations:
The highest PBKDF2 iteration count found in widespread/mainstream usage is 10,000,000 (10 million).
This comes from Apple’s iOS/macOS data protection system. It’s used to derive keys from backup passwords (or similar passcodes) to protect keybags—the encrypted containers holding Data Protection class keys, Keychain items, and related secrets in iTunes/iCloud backups and on-device protection. It has been documented since around iOS 10.2 (with PBKDF2-SHA256) and is still referenced in Apple’s security documentation as using a “large iteration count” of PBKDF2. This is extremely mainstream (billions of devices).
Other notable mainstream examples (all PBKDF2 unless noted)
Password managers (very common real-world use):
Keeper: Defaults to 1,000,000 iterations (PBKDF2-HMAC-SHA256) for master password key derivation on the client.
1Password: ~650,000 iterations.
Bitwarden: Default 600,000 (aligned with OWASP); users can (and do) configure up to the enforced cap of 2,000,000.
LastPass: Defaults in the 100k–600k range historically/recently (varies by account age).
Disk encryption / volume tools:
VeraCrypt (successor to TrueCrypt): Defaults to 200,000–500,000 depending on volume type/hash (e.g., 500k for non-system containers with most hashes). Higher values are possible via the PIM (Personal Iterations Multiplier) setting (e.g., PIM 485 yields the 500k default; users can go higher), but the defaults are the mainstream baseline.
Frameworks / libraries (common in web/server apps):
Django (recent versions, e.g., 5.x): Defaults increased to 1,000,000 iterations for PBKDF2-SHA256.
Standards / recommendations (not usage, but influential):
OWASP Password Storage Cheat Sheet (current): Minimum 600,000 for PBKDF2-HMAC-SHA256 (or ~1.4M for SHA-1, 210k–220k for SHA-512). This is a floor for new deployments, not a ceiling.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 1 skipped
Posted findings
- [High] Missing WC_PBKDF_MAX_ITERATIONS check in non-MP-API wc_PKCS12_PBKDF_ex —
wolfcrypt/src/pwdbased.c:625-627 - [Medium] No test coverage for WC_PBKDF_MAX_ITERATIONS enforcement —
wolfcrypt/test/test.c
Skipped findings
- [Medium] No test coverage for WC_PBKDF_MAX_ITERATIONS enforcement
Review generated by Skoll via openclaw
|
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 2 posted, 3 skipped
Posted findings
- [Medium] No test coverage for new upper-bound iteration check —
wolfcrypt/src/pwdbased.c:82-85,223-226,416-420,630-634 - [Medium] Backward-compatibility / behavior change not documented —
wolfssl/wolfcrypt/pwdbased.h:38-45
Skipped findings
- [Low] Duplicated bounds-check block could be a small helper/macro
- [Low] Check placed after iteration clamp; consider ordering
- [Medium] No test coverage for new upper-bound iteration check
Review generated by Skoll via openclaw
|
retest this please |
Add WC_PBKDF_MAX_ITERATIONS (default 100000) to cap the iteration count in wc_PBKDF1_ex(), wc_PBKDF2_ex(), and wc_PKCS12_PBKDF_ex().
…ise it to 10000000, add wc_PBKDF_max_iterations_set() and wc_PBKDF_max_iterations_get(), and restore new negative tests in pwdbased_test().
* add correct gating around pbkdf1_test(), pkcs12_pbkdf_test(), and scrypt_test() prototypes; * add unit tests for wc_PBKDF_max_iterations_set() and wc_PBKDF_max_iterations_get() in pbkdf2_test(); * fix pkcs12_test() to skip the evilPkcs12 test if evil_p12 can't be parsed for any reason, mirroring the new stanza around evil_p12 in pwdbased_test().
* fix typography of wc_PBKDF_max_iterations_set() and wc_PBKDF_max_iterations_get() (peer review). * refactor overflow prevention in wc_PKCS12_PBKDF_ex() to use WC_SAFE_SUM_UNSIGNED(). wolfcrypt/test/test.c: in pwdbased_test(), omit "INT_MAX MAC iterations" test if WOLFSSL_NO_MALLOC (uses wc_PKCS12_new_ex()).
…t codes in pwdbased_test().
|
retest this please |
* in pbkdf2_test(), pwdbased_test(), and pkcs12_test(), add missing FIPS v7+
gates around stanzas that use wc_PBKDF_max_iterations_set() and
wc_PBKDF_max_iterations_get() or depend on erroring for excessive PBKDF
iterations (fixes wolfSSL#10050);
* in ecc_test_buffers(), omit new corrupt HMAC tag test on FIPS <v6 (fixes
8f2a3f9).
tests/api/test_dtls.c: add FIPS v7+ gate to test_dtls13_frag_ch2_with_ch1_rtx().
wolfssl/wolfcrypt/memory.h: #include "../../linuxkm/linuxkm_memory.h" rather than "linuxkm/linuxkm_memory.h", following pattern in wc_port.h.
5 participants
Fixes ZD 21402