Re: demux_mf: improve format string processing

 
On Wednesday, April 28, 2021, 09:47:32 AM GMT+3, Alexander Strasser <[email protected]> wrote:

>> <JEEB> since this code originates in mplayer and I wasn't able to find
>> an up-to-date web-viewable SVN repo, this might be applicable to
>> mplayer as well
>>
>> https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabdae34e6
>
> That code is still in MPlayer.
>
> After some discussion with Reimar, we concluded that mostly everything
> is lost if the attacker can control the command line or more in general
> the arguments passed to the mplayer process.
>
> The sprintf is bad anyway, so I painfully came up with a patch to
> replace it. While doing that I promptly stumbled over another bug.
> Both patches sent here in this thread:
> http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2021-April/073973.html
>
> 1. libmpdemux/mf: Replace sprintf by mp_asprintf
> 2. libmpdemux/demux_mf: Don't crash if no paths were selected

Thanks.

CC Stefan Schiller - the original reporter.

As for patch 2/2, as far as I can tell mpv is unaffected. Right after
calling open_mf_pattern, if the number of files is less than 1 then
the mf result is rejected - before trying to probe it. See:
 https://github.com/mpv-player/mpv/blob/master/demux/demux_mf.c#L365

Re "everything is lost if the attacker can control the command line",
at least in mpv it could also be part of a playlist, which could be
hosted remotely, as in: mpv https://evil.com/evil.m3u

I don't know whether or not mplayer is affected by this case, but
if it is, then using mp_asprintf is likely not enough, because it's
still vulnerable to abuse of the format via non-matching arguments,
and even writing to somewhat arbitrary memory locations using %n .

If you're interested in a deeper analysis of this vulnerability and
possible attacks, Stefan Schiller (the reporter) has written this:
 https://devel0pment.de/?p=2217

Thanks again,
avih