ext/sodium: throw ValueError for pwhash argument errors#22388
Conversation
4bdd40d to
1c35eaa
Compare
| try { | ||
| wrap($secret); | ||
| } catch (\ValueError $e) { | ||
| echo get_class($e), "\n"; | ||
| echo $e->getMessage(), "\n"; | ||
| $leaked = str_contains($e->getTraceAsString(), $secret); | ||
| foreach ($e->getTrace() as $frame) { | ||
| if (!empty($frame['args'])) { | ||
| $leaked = true; | ||
| } | ||
| } | ||
| var_dump($leaked); | ||
| } |
There was a problem hiding this comment.
I think this test would be more readable when you would just leave the exception unhandled:
| try { | |
| wrap($secret); | |
| } catch (\ValueError $e) { | |
| echo get_class($e), "\n"; | |
| echo $e->getMessage(), "\n"; | |
| $leaked = str_contains($e->getTraceAsString(), $secret); | |
| foreach ($e->getTrace() as $frame) { | |
| if (!empty($frame['args'])) { | |
| $leaked = true; | |
| } | |
| } | |
| var_dump($leaked); | |
| } | |
| wrap($secret); |
There was a problem hiding this comment.
Switched to the uncaught form, matching exception_trace_without_args.phpt in the same dir. The fatal-error trace shows the scrubbed frames directly.
| still thrown for internal libsodium failures. Catch ValueError, or its | ||
| parent Error, for these argument errors. |
There was a problem hiding this comment.
| still thrown for internal libsodium failures. Catch ValueError, or its | |
| parent Error, for these argument errors. | |
| still thrown for internal libsodium failures. |
We should encourage folks to catch these. This is also not mentioned for the other ValueError conversions.
The four password-hashing functions reported out-of-range arguments (a non-positive or below-minimum opslimit or memlimit, an oversized hash length or password, a wrong-length salt) as a SodiumException. These are argument-value errors, so throw ValueError via zend_argument_value_error() instead, matching the rest of the engine. SodiumException is still used for internal libsodium failures. SodiumException's create_object empties the whole backtrace, which also protects caller frames holding the password; a plain ValueError does not, so each converted site keeps an explicit sodium_remove_param_values_from_backtrace(EG(exception)), mirroring the ZPP-failure paths.
1c35eaa to
cd0577c
Compare
TimWolla
left a comment
There was a problem hiding this comment.
LGTM now, but please wait for the second approval.
Girgias
left a comment
There was a problem hiding this comment.
Not sure if this is really worth it? I'd rather focus on cases that don't do any validation or emit E_WARNINGs.
Aside: is sodium_remove_param_values_from_backtrace still needed with the sensitive param attribute?
| } | ||
| if (hash_len <= 0) { | ||
| zend_argument_error(sodium_exception_ce, 1, "must be greater than 0"); | ||
| zend_argument_value_error(1, "must be greater than 0"); |
| } | ||
| if (hash_len >= 0xffffffff) { | ||
| zend_argument_error(sodium_exception_ce, 1, "is too large"); | ||
| zend_argument_value_error(1, "is too large"); |
There was a problem hiding this comment.
| zend_argument_value_error(1, "is too large"); | |
| zend_argument_value_error(1, "must be less than %d bytes"); |
(or whatever the correct format specifier is)
| } | ||
| if (passwd_len >= 0xffffffff) { | ||
| zend_argument_error(sodium_exception_ce, 2, "is too long"); | ||
| zend_argument_value_error(2, "is too long"); |
| } | ||
| if (passwd_len >= 0xffffffff) { | ||
| zend_argument_error(sodium_exception_ce, 1, "is too long"); | ||
| zend_argument_value_error(1, "is too long"); |
|
On whether it's worth it: this came out of the GH-22383 review suggestion to move argument errors off SodiumException. No strong attachment though; if the sodium sites that don't validate at all (or only emit E_WARNING) are the better target, I'll drop this and start there instead. On the aside: yes, still needed. |
Follow-up to GH-22383, as suggested in review: the four sodium pwhash functions now throw ValueError for out-of-range arguments instead of SodiumException, keeping SodiumException for internal libsodium failures. Each converted site keeps an explicit backtrace scrub so the ValueError still protects the password in caller frames, with a test covering it. Scoped to pwhash for now; changing the remaining zend_argument_error(sodium_exception_ce, ...) sites across the extension might be a bigger BC change, so keeping this one small.