Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fnematch: fix out-of-bounds access on EOF #211

Closed
wants to merge 1 commit into from

Conversation

millert
Copy link
Contributor

@millert millert commented Nov 14, 2023

fnematch() expects to store a NUL byte when EOF is encountered. However, the rewrite broke this assumption because r.len from getrune() is zero on EOF. This results in j becoming negative on EOF, causing an out-of-bounds access. It is simplest to just force r.len to 1 on EOF to copy a single NUL byte--the rune is initialized to zero even for EOF.

This also fixes the call to adjbuf(). We cannot use 'k' to determine when we need to expand the buffer now that we are potentially reading more than a single byte at a time.

fnematch() expects to store a NUL byte when EOF is encountered.
However, the rewrite broke this assumption because r.len from getrune()
is zero on EOF.  This results in j becoming negative on EOF, causing an
out-of-bounds access.  It is simplest to just force r.len to 1 on EOF
to copy a single NUL byte--the rune is initialized to zero even for EOF.

This also fixes the call to adjbuf().  We cannot use 'k' to determine
when we need to expand the buffer now that we are potentially reading
more than a single byte at a time.
@millert
Copy link
Contributor Author

millert commented Nov 14, 2023

This bug causes a crash with OpenBSD's malloc but on Linux you will need to use ASAN or valgrind. A reduced testcase is:

printf "route:\t194.32.71.0/24\n" | ./a.out 'BEGIN{ RS = "route:" ; FS = "\n" }{ print $1 }'

ASAN output:

==3037598==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6250000000ff at pc 0x55b393121e09 bp 0x7ffe78860c30 sp 0x7ffe78860c20
READ of size 1 at 0x6250000000ff thread T0
    #0 0x55b393121e08 in fnematch b.c:872
    #1 0x55b39312a754 in readrec lib.c:246
    #2 0x55b39312d40f in getrec lib.c:179
    #3 0x55b3931304c0 in program run.c:197
    #4 0x55b3931301da in execute run.c:166
    #5 0x55b39313c9e7 in run run.c:141
    #6 0x55b3931232da in main main.c:233
    #7 0x7fc9d1423a8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #8 0x7fc9d1423b48 in __libc_start_main_impl ../csu/libc-start.c:360
    #9 0x55b393117b64 in _start (a.out+0x13b64) (BuildId: 35cebf6c2cfeab9be9c4e3a90c48ef266d6e9114)

0x6250000000ff is located 1 bytes before 8192-byte region [0x625000000100,0x625000002100)
allocated by thread T0 here:
    #0 0x7fc9d18defef in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x55b3931298d9 in recinit lib.c:67
    #2 0x55b3931231ad in main main.c:214
    #3 0x7fc9d1423a8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

SUMMARY: AddressSanitizer: heap-buffer-overflow b.c:872 in fnematch

@plan9
Copy link
Collaborator

plan9 commented Nov 14, 2023

thanks todd, this will go with some other changes tonight.

@mpinjr mpinjr mentioned this pull request Nov 15, 2023
bob-beck pushed a commit to openbsd/src that referenced this pull request Nov 15, 2023
fnematch() expects to store a NUL byte when EOF is encountered.
However, the rewrite broke this assumption because r.len from getrune()
is zero on EOF.  This results in j becoming negative on EOF, causing an
out-of-bounds access.  It is simplest to just force r.len to 1 on EOF
to copy a single NUL byte--the rune is initialized to zero even for EOF.

This also fixes the call to adjbuf().  We cannot use 'k' to determine
when we need to expand the buffer now that we are potentially reading
more than a single byte at a time.

onetrueawk/awk#211
@plan9 plan9 closed this Nov 20, 2023
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.

None yet

2 participants