Bug#864195: libopenmpt: Security updates libopenmpt-0.2.7386-beta20.3-p7 available

[Jörn Heusipp]

Hi,

On 06/07/2017 11:45 AM, James Cowgill wrote:
> On 05/06/17 07:03, Jörn Heusipp wrote:

>> A couple of security-related fixes have been released upstream as
>> version 0.2.7386-beta20.3-p7. See
>> https://lib.openmpt.org/libopenmpt/md_announce-2017-06-02.html
>>
>> These most importantly fix a couple of possible crashes which can be
>> triggered by maliciously modified or malformed or truncated module
>> files as well as denial-of-service through hangs or excessive CPU
>> consumption which can also be triggered maliciously modfied or
>> malformed or truncated module files.
> 
> I've had a look at the patches now and this is what I think:
> 
> p1-division-by-zero-in-tempo-calculation.patch
> p2-infinite-loop-in-plugin-routing.patch
> p6-invalid-memory-read-when-applying-nnas-to-effect-plugins.patch
> 
> These three are clearly denial-of-service by malicious module file and
> should be fixed in stretch. However, I don't think the first two are
> "serious" because they're just denial-of-service bugs in a library
> almost exclusively used on end user machines (as opposed to eg remote
> code execution).

Agreed.

> I don't understand patch p6 well enough to say how
> serious it is (depends on where the invalid pointer being dereferenced
> comes from).

As far as I know, it is just a NULL pointer. Johannes did the analysis 
and might be able to elaborate (CCed).

> p3-excessive-cpu-consumption-on-malformed-files-dmf-mdl.patch
> p5-excessive-cpu-consumption-on-malformed-files-ams.patch
> 
> Are these actually security bugs? As long as the code finishes in a
> reasonable amount of time and produces the right results, then there's
> not much harm in leaving the code as it is.

Again, Johannes knows more about these.

> p4-theoretical-null-pointer-dereference-during-out-of-memory-while-error-handling.patch
> 
[...]
> 
> I also note that the C++ standard _requires_ std::exception::what to
> return a non-null value so a very intelligent compiler could
> legitimately remove the null check (I doubt GCC does this though).

Correct, I had realized that too late. I had originally triggered that 
when testing corner cases in error handling, but it can indeed only 
happen if other code does not behave correctly and returns nullptr from 
std::exception::what. openmpt::exception::what in 0.2.7386-beta20.3 
returns a static string as a fallback and cannot trigger it either.

> p7-race-condition-in-multi-threaded-use-it.patch
> 
> I also don't think this is a security bug (at least on Linux). Looking
> at the glibc sources, the internal tzdata state is protected by a mutex
> so the only risk here is that localtime will return some garbage time
> values. Since the string generated by this function is only going to be
> shown to the user, nothing that bad will happen in this case.

Yes, if glibc uses a mutex internally (I was not aware of that), the 
worst case outcome is indeed only a wrong string getting returned.

> Finally,
> it relies on a multithreaded client application loading 2 modules at the
> same time which seems unlikely to me.

Or any other concurrent call to localtime(). Which is also unlikely.

Jörn