NULL Pointer Dereference still exists in gf_isom_parse_movie_boxes_internal

[0xdd96]

version info:

root@d8a714203f6e:# ./MP4Box -version
MP4Box - GPAC version 2.1-DEV-rev87-g053aae8-master
(c) 2000-2022 Telecom Paris distributed under LGPL v2.1+ - http://gpac.io

Please cite our work in your research:
        GPAC Filters: https://doi.org/10.1145/3339825.3394929
        GPAC: https://doi.org/10.1145/1291233.1291452

GPAC Configuration: --prefix=/path_to_gpac/build --enable-debug --enable-sanitizer
Features: GPAC_CONFIG_LINUX GPAC_64_BITS GPAC_HAS_IPV6 GPAC_HAS_SSL GPAC_HAS_SOCK_UN GPAC_MINIMAL_ODF GPAC_HAS_QJS GPAC_HAS_FAAD GPAC_HAS_MAD GPAC_HAS_LIBA52 GPAC_HAS_JPEG GPAC_HAS_PNG GPAC_HAS_FFMPEG GPAC_HAS_JP2 GPAC_HAS_THEORA GPAC_HAS_VORBIS GPAC_HAS_XVID GPAC_HAS_LINUX_DVB

poc:poc
command: MP4Box -hint -out /dev/null $poc$
crash:

root@d8a714203f6e:# ./MP4Box -hint -out /dev/null poc
[iso file] Read Box type 00000000 (0x00000000) at position 45 has size 0 but is not at root/file level. Forbidden, skipping end of parent box !
[iso file] Read Box "abst" (start 0) failed (Unknown Error (10)) - skipping
isomedia/isom_intern.c:392:12: runtime error: member access within null pointer of type 'struct GF_Box'

When size=0 and is_root_box=false, gf_isom_box_parse_ex will return GF_SKIP_BOX (i.e., 10) at line 138 of box_funcs.c.

gpac/src/isomedia/box_funcs.c

Lines 129 to 142 in 7f060bb

	if (!size) {
	if (is_root_box) {
	if (!skip_logs) {
	GF_LOG(GF_LOG_DEBUG, GF_LOG_CONTAINER, ("[iso file] Warning Read Box type %s (0x%08X) size 0 reading till the end of file\n", gf_4cc_to_str(type), type));
	}
	size = gf_bs_available(bs) + 8;
	} else {
	if (!skip_logs) {
	GF_LOG(GF_LOG_ERROR, GF_LOG_CONTAINER, ("[iso file] Read Box type %s (0x%08X) at position "LLU" has size 0 but is not at root/file level. Forbidden, skipping end of parent box !\n", gf_4cc_to_str(type), type, start));
	return GF_SKIP_BOX;
	}
	return GF_OK;
	}
	}

This will cause *outBox to be set to NULL (in box_funcs.c:312) and the return value GF_SKIP_BOX will be passed to the upper function ( in box_funcs.c:318).

gpac/src/isomedia/box_funcs.c

Lines 310 to 319 in 7f060bb

	if (e && (e != GF_ISOM_INCOMPLETE_FILE)) {
	gf_isom_box_del(newBox);
	*outBox = NULL;
	if (!skip_logs) {
	GF_LOG(GF_LOG_ERROR, GF_LOG_CONTAINER, ("[iso file] Read Box \"%s\" (start "LLU") failed (%s) - skipping\n", gf_4cc_to_str(type), start, gf_error_to_string(e)));
	}
	//we don't try to reparse known boxes that have been failing (too dangerous)
	return e;
	}

The program now executes the empty if block when e>=0( in isom_intern.c:375-377), and later dereferences the null pointer in line 392 of isom_intern.c.

gpac/src/isomedia/isom_intern.c

Lines 373 to 392 in 7f060bb

	e = gf_isom_parse_root_box(&a, mov->movieFileMap->bs, boxType, bytesMissing, progressive_mode);
	if (e >= 0) {
	} else if (e == GF_ISOM_INCOMPLETE_FILE) {
	/*our mdat is uncomplete, only valid for READ ONLY files...*/
	if (mov->openMode != GF_ISOM_OPEN_READ) {
	GF_LOG(GF_LOG_ERROR, GF_LOG_CONTAINER, ("[iso file] Incomplete MDAT while file is not read-only\n"));
	return GF_ISOM_INVALID_FILE;
	}
	if ((mov->openMode == GF_ISOM_OPEN_READ) && !progressive_mode) {
	GF_LOG(GF_LOG_ERROR, GF_LOG_CONTAINER, ("[iso file] Incomplete file while reading for dump - aborting parsing\n"));
	break;
	}
	return e;
	} else {
	return e;
	}
	switch (a->type) {

Note that although the crash path is the same as in issue #2155, their root cause is different.

[hmnthabit]

Thanks for reporting this

What's the problem of checking a strucut before the switch statement? @jeanlf

Also, it's better to test the poc against all the arguments that do file parsing.