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
