Re: demux_mf: improve format string processing

Hi avih!

Seems your message is still stuck in the ml mod queue. I quote it
entirely in my reply below.

On 2021-04-28 08:58 +0000, avih wrote:
>
> 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

Ah sorry, I didn't notice while scanning through the code. Should be
fine then.


> 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

Playlists are not really safely supported in MPlayer since
forever I fear.

We have two options guarding playlist playback somewhat:

1. -playlist <filename>
2. -allow-dangerous-playlist-parsing

For better or worse it might be easier to use playlists in
the GUI.

To me it seems unfortunately never to be a good idea to use
playlists with MPlayer which one can't check.

Do you know how mpv secures playlist playback?


> 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 .

Agree. IMHO allowing complete printf format language is too much
for mf. Though I'm not really sure how much is useful.

We could probably just use av_get_frame_filename from libavformat,
which is quite limited, but probably would satisfy 99.99999% of
our users.


> 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

Nice write-up.


> Thanks again,
> avih

Thanks,
  Alexander