CVE-2023-4508: Access of Uninitialized Pointer (CWE-824)
value: A user able to control file input to Gerbv, between versions 2.4.0 and 2.10.0, can cause a crash and cause denial-of-service with a specially crafted Gerber RS-274X file.
CWE description: The product accesses or uses a pointer that has not been initialized.
CVSS v3 base score: 5.5
Published: 2023-08-24T23:15Z
Repository: https://github.com/gerbv/gerbv
Language: C
Files changed: src/gerb_file.c, src/gerber.c, src/gerbv.c, test/inputs/parse_aperture_strtok.1.poc, test/inputs/parse_aperture_strtok.2.poc, test/run_valgrind_tests.sh, test/tests.list
Commit date: 2023-07-13 21:18:15 +0200
Commit message: fix: Out-of-bounds memory access of filename.

Put all the filename allocation and deallocation into the `gerb_fopen`
and `gerb_fclose` functions so that the caller doesn't need to deal
with this anymore.

Also, free includeFilename.  It was allocated as part of
`gerb_fgetstring()` above but never freed properly.

Unit tests added to valgrind.

Diff: diff --git a/src/gerb_file.c b/src/gerb_file.c
index 9a9735c..dbc2da1 100644
--- a/src/gerb_file.c
+++ b/src/gerb_file.c
@@ -129,10 +129,13 @@ gerb_fopen(char const * filename)
     }
     rewind (fd->fd);
 
 #endif
 
+    dprintf("     Setting filename\n");
+    fd->filename = g_strdup(filename);
+
     dprintf("<--- Leaving gerb_fopen\n");
     return fd;
 } /* gerb_fopen */
 
 
@@ -236,10 +239,12 @@ gerb_ungetc(gerb_file_t *fd)
 
 void
 gerb_fclose(gerb_file_t *fd)
 {
     if (fd) {
+        g_free(fd->filename);
+
 #ifdef HAVE_SYS_MMAN_H
 	if (munmap(fd->data, fd->datalen) < 0)
 	    GERB_FATAL_ERROR("munmap: %s", strerror(errno));
 #else
 	g_free(fd->data);
diff --git a/src/gerber.c b/src/gerber.c
index 50cf01c..1a608ab 100644
--- a/src/gerber.c
+++ b/src/gerber.c
@@ -1474,11 +1474,11 @@ parse_rs274x(gint levelOfRecursion, gerb_file_t *fd, gerbv_image_t *image,
 		    gerbv_stats_printf(error_list, GERBV_MESSAGE_ERROR, -1,
 			    _("Parser encountered more than 10 levels of "
 				"include file recursion which is not allowed "
 				"by the RS-274X spec"));
 		}
-		
+		g_free (includeFilename);
 	    }
 	}
 	break;
     case A2I('I','O'): /* Image offset */
 	op[0] = gerb_fgetc(fd);
diff --git a/src/gerbv.c b/src/gerbv.c
index 579d0f8..db10cc9 100644
--- a/src/gerbv.c
+++ b/src/gerbv.c
@@ -505,13 +505,10 @@ gerbv_open_image(gerbv_project_t *gerbvProject, gchar const* filename, int idx,
 	GERB_COMPILE_ERROR(_("Trying to open \"%s\": %s"),
 			filename, strerror(errno));
 	return -1;
     }
 
-    /* Store filename info fd for further use */
-    fd->filename = g_strdup(filename);
-    
     dprintf("In open_image, successfully opened file.  Now check its type....\n");
     /* Here's where we decide what file type we have */
     /* Note: if the file has some invalid characters in it but still appears to
        be a valid file, we check with the user if he wants to continue (only
        if user opens the layer from the menu...if from the command line, we go
@@ -576,11 +573,10 @@ gerbv_open_image(gerbv_project_t *gerbvProject, gchar const* filename, int idx,
 	dprintf("Unknown filetype");
 	GERB_COMPILE_ERROR(_("%s: Unknown file type."), filename);
 	parsed_image = NULL;
     }
     
-    g_free(fd->filename);
     gerb_fclose(fd);
     if (parsed_image == NULL) {
 	return -1;
     }
     
diff --git a/test/golden/parse_aperture_strtok.png b/test/golden/parse_aperture_strtok.png
new file mode 100644
index 0000000..9015ba7
Binary files /dev/null and b/test/golden/parse_aperture_strtok.png differ
diff --git a/test/inputs/parse_aperture_strtok.1.poc b/test/inputs/parse_aperture_strtok.1.poc
new file mode 100644
index 0000000..2dcfae5
--- /dev/null
+++ b/test/inputs/parse_aperture_strtok.1.poc
@@ -0,0 +1,11 @@
+%FSLAX25Y25*%
+%MOIN*%
+
+%ADD20C,BBBB*%
+
+%IFparse_aperture_strtok.2.poc*%
+
+D21*
+X400000Y400000D03*
+
+M02*
diff --git a/test/inputs/parse_aperture_strtok.2.poc b/test/inputs/parse_aperture_strtok.2.poc
new file mode 100644
index 0000000..fec5092
--- /dev/null
+++ b/test/inputs/parse_aperture_strtok.2.poc
@@ -0,0 +1,2 @@
+M09*
+
diff --git a/test/run_valgrind_tests.sh b/test/run_valgrind_tests.sh
index 5b1b052..0c6fa7c 100755
--- a/test/run_valgrind_tests.sh
+++ b/test/run_valgrind_tests.sh
@@ -1,5 +1,6 @@
 #!/bin/sh
 ./run_tests.sh --valgrind \
                example_cslk \
                out-of-bounds-drill-tool \
-               example_am_test
+               example_am_test \
+               parse_aperture_strtok
diff --git a/test/tests.list b/test/tests.list
index 64dfa68..3c68777 100644
--- a/test/tests.list
+++ b/test/tests.list
@@ -180,10 +180,12 @@ test-circular-interpolation-mq-ccw      |  test-circular-interpolation-mq-ccw.gb
 test-circular-interpolation-zero-error  |  test-circular-interpolation-zero-error.gbx
 
 test-merge-a+b_temporary | test-merge-a.gbx test-merge-b.gbx | ! --export=rs274x --output=inputs/test-merge-a+b_temporary.gbx
 test-merge-a+b | test-merge-a+b_temporary.gbx
 
+parse_aperture_strtok | parse_aperture_strtok.1.poc
+
 # ---------------------------------------------
 # Excellon drill tests
 # ---------------------------------------------
 test-drill-trailing-zero-1 | test-drill-trailing-zero-1.exc
 test-drill-leading-zero-1 | test-drill-leading-zero-1.exc

Vulnerable code: 		
    /* Store filename info fd for further use */
    fd->filename = g_strdup(filename);
    
    g_free(fd->filename);
               example_am_test
Fixed code:     dprintf("     Setting filename\n");
    fd->filename = g_strdup(filename);

        g_free(fd->filename);

		g_free (includeFilename);
%FSLAX25Y25*%
%MOIN*%

%ADD20C,BBBB*%

%IFparse_aperture_strtok.2.poc*%

D21*
X400000Y400000D03*

M02*
M09*

               example_am_test \
               parse_aperture_strtok
parse_aperture_strtok | parse_aperture_strtok.1.poc