ext/sodium: throw ValueError for pwhash argument errors#22388
Open
iliaal wants to merge 1 commit into
Open
Conversation
iliaal
force-pushed
the
sodium-pwhash-valueerror
branch
from
4bdd40d to
1c35eaa
Compare
TimWolla
reviewed
Jun 21, 2026
Comment on lines
+17
to
+29
| 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); | ||
| } |
Member
There was a problem hiding this comment.
I think this test would be more readable when you would just leave the exception unhandled:
Suggested change
| 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); |
Contributor
Author
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.
Comment on lines
+138
to
+139
| still thrown for internal libsodium failures. Catch ValueError, or its | ||
| parent Error, for these argument errors. |
Member
There was a problem hiding this comment.
Suggested change
| 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.
iliaal
force-pushed
the
sodium-pwhash-valueerror
branch
from
1c35eaa to
cd0577c
Compare
TimWolla
approved these changes
Jun 21, 2026
TimWolla
left a comment
TimWolla
left a comment
Member
There was a problem hiding this comment.
LGTM now, but please wait for the second approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.