search

Artefact2 / libxm

A small XM (FastTracker II Extended Module) player library.
Do What The F*ck You Want To Public License
142 stars 14 forks source link

Buffer overrun in xm_create_context #6

Closed nukep closed 9 years ago

nukep commented 9 years ago

I'm writing a Rust wrapper for libxm, and noticed that there's no bounds for moddata in xm_create_context(). This is obviously bad if moddata is corrupted or invalid.

From what it appears, you seem to be aware of this issue already; it's listed in the README as:

Are there any plans to address this issue in the near future?

Artefact2 commented 9 years ago

Sure! Do you have ideas for a fix?

nukep commented 9 years ago

How about adding a length parameter to xm_create_context?

int xm_create_context(xm_context_t**, const char* moddata, size_t moddata_length, uint32_t rate);

Adding an additional parameter to an existing function would break the API. Buuutt... we could change it anyway if you're not concerned about backwards compatibility. Otherwise, add something like xm_create_context_2?

Basically, by providing a length for moddata, we can check that all read offsets are within the bounds of moddata and return an error if it's out of bounds. In theory, this should prevent buffer overruns and the segfaults that likely resulted from them.

If you give me the green light, I can attempt a fix and keep you posted. :)

Artefact2 commented 9 years ago

Sure, go ahead! But don't break the ABI. I suggest you make a xm_create_context_safe() function, perhaps. (And you can change xm_create_context() to call the safe variant with INT_MAX length.)

It will make the load code a LOT more tedious, though.

libxm was originally built for demos, where you'd need as small a library as possible, and where you'd usually only load a known (safe) module. That's why I didn't bother checking about length in the first place. However it seems to have grown beyond that as a general-purpose xm player, heh.

nukep commented 9 years ago

That's fine by me.

Artefact2 commented 9 years ago

Actually, you could probably do it quickly and reliably by adding a bounds check to the READ_U8 macro. I believe all our reads in moddata use this. You can either make it return zeros (making moddata seem like an infinitely big, zero-padded file), or just return an error (not very clean, but would work too).

nukep commented 9 years ago

Sure, that's easy. Do you want the macros to assume that moddata_length is in scope, or do you want it supplied as a parameter?

Artefact2 commented 9 years ago

I'm fine with either.