Skip to content

genmkvpwd: bound every nbparts read, clamp max_lvl before both enumeration branches - #6036

Open
MRX-72 wants to merge 1 commit into
openwall:bleeding-jumbofrom
MRX-72:fix-genmkvpwd-bounds
Open

MRX-72 wants to merge 1 commit into
openwall:bleeding-jumbofrom
MRX-72:fix-genmkvpwd-bounds

Conversation

@MRX-72

@MRX-72 MRX-72 commented Sep 17, 2026

Copy link
Copy Markdown

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.

$ ./configure --enable-asan && make -sj4 ../run/genmkvpwd
$ ../run/genmkvpwd ../run/stats 1 1 1
before: ASan BUS at genmkvpwd.c:61 in show_pwd_r
after:  "No password reachable at index 1 ...", rc=0

$ ../run/genmkvpwd ../run/stats 27 3 1
before: ASan SEGV at genmkvpwd.c:70 in show_pwd_r
after:  prints c / c / m / s / a, rc=0

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.

$ ../run/genmkvpwd ../run/stats 99999
before: 1400000 KB for nbparts at len=6, climbing past 4 GB
after:  "Warning: Level = 99999 is too large (max = 400)"

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

…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
@MRX-72

MRX-72 commented Sep 20, 2026

Copy link
Copy Markdown
Author

@solardiz friendly ping — this small genmkvpwd bounds fix is ready for review whenever you have a moment. Happy to adjust.

@solardiz

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Out-of-bounds array access in genmkvpwd genmkvpwd max_lvl parameter memory exhaustion

2 participants