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

Fix CVE-2023-31722 #83

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hongjinghao
Copy link

paramlen has heap memory of length nparam+1. The value of variable i may be greater than nparam+1, causing heap memory overflow. Therefore, i and nparam+1 needs to be determined in the loop.
Fix:https://bugzilla.nasm.us/show_bug.cgi?id=3392857#c1

@sezero
Copy link
Contributor

sezero commented Sep 5, 2023

Patch is utterly broken because:

(1) wrong parenthesing:

asm/preproc.c: In function ‘expand_mmacro’:
asm/preproc.c:6820: warning: assignment makes pointer from integer without a cast

... and:
(2) it allows out-of-bounds read

... and still segfaults, of course:

$ valgrind ./nasm -f elf64 ../nasm-crash -o 1.o
==5002== Memcheck, a memory error detector
==5002== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==5002== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==5002== Command: ./nasm -f elf64 ../nasm-crash -o 1.o
==5002== 
../nasm-crash:2: error: parser: instruction expected
../nasm-crash:5: error: label or instruction expected at start of line
../nasm-crash:6: error: parser: instruction expected
../nasm-crash:8: error: label or instruction expected at start of line
../nasm-crash:9: error: parser: instruction expected
../nasm-crash:12: warning: unterminated %[...] construct (missing `]') [-w+pp-open-brackets]
../nasm-crash:12: error: `%0': not in a macro call
../nasm-crash:12: error: `%1': not in a macro call
../nasm-crash:12: error: `%2': not in a macro call
../nasm-crash:12: error: `%3': not in a macro call
../nasm-crash:12: error: `%4': not in a macro call
../nasm-crash:12: error: `%5': not in a macro call
../nasm-crash:12: warning: label alone on a line without a colon might be in error [-w+label-orphan]
../nasm-crash:13: error: `%ENDMACRO': not defining a macro
../nasm-crash:14: error: parser: instruction expected
../nasm-crash:15: error: parser: instruction expected
../nasm-crash:28: error: parser: instruction expected
../nasm-crash:29: error: parser: instruction expected
==5002== Invalid read of size 4
==5002==    at 0x8061168: pp_tokline (preproc.c:673)
==5002==    by 0x8063D6C: pp_getline (preproc.c:7823)
==5002==    by 0x804A710: assemble_file (nasm.c:1719)
==5002==    by 0x804C4CC: main (nasm.c:716)
==5002==  Address 0x5 is not stack'd, malloc'd or (recently) free'd
==5002== 
==5002== 
==5002== Process terminating with default action of signal 11 (SIGSEGV)
==5002==  Access not within mapped region at address 0x5
==5002==    at 0x8061168: pp_tokline (preproc.c:673)
==5002==    by 0x8063D6C: pp_getline (preproc.c:7823)
==5002==    by 0x804A710: assemble_file (nasm.c:1719)
==5002==    by 0x804C4CC: main (nasm.c:716)

Correct one should be like:

diff --git a/asm/preproc.c b/asm/preproc.c
index ac42131e..24f167b9 100644
--- a/asm/preproc.c
+++ b/asm/preproc.c
@@ -6817,7 +6817,7 @@ static int expand_mmacro(Token * tline)
      */
     nasm_newn(paramlen, nparam+1);
 
-    for (i = 1; (t = params[i]); i++) {
+    for (i = 1; i < nparam+1 && (t = params[i]); i++) {
         bool braced = false;
         int brace = 0;
         int white = 0;

paramlen has heap memory of length nparam+1. The value of variable i may be greater than nparam+1, causing heap memory overflow. Therefore,  i and nparam+1 needs to be determined in the loop.
fix:https://bugzilla.nasm.us/show_bug.cgi?id=3392857#c1
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.

2 participants