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 on parsing MTM #201

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

Heap Overflow on parsing MTM #201

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

Comments

@nicowaisman
Copy link

Hey schismtracker team,

I would like to report a security vulnerability in schismtracker MTM parser (fmt/mtm.c).
There is a potential heap overflow on the way schismtracker parse MTM files, specifically while working with song patterns.
On fmt_mtm_load_song, it takes the number of patterns from the file in line 112
npat = slurp_getc(fp);
And later in code, there is a loop that fills the song patterns and patterns_sizethat is defined as:
#define MAX_PATTERNS 240
song_note_t *patterns[MAX_PATTERNS]; // Patterns
uint16_t pattern_size[MAX_PATTERNS]; // Pattern Lengths

The loop takes the value of npat directly, this is a one byte long field which maximun size can hold up to 255 entries.
/* patterns */
for (pat = 0; pat <= npat; pat++) {
song->patterns[pat] = csf_allocate_pattern(MAX(rows, 32));
song->pattern_size[pat] = song->pattern_alloc_size[pat] = 64;
tracknote = trackdata[n];

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

I just fixed a few issues with this loader yesterday, but I missed this one. It should be fixed now.

@jangler jangler added the bug label Aug 1, 2019
@nicowaisman
Copy link
Author

Sweet! I have one more for you

@sagamusix
Copy link
Member

sagamusix commented Aug 1, 2019

jangler, I would suggest to completely revise the workaround for #198. The fact that some of these variables can now contain negative values just extends some of those possible out of bound reads / writes into the negative direction. For example, just from skimming the MTM (even after commit c8986a8) loader it is possible to now set song->channels[-1].flags = CHN_MUTE; if nchn == -1 (i.e. there was an EOF when reading the number of channels).

I think that slurp_eof check should be moved directly after the reads, and the variables should be unsigned again.

@jangler
Copy link
Member

jangler commented Aug 1, 2019

The fact that some of these variables can now contain negative values just extends some of those possible out of bound reads / writes into the negative direction.

Well, no, because in the majority of cases those variables are only used as loop terminators, in which case -1 means nothing would happen, even if EOF weren't checked. In any case I didn't notice that nchan was used immediately and I agree that the check should be moved before that, and now that the check is there, most of the variables can be uint8_t.

@nicowaisman
Copy link
Author

Thanks guys.
Here is the CVE assigned: CVE-2019-14524

@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

3 participants