Throw on below-minimum opslimit/memlimit in sodium pwhash#22383
Conversation
The four sodium pwhash functions queued a zend_argument_error for an opslimit or memlimit below the documented minimum but fell through to the KDF instead of returning. When libsodium rejects the value the precise argument error is clobbered by a generic "internal error"; when it accepts the value the full KDF runs before the queued error surfaces, defeating the minimum-cost gate. Add the missing RETURN_THROWS() so each lower-bound check returns like its sibling upper-bound branches.
TimWolla
left a comment
There was a problem hiding this comment.
LGTM.
Can you double-check that sodium_remove_param_values_from_backtrace() keeps working with this change before merging? I suspect it does, but better to make sure.
| @@ -1532,9 +1533,11 @@ PHP_FUNCTION(sodium_crypto_pwhash_str) | |||
| } | |||
| if (opslimit < crypto_pwhash_OPSLIMIT_MIN) { | |||
| zend_argument_error(sodium_exception_ce, 2, "must be greater than or equal to %d", crypto_pwhash_OPSLIMIT_MIN); | |||
There was a problem hiding this comment.
This probably should also just be a ValueError for master. Though I think this conflicts with the “stack scrubbing” feature of ext/sodium (sodium_remove_param_values_from_backtrace). But this might be okay with SensitiveParameter and ValueErrors not being something that should happen at runtime.
There was a problem hiding this comment.
Reasonable for master, but as a separate change: this one is based on 8.4 where the type can't change, and to stay consistent it would need to flip every zend_argument_error(sodium_exception_ce, ...) site here, not just the lower bounds. Scrubbing isn't a blocker for that either, $password is #[\SensitiveParameter] so a ValueError backtrace redacts it too. I can do that master-only follow-up if you want it.
There was a problem hiding this comment.
Scrubbing isn't a blocker for that either,
The sodium scrubbing affects the entire stack trace, not just the sodium function itself. Thus:
function foo($pw) {
sodium_crypto_pwhash_str($pw, 0, 0);
}
foo('abc');
Currently the call to foo() is also protected, despite not having the attribute itself.
I can do that master-only follow-up if you want it.
Yes, I think this would be reasonable, even if only to have something to discuss.
There was a problem hiding this comment.
Ah, you're right, the whole-trace scrub covers the caller frames that SensitiveParameter doesn't, so a bare ValueError would leak the password through foo()'s frame. The follow-up will need to keep an explicit sodium_remove_param_values_from_backtrace(EG(exception)) after the throw, like the ZPP sites already do. I'll open it against master.
|
Confirmed. The scrubbing runs in |
The four sodium pwhash functions queue a zend_argument_error when opslimit or memlimit is below the documented minimum but fall through to the KDF instead of returning. When libsodium rejects the value the precise argument error is clobbered by a generic "internal error"; when it accepts the value the full KDF runs before the queued error surfaces, defeating the minimum-cost gate. Adding the missing RETURN_THROWS() makes each lower-bound check return like its sibling upper-bound branches.