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

Heap Overflow while parsing Amiga Oktalyzer files #202

Closed
nicowaisman opened this issue Aug 1, 2019 · 4 comments
Closed

Heap Overflow while parsing Amiga Oktalyzer files #202

nicowaisman opened this issue Aug 1, 2019 · 4 comments
Labels

Comments

@nicowaisman
Copy link

Hey schismtracker team,
I would like to report a security vulnerability in Amiga Oktalyzer parser (fmt/okt.c).

There is a heap overflow in the way the parser handles Song's orderlist in Amiga Oktalyzer file format.
The fmt_okt_load_song function, takes the 2 bytes long length (plen) of the song orderlist directly from the file.
At the end of the function, it try to memser the structure, however the size of the memset is calculated by substracting MAX_ORDERS minus the plen ([2]). As a consequence, if a file is created with a plen bigger than MAX_ORDER (256), it will underflow and become a big unsigned integer that will make memset overflow beyond their boundaries.

int fmt_okt_load_song(song_t *song, slurp_t *fp, unsigned int lflags)
{
int plen = 0; // how many positions in the orderlist are valid

while (!slurp_eof(fp)) {
	uint32_t blklen; // length of this block
	size_t nextpos; // ... and start of next one

	slurp_read(fp, tag, 4);
	slurp_read(fp, &blklen, 4);
	blklen = bswapBE32(blklen);
	nextpos = slurp_tell(fp) + blklen;
	[...]
	switch (OKT_BLOCK(tag[0], tag[1], tag[2], tag[3])) {
	[...]
	case OKT_BLK_PLEN:
		if (!(readflags & OKT_HAS_PLEN)) {
			readflags |= OKT_HAS_PLEN;
			slurp_read(fp, &w, 2);  [1]
			plen = bswapBE16(w);
		}


[...]
song->pan_separation = 64;
memset(song->orderlist + plen, ORDER_LAST, MAX_ORDERS - plen); [2]

Please let me know when you have fixed the vulnerability so that I can coordinate my disclosure with yours. For reference, here is a link to Semmle's vulnerability disclosure policy: https://lgtm.com/security#disclosure_policy

Thank you,

Nico Waisman
Semmle Security Research Team

@jangler
Copy link
Member

jangler commented Aug 1, 2019

Actually, that commit message is wrong – the MTM loader memset can't underflow, but a couple of callocs could have been passed negative nmemb args, which to my limited knowledge seems like invoking undefined behavior. Which is never good.

@jangler
Copy link
Member

jangler commented Aug 1, 2019

Nvm it's size_t so it would just underflow to something huge, like you said.

@nicowaisman
Copy link
Author

CVE assigned: CVE-2019-14523

@jangler
Copy link
Member

jangler commented Aug 5, 2019

Fix included in 20190805 release.

@jangler jangler closed this as completed Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants