Skip to content

Fix: Buffer overflow in CHDDisc::tryOpen#2384

Open
LucaPalumbo wants to merge 1 commit into
flyinghead:masterfrom
LucaPalumbo:master
Open

Fix: Buffer overflow in CHDDisc::tryOpen#2384
LucaPalumbo wants to merge 1 commit into
flyinghead:masterfrom
LucaPalumbo:master

Conversation

@LucaPalumbo

Copy link
Copy Markdown

I have identified a potential stack buffer overflow vulnerability in the CHDDisk::TryOpen function. The issue occurs when the function parses the header of a CHD file using sscanf with an unbounded %s formatter.

Because the destination buffers for the parsed strings (such as type, subtype, pgtype, and pgsub) are only 16 bytes in length, parsing a malformed or maliciously crafted CHD header can overflow these buffers. The theoretical maximum write is bounded only by the size of the temp buffer, which is 512 bytes (not enough to reach the return address in stack).

The bug is present in every of the following lines:

			sscanf(temp, CDROM_TRACK_METADATA2_FORMAT, &tkid, type, subtype, &frames, &pregap, pgtype, pgsub, &postgap);
			sscanf(temp, CDROM_TRACK_METADATA_FORMAT, &tkid, type, subtype, &frames);
			sscanf(temp, GDROM_TRACK_METADATA_FORMAT, &tkid, type, subtype, &frames, &padframes, &pregap, pgtype, pgsub, &postgap);

since the format strings are defined as:

#define CDROM_TRACK_METADATA_FORMAT	"TRACK:%d TYPE:%s SUBTYPE:%s FRAMES:%d"
#define CDROM_TRACK_METADATA2_FORMAT	"TRACK:%d TYPE:%s SUBTYPE:%s FRAMES:%d PREGAP:%d PGTYPE:%s PGSUB:%s POSTGAP:%d"
#define GDROM_TRACK_METADATA_FORMAT	"TRACK:%d TYPE:%s SUBTYPE:%s FRAMES:%d PAD:%d PREGAP:%d PGTYPE:%s PGSUB:%s POSTGAP:%d"

A possible fix would be to use %15s everywhere, like in this PR.

Next step:
I think the same fix should be applied also to the dependency https://github.com/flyinghead/libchdr to always avoid the occurrence of the bug.

@flyinghead

Copy link
Copy Markdown
Owner

core/deps/libretro-common/include/libchdr/chd.h isn't used by Flycast. The one in core/deps/libchdr/include/libchdris used instead.
So this fix should go to the upstream repository (https://github.com/flyinghead/libchdr), or even better, to the original upstream one (https://github.com/rtissera/libchdr), which is used by many other emulators. (Flycast uses a fork of libchdr to fix some Flycast-specific build issues.)

@LucaPalumbo

Copy link
Copy Markdown
Author

I opened an issue on libchdr.
In the meantime, what do you think about fixing it by hardcoding the format strings in the .cpp file like this?

			sscanf(temp, "TRACK:%d TYPE:%15s SUBTYPE:%15s FRAMES:%d PREGAP:%d PGTYPE:%15s PGSUB:%15s POSTGAP:%d", &tkid, type, subtype, &frames, &pregap, pgtype, pgsub, &postgap);
			sscanf(temp, "TRACK:%d TYPE:%15s SUBTYPE:%15s FRAMES:%d", &tkid, type, subtype, &frames);
			sscanf(temp, "TRACK:%d TYPE:%15s SUBTYPE:%15s FRAMES:%d PAD:%d PREGAP:%d PGTYPE:%15s PGSUB:%15s POSTGAP:%d", &tkid, type, subtype, &frames, &padframes, &pregap, pgtype, pgsub, &postgap);

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