Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol whitelist in ytdl_hook #5456

Closed
atx opened this issue Jan 27, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@atx
Copy link

commented Jan 27, 2018

The recent commits e6e6b0d, f8263e8 and ce42a96 fix and issue whereby mpv could be convinced to play a "non-safe" URL from a remote source.

Reproduction steps

An attacker convinces has the victim play an HTTP(S) URL.

$ mpv https://example.org/play.flv

The URL gets processed by the ytdl_hook script.

youtube-dl attempts to extract videos from the URL by contacting the HTTP server, which
responds with something like (text/html mime typed) :

<html>
  <head>
  </head>
  <body>
    <video>
      <source src="av://lavfi:ladspa=file=/home/user/Downloads/libevil.so"></source>
    </video>
  </body>
</html>

As youtube-dl does not perform any validation on the extracted URLs for <video> tags, the av://lavfi URL gets passed back to the hook script.

Note that there are likely many ways in which youtube-dl can return "bad" URLs.

The hook script then passes the extracted URL to mpv, which does not apply the usual safe-protocol only checks.

As shown in the example above, this URL can be, for instance, used to dlopen() arbitrary files on the filesystem using the ffmpeg lavfi ladspa plugin.

@atx atx closed this Jan 27, 2018

@atx

This comment has been minimized.

Copy link
Author

commented Jan 28, 2018

CVE-2018-6360 has been assigned to this issue

@jcowgill

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

@atalax 2a0f9fc seems to depend on 7eb3427 (which would therefore needs adding to your list), but I am wondering if either of these commits are relevant to the security bug at all?

@CounterPillow

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

I don't think 2a0f9fc is actually relevant to this CVE at all. The "whitelist" it mentions appears to be related to selecting a media stream from a manifest, and has no security whitelisting implications whatsoever.

@atx

This comment has been minimized.

Copy link
Author

commented Feb 1, 2018

Hmm, 2a0f9fc really doesn't seem relevant. Ping @wiiaboo

@wiiaboo

This comment has been minimized.

Copy link
Member

commented Feb 1, 2018

It's a bugfix to 7eb3427, yes, so not related to this issue.

@wiiaboo

This comment has been minimized.

Copy link
Member

commented Feb 1, 2018

Correct string of commits needed is e6e6b0d (has merge conflicts with v0.27.0 due to a few changes since) + f8263e8 + ce42a96.

andir added a commit to andir/nixpkgs that referenced this issue Feb 7, 2018

mpv: fix CVE-2018-6460
Upstream has fixed this in a series of commits ontop of 0.28.0. Debian
has backported the fixes to 0.27.0.

Upstream issue: mpv-player/mpv#5456
Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=888654#8

@andir andir referenced this issue Feb 7, 2018

Merged

mpv: fix CVE-2018-6460 #34692

4 of 8 tasks complete

andir added a commit to andir/nixpkgs that referenced this issue Feb 7, 2018

mpv: fix CVE-2018-6460
Upstream has fixed this in a series of commits ontop of 0.28.0. Debian
has backported the fixes to 0.27.0.

Upstream issue: mpv-player/mpv#5456
Debian bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=888654#8

(cherry picked from commit 95f4d6b)

@andir andir referenced this issue Feb 7, 2018

Merged

[17.09] mpv: fix CVE-2018-6460 #34693

3 of 8 tasks complete
@lfam

This comment has been minimized.

Copy link

commented Feb 8, 2018

I noticed that Debian pushed a fix for this bug but excluded the first commit of the four specified by @wiiaboo, 828bd29.

This can be seen here.

Can you clarify if that commit is necessary to fix the problem?

@kevmitch

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

I'll let @wiiaboo give the final word, but it looks like that commit just adds a property. It provides information about which demuxers are available. It is not used in the subsequent white-listing commits which fix the actual problem.

@wiiaboo

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

Yeah, nevermind. Being able to use the native dash demuxer is not necessary for the security fixes.

@jcowgill

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

FYI I screwed up the fix I applied to Debian's 0.27 and 0.23 and broke YouTube playlists so don't take the patch from there just yet. Hopefully it will be fixed within a day or so.

kevmitch added a commit that referenced this issue Feb 12, 2018

ytdl_hook: whitelist subtitle URLs as well
This was overlooked when doing the whitelisting for video and audio to
fix #5456.

kevmitch added a commit to kevmitch/mpv that referenced this issue Feb 13, 2018

ytdl_hook: whitelist subtitle URLs as well
This was overlooked when doing the whitelisting for video and audio to
fix mpv-player#5456.

kevmitch added a commit to kevmitch/mpv that referenced this issue Feb 13, 2018

ytdl_hook: whitelist subtitle URLs as well
This was overlooked when doing the whitelisting for video and audio to
fix mpv-player#5456.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.