openwall / openwall/john

Stack buffer overflow in genmkvpwd due to excessive max_len parameter

Open Beginner friendly
#5,797 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug maintenance/cleanup
Dominant language
C
Stars
13.6k
Forks
2.6k
PR merge metrics
No merged PRs in 30d

Description

Hi, we have found a stack buffer overflow and would like to report this issue.
We're running the proposed fix locally but haven't tested thoroughly. Please make any needed corrections and push.

Summary

When max_len > MAX_MKV_LEN, invalid memory access occurs.

Reproduction

  • Operating System: Ubuntu 24.04 LTS
  • Architecture: x86_64
  • Compiler: GCC 14.2.0

Reproduction Steps

git clone https://github.com/openwall/john
cd john/src
./configure --enable-asan
make
../run/genmkvpwd  ../run/stats -1
Output
allocated 17408 KB for nbparts
70 G possible passwords (70399740865)
=================================================================
==1042409==ERROR: AddressSanitizer: unknown-crash on address 0x7f8213d0002c at pc 0x5807372ab9f3 bp 0x7ffc9e8c2f80 sp 0x7ffc9e8c2f70
WRITE of size 34 at 0x7f8213d0002c thread T0
    #0 0x5807372ab9f2 in memset /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59
    #1 0x5807372ab9f2 in main /tmp/john/src/genmkvpwd.c:294
    #2 0x7f8215c2a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #3 0x7f8215c2a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #4 0x5807372a97e4 in _start (/tmp/john/run/genmkvpwd+0x47e4) (BuildId: b4e075dafb2127c3f70d28cd97c5c1dcc7034212)

Address 0x7f8213d0002c is located in stack of thread T0 at offset 44 in frame
    #0 0x5807372aab71 in main /tmp/john/src/genmkvpwd.c:200

  This frame has 2 object(s):
    [32, 76) 'pwd' (line 201) <== Memory access at offset 44 partially overflows this variable
    [112, 156) 'pwd2' (line 202)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: unknown-crash /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59 in memset
Shadow bytes around the buggy address:
  0x7f8213cffd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213cffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213cffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213cfff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213cfff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x7f8213d00000: f1 f1 f1 f1 00[00]00 00 00 04 f2 f2 f2 f2 00 00
  0x7f8213d00080: 00 00 00 00 00 04 f3 f3 f3 f3 00 00 00 00 00 00
  0x7f8213d00100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213d00180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213d00200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7f8213d00280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1042409==ABORTING

Root Cause Analysis

[src/genmkvpwd.c:294](https://github.com/openwall/john/blob/5baa3c47dbef1466b106a5894e3096b13a8c22f1/src/genmkvpwd.c#L294)
When the value of max_len exceeds the length of pwd.password, it causes invalid memory access.

[src/mkvlib.h:17](https://github.com/openwall/john/blob/5baa3c47dbef1466b106a5894e3096b13a8c22f1/src/mkvlib.h#L17)
The size of s_pwd.password is MAX_MKV_LEN + 1.

[src/params.h:482](https://github.com/openwall/john/blob/5baa3c47dbef1466b106a5894e3096b13a8c22f1/src/params.h#L482)
Reference: MAX_MKV_LEN is 30.

Proposed Fix

Adding a check similar to MAX_MKV_LVL can avoid this issue.
Using strtol instead of atoi for parsing max_len would be safer.

diff --git a/src/genmkvpwd.c b/src/genmkvpwd.c
index de62d0330..59ce93f91 100644
--- a/src/genmkvpwd.c
+++ b/src/genmkvpwd.c
@@ -272,6 +272,10 @@ int main(int argc, char * * argv)
 	}
 
 	nbparts = mem_alloc(256*(max_lvl+1)*sizeof(long long)*(max_len+1));
+	if (max_len > MAX_MKV_LEN){
+		fprintf(stderr, "Warning: max_len = %d is too large (max = %d)\n", max_len,  MAX_MKV_LEN);
+		max_len = MAX_MKV_LEN;
+	}
 	fprintf(stderr, "allocated %lu KB for nbparts\n", (unsigned long)(256UL*(max_lvl+1)*(max_len+1)*sizeof(long long)/1024));
 	memset(nbparts, 0, 256*(max_lvl+1)*(max_len+1)*sizeof(long long));

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/genmkvpwd.c at the max_len parsing and memset around line 294, then review the buffer definition in src/mkvlib.h and MAX_MKV_LEN in src/params.h. Build with --enable-asan and run the reported genmkvpwd command; done means excessive max_len input no longer causes an invalid memory access and the existing behavior remains valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
cli, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.