Conversation
…lamp max_lvl before both enumeration branches nb_parts() only ever writes nbparts[c, len, level] for level <= max_lvl, and its terminal case at len == max_len never recurses past it, so nothing beyond either bound is populated. show_pwd_r() and show_pwd_rnbs() compute a candidate level or length one step ahead of what they currently hold and indexed nbparts with it before checking either bound, reading whatever memory the resulting offset landed on. A max_lvl small enough that no single character fits under it made this reachable from argv, since print_pwd() then leaves the starting password at length 0, and show_pwd() forced it back to length 1 with a level nothing had bounded. All six such reads now go through one get_nbparts() that returns 0 outside nb_parts()'s domain, the same thing nb_parts() itself would have returned for a level over max_lvl. Separately, the max_lvl > MAX_MKV_LVL clamp only ran on the path after both enumeration branches in main(). Either branch sizes nbparts from the same unclamped max_lvl, which is how an unbounded value on the command line reached mem_alloc(). Moved the clamp before both. Fixes openwall#5799 Fixes openwall#5798
Author
|
@solardiz friendly ping — this small genmkvpwd bounds fix is ready for review whenever you have a moment. Happy to adjust. |
Member
|
Yes, thank you! As you can see, we have quite many pending PRs to get through here. We intend to. For now, I let the rest of CI jobs run on this one. |
This was referenced Sep 21, 2026
Open
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
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.
Two out-of-bounds bugs in genmkvpwd, both from max_lvl/max_len/start not being checked against what nb_parts() actually populated.
#5799: out-of-bounds read
nb_parts() only writes nbparts[c, len, level] for level <= max_lvl and stops recursing at len == max_len, so nothing past either bound is set. show_pwd_r() and show_pwd_rnbs() compute a candidate level or length one step ahead and index nbparts with it before checking the bounds, so they read past the populated region.
The first case is print_pwd() not finding even one character under max_lvl, leaving the password at length 0; show_pwd() then forced it back to length 1 with a level from an unbounded proba1[0]. I added a check after print_pwd() for that. The second is that every read in both functions has the same shape - compute a candidate len/level, index nbparts immediately - so I routed all six through one get_nbparts() that returns 0 outside nb_parts()'s domain, matching what nb_parts() returns for level > max_lvl.
For ordinary parameters the output is unchanged: I diff'd genmkvpwd ../run/stats 200 6 0 20 byte-for-byte between the unpatched and patched binary, identical.
#5798: memory exhaustion
The max_lvl > MAX_MKV_LVL clamp only ran after both special-case branches in main(), but each branch sizes its own nbparts allocation from the unclamped max_lvl, so a huge value on the command line reached mem_alloc() first.
I moved the existing clamp to right after argv parsing so it applies before either branch.
Compiles clean under gcc -Wall with ASan, and normal usage is byte-identical before and after.
Fixes #5799
Fixes #5798