[FFmpeg-devel,RFC] configure: Disable unsafe demuxers by default

Submitted by Derek Buitenhuis on May 10, 2018, 3:44 p.m.

Details

Message ID 1525967099-28195-1-git-send-email-derek.buitenhuis@gmail.com
State New
Headers show

Commit Message

Derek Buitenhuis May 10, 2018, 3:44 p.m.
These demuxers have probes that mainly probe based on file extension,
and map to codec IDs that render text as video. The result is that
ffmpeg will, by default, happily render, for example, .txt files
as images. This is not exactly a good security practice, an only
makes it easier for potential attackers to gain the contents of
system files.

Disable building these by default.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
I've been hard disabling these at $dayjob for a long time, after some
"interesting" upload attempts, but it should probably be done for
everyone.

I'm not overly attached implementaion details like the option name
or whether it's done at build time ot runtime, but I think the concept
of "don't render arbitrary system text files" is an important one.
---
 Changelog     | 1 +
 configure     | 7 +++++++
 tests/fate.sh | 1 +
 3 files changed, 9 insertions(+)

Comments

Paul B Mahol May 10, 2018, 3:53 p.m.
On 5/10/18, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
>
> Disable building these by default.
>
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> I've been hard disabling these at $dayjob for a long time, after some
> "interesting" upload attempts, but it should probably be done for
> everyone.
>
> I'm not overly attached implementaion details like the option name
> or whether it's done at build time ot runtime, but I think the concept
> of "don't render arbitrary system text files" is an important one.
> ---
>  Changelog     | 1 +
>  configure     | 7 +++++++
>  tests/fate.sh | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/Changelog b/Changelog
> index d442ced..e3f8e83 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -6,6 +6,7 @@ version <next>:
>  - tmix filter
>  - amplify filter
>  - fftdnoiz filter
> +- unsafe demuxers that render text files now disabled by default
>

Against.
Rostislav Pehlivanov May 10, 2018, 3:55 p.m.
On 10 May 2018 at 16:44, Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
>
> Disable building these by default.
>
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> I've been hard disabling these at $dayjob for a long time, after some
> "interesting" upload attempts, but it should probably be done for
> everyone.
>
> I'm not overly attached implementaion details like the option name
> or whether it's done at build time ot runtime, but I think the concept
> of "don't render arbitrary system text files" is an important one.
> ---
>  Changelog     | 1 +
>  configure     | 7 +++++++
>  tests/fate.sh | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/Changelog b/Changelog
> index d442ced..e3f8e83 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -6,6 +6,7 @@ version <next>:
>  - tmix filter
>  - amplify filter
>  - fftdnoiz filter
> +- unsafe demuxers that render text files now disabled by default
>
>
>  version 4.0:
> diff --git a/configure b/configure
> index a1f13a7..2f2805e 100755
> --- a/configure
> +++ b/configure
> @@ -107,6 +107,7 @@ Configuration options:
>    --enable-small           optimize for size instead of speed
>    --disable-runtime-cpudetect disable detecting CPU capabilities at
> runtime (smaller binary)
>    --enable-gray            enable full grayscale support (slower color)
> +  --enable-unsafe-demuxers enable unsafe-by-default demuxers
>    --disable-swscale-alpha  disable alpha channel support in swscale
>    --disable-all            disable building components, libraries and
> programs
>    --disable-autodetect     disable automatically detected external
> libraries [no]
> @@ -1784,6 +1785,7 @@ FEATURE_LIST="
>      small
>      static
>      swscale_alpha
> +    unsafe_demuxers
>  "
>
>  LIBRARY_LIST="
> @@ -3100,6 +3102,7 @@ videotoolbox_encoder_deps="videotoolbox
> VTCompressionSessionPrepareToEncodeFrame
>
>  # demuxers / muxers
>  ac3_demuxer_select="ac3_parser"
> +adf_demuxer_deps="unsafe_demuxers"
>  aiff_muxer_select="iso_media"
>  asf_demuxer_select="riffdec"
>  asf_o_demuxer_select="riffdec"
> @@ -3107,6 +3110,7 @@ asf_muxer_select="riffenc"
>  asf_stream_muxer_select="asf_muxer"
>  avi_demuxer_select="iso_media riffdec exif"
>  avi_muxer_select="riffenc"
> +bintext_demuxer_deps="unsafe_demuxers"
>  caf_demuxer_select="iso_media riffdec"
>  caf_muxer_select="iso_media"
>  dash_muxer_select="mp4_muxer"
> @@ -3124,6 +3128,7 @@ flac_demuxer_select="flac_parser"
>  hds_muxer_select="flv_muxer"
>  hls_muxer_select="mpegts_muxer"
>  hls_muxer_suggest="gcrypt openssl"
> +idf_demuxer_deps="unsafe_demuxers"
>  image2_alias_pix_demuxer_select="image2_demuxer"
>  image2_brender_pix_demuxer_select="image2_demuxer"
>  ipod_muxer_select="mov_muxer"
> @@ -3167,6 +3172,7 @@ swf_demuxer_suggest="zlib"
>  tak_demuxer_select="tak_parser"
>  tg2_muxer_select="mov_muxer"
>  tgp_muxer_select="mov_muxer"
> +tty_demuxer_deps="unsafe_demuxers"
>  vobsub_demuxer_select="mpegps_demuxer"
>  w64_demuxer_select="wav_demuxer"
>  w64_muxer_select="wav_muxer"
> @@ -3176,6 +3182,7 @@ webm_muxer_select="iso_media riffenc"
>  webm_dash_manifest_demuxer_select="matroska_demuxer"
>  wtv_demuxer_select="mpegts_demuxer riffdec"
>  wtv_muxer_select="mpegts_muxer riffenc"
> +xbin_demuxer_deps="unsafe_demuxers"
>  xmv_demuxer_select="riffdec"
>  xwma_demuxer_select="riffdec"
>
> diff --git a/tests/fate.sh b/tests/fate.sh
> index 0edee7f..6a99d66 100755
> --- a/tests/fate.sh
> +++ b/tests/fate.sh
> @@ -49,6 +49,7 @@ configure()(
>          --enable-gpl                                                    \
>          --enable-memory-poisoning                                       \
>          --enable-avresample                                             \
> +        --enable-unsafe-demuxers                                        \
>          ${ignore_tests:+--ignore-tests="$ignore_tests"}                 \
>          ${arch:+--arch=$arch}                                           \
>          ${cpu:+--cpu="$cpu"}                                            \
> --
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Could you send a patch to disable the decoders as well?
Looks good otherwise.
Jan Ekström May 10, 2018, 4:05 p.m.
On Thu, May 10, 2018 at 6:53 PM, Paul B Mahol <onemda@gmail.com> wrote:
>
> Against.

Hi,

Thank you for your review.

I would recommend you put a bit more effort into explaining which
parts you are opposed to. Think of yourself being on the receiving end
of such comments, how you would like to just get a flat out refusal
without any further details. You do not give the other side any chance
of doing anything, because you have not noted what is wrong with it.

Best regards,
Jan
Paul B Mahol May 10, 2018, 4:11 p.m.
On 5/10/18, Jan Ekstroem <jeebjp@gmail.com> wrote:
> On Thu, May 10, 2018 at 6:53 PM, Paul B Mahol <onemda@gmail.com> wrote:
>>
>> Against.
>
> Hi,
>
> Thank you for your review.
>
> I would recommend you put a bit more effort into explaining which
> parts you are opposed to. Think of yourself being on the receiving end
> of such comments, how you would like to just get a flat out refusal
> without any further details. You do not give the other side any chance
> of doing anything, because you have not noted what is wrong with it.

I do not like it being disabled by default.

There are options to disable compilation of such modules already.
Derek Buitenhuis May 10, 2018, 4:15 p.m.
On Thu, May 10, 2018 at 4:55 PM, Rostislav Pehlivanov
<atomnuker@gmail.com> wrote:
> Could you send a patch to disable the decoders as well?
> Looks good otherwise.

Yeah, I thought about doing that too. I can add that, though the option
will have to be renamed to something else.

- Derek
Derek Buitenhuis May 10, 2018, 4:18 p.m.
On Thu, May 10, 2018 at 5:11 PM, Paul B Mahol <onemda@gmail.com> wrote:
> I do not like it being disabled by default.

With all due respect, this is not a valid technical argument, in any respect.

> There are options to disable compilation of such modules already.

We should be defaulting to 'safe/sane by default'. Insecure and insane
defaults are
the cause of a huge number of issues across the web. The onus should not be on
the user to disable/fix all the bad defaults. That's how you become PHP.

- Derek

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Marton Balint May 10, 2018, 5:47 p.m.
On Thu, 10 May 2018, Derek Buitenhuis wrote:

> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.

Maybe it is better if we simply get rid of the "probing" part, so the user 
would have to explicitly specify the demuxer to use them.

Regards,
Marton
Gyan May 10, 2018, 5:52 p.m.
On 5/10/2018 11:17 PM, Marton Balint wrote:
> 
> Maybe it is better if we simply get rid of the "probing" part, so the 
> user would have to explicitly specify the demuxer to use them.

+1

Maybe shift such probes to inside a block and add a new generic lavf 
option to set whether such probes are enabled. Default would to require 
user-set demuxer.

Regards,
Gyan
Derek Buitenhuis May 10, 2018, 7:26 p.m.
On Thu, May 10, 2018 at 6:52 PM, Gyan Doshi <gyandoshi@gmail.com> wrote:
> On 5/10/2018 11:17 PM, Marton Balint wrote:
>> Maybe it is better if we simply get rid of the "probing" part, so the user
>> would have to explicitly specify the demuxer to use them.
>
> +1
>
> Maybe shift such probes to inside a block and add a new generic lavf option
> to set whether such probes are enabled. Default would to require user-set
> demuxer.

I don't object to either approach.

- Derek
Michael Niedermayer May 10, 2018, 8:46 p.m.
On Thu, May 10, 2018 at 04:44:59PM +0100, Derek Buitenhuis wrote:
> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
> 
> Disable building these by default.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> I've been hard disabling these at $dayjob for a long time, after some
> "interesting" upload attempts, but it should probably be done for
> everyone.
> 
> I'm not overly attached implementaion details like the option name
> or whether it's done at build time ot runtime, but I think the concept
> of "don't render arbitrary system text files" is an important one.
> ---
>  Changelog     | 1 +
>  configure     | 7 +++++++
>  tests/fate.sh | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Changelog b/Changelog
> index d442ced..e3f8e83 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -6,6 +6,7 @@ version <next>:
>  - tmix filter
>  - amplify filter
>  - fftdnoiz filter
> +- unsafe demuxers that render text files now disabled by default

The problem and the suggested solution seem only somewhat overlapping

please correct me if iam wrong, theres quite a bit iam guessing here
IIUC the problem is that in your usecase 
1. ffmpeg has access to sensitive files
2. one of these files can be opened by an attacker with ffmpeg
2b. This involves the file being probed as a supported format
3. The file is then demuxed, decoded, encoded, muxed
4. the result is given back to the attacker

This patchset removes one of the demuxers involved in the attack

The first problem of this patchset is that it does not provide any
evidence of how the other demuxers probe functions can trigger on
random text files.
for example idf_probe() requires, the first 12 bytes of the file  to
match exactly and some of these are not text. So a attack which depends
on the probing of sensitive text data to succeed should not work with it

The second problem i see is that the patch disables the demuxers, while
disabling the probe functions affected should have the same effect
security wise but is a smaller step in respect to disabling.

The third problem i see is that really once you read sensitive data
and pass something back to the user you already lost.
A text demuxer and decoder makes it easier and it makes it much easier
to demonstrate the attack in a flashy way. But having the probe code
access the sensitive data even if none succeeds already makes quite
a bit of internal state (caches, branch prediction, deallocated memory, ...)
be contaminated with sensitive information. Its hard to ensure that
none of this can be recovered by the attacker.

I think first ffmpeg should not be able to access sensitive data.
Then none of our probe functions should succeed on the average
sensitive text file. If one does, we should look into making it not
detect that.

Also it may seem counter-intuitive but adding a probe function which
is designed to succeed on sensitive text files may be more reliable
to stop their detection than to disable probe functions.
The probe system is basically doing differential probing. That is
the one of 2 that has the higher score wins.
So it may be more effective to add a function that that returns a
high score intentionally for a null demuxer than to try to stop
every function that returns more than 0
if such probe code is added, it also could maybe warn the admin about a
potential misconfiguration that allows probing to reach to sensitive
data.


[...]
Carl Eugen Hoyos May 10, 2018, 8:57 p.m.
2018-05-10 17:44 GMT+02:00, Derek Buitenhuis <derek.buitenhuis@gmail.com>:
> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
>
> Disable building these by default.
>
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> I've been hard disabling these at $dayjob for a long time, after some
> "interesting" upload attempts, but it should probably be done for
> everyone.
>
> I'm not overly attached implementaion details like the option name
> or whether it's done at build time ot runtime, but I think the concept
> of "don't render arbitrary system text files" is an important one.

Disabling demuxers by default does not seem to be a good idea to me.

Carl Eugen
James Darnley May 10, 2018, 9:11 p.m.
On 2018-05-10 17:44, Derek Buitenhuis wrote:
> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
> 
> Disable building these by default.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> I've been hard disabling these at $dayjob for a long time, after some
> "interesting" upload attempts, but it should probably be done for
> everyone.
> 
> I'm not overly attached implementaion details like the option name
> or whether it's done at build time ot runtime, but I think the concept
> of "don't render arbitrary system text files" is an important one.
> ---

You web people already have options for the various annoying whitelists.
 Is this not covered by one of them?
wm4 May 10, 2018, 10:17 p.m.
On Thu, 10 May 2018 16:44:59 +0100
Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:

> These demuxers have probes that mainly probe based on file extension,
> and map to codec IDs that render text as video. The result is that
> ffmpeg will, by default, happily render, for example, .txt files
> as images. This is not exactly a good security practice, an only
> makes it easier for potential attackers to gain the contents of
> system files.
> 
> Disable building these by default.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---

+1

You should send a patch that disables all those useless game demuxers
too. They only cause security issues and bloated library sizes.
Paul B Mahol May 10, 2018, 10:27 p.m.
On 5/11/18, wm4 <nfxjfg@googlemail.com> wrote:
> On Thu, 10 May 2018 16:44:59 +0100
> Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
>
>> These demuxers have probes that mainly probe based on file extension,
>> and map to codec IDs that render text as video. The result is that
>> ffmpeg will, by default, happily render, for example, .txt files
>> as images. This is not exactly a good security practice, an only
>> makes it easier for potential attackers to gain the contents of
>> system files.
>>
>> Disable building these by default.
>>
>> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
>> ---
>
> +1
>
> You should send a patch that disables all those useless game demuxers
> too. They only cause security issues and bloated library sizes.

Against.
Derek Buitenhuis May 10, 2018, 10:57 p.m.
> Disabling demuxers by default does not seem to be a good idea to me.

So rendering arbitrary text files by default seems like a good idea in
comparsion?

- Derek
Derek Buitenhuis May 10, 2018, 11 p.m.
> You web people already have options for the various annoying whitelists.
>  Is this not covered by one of them?

As noted in my other reply to Paul, I find it a poor choice to shunt
the responsibility
of good/secure defaults to the user.

As a side note, derisively referring to me as "you web people" isn't
particularily charming.

- Derek
Paul B Mahol May 10, 2018, 11:04 p.m.
On 5/11/18, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
>> Disabling demuxers by default does not seem to be a good idea to me.
>
> So rendering arbitrary text files by default seems like a good idea in
> comparsion?

That is useful feature.
Derek Buitenhuis May 10, 2018, 11:14 p.m.
> please correct me if iam wrong, theres quite a bit iam guessing here
> IIUC the problem is that in your usecase
> 1. ffmpeg has access to sensitive files
> 2. one of these files can be opened by an attacker with ffmpeg
> 2b. This involves the file being probed as a supported format

It is "probed" as file extension mostly. .txt is one of these
extensions, which 99.999999999999999%
of the time, is not a multimedia file, for example.

> 3. The file is then demuxed, decoded, encoded, muxed
> 4. the result is given back to the attacker
>
> This patchset removes one of the demuxers involved in the attack

Not removed. Disabled. As discussed in other replies, making it manual
also seems
fine to me. Rendering arbitrary text files as images (as in, rendering
the text using
a built in font, to an image with that text in it) isn't exactly a
great and secure default
behavior, especially for the world's most commont text file extension...

> The first problem of this patchset is that it does not provide any
> evidence of how the other demuxers probe functions can trigger on
> random text files.

ffmpeg -i something.txt a.png

But really, it is not necssarily an attack on its own (it could be),
but it makes any
other attack vastly easier to exploit. For example, that HLS stuff
avformat had fixed
last year could have pointed at various .txt files. I don't really
understand why the
concept of "rendering arbitrary text files as images" is not obviously bad?

> for example idf_probe() requires, the first 12 bytes of the file  to
> match exactly and some of these are not text. So a attack which depends
> on the probing of sensitive text data to succeed should not work with it

it is not specifci to probing. It's choosing e.g. the ansi decoder based off the
.txt file extension, for example. Or the bintext decoder based off the
.bin extension.

> The second problem i see is that the patch disables the demuxers, while
> disabling the probe functions affected should have the same effect
> security wise but is a smaller step in respect to disabling.

As noted in my reply to Marton and Gyan, I'm fine with this approach,
as the implications
are the same.

> The third problem i see is that really once you read sensitive data
> and pass something back to the user you already lost.

[...]

> A text demuxer and decoder makes it easier and it makes it much easier
> to demonstrate the attack in a flashy way. But having the probe code
> access the sensitive data even if none succeeds already makes quite
> a bit of internal state (caches, branch prediction, deallocated memory, ...)
> be contaminated with sensitive information. Its hard to ensure that
> none of this can be recovered by the attacker.

Ah the classic "well it can technically be beaten, you may as well make it
as easy as possible" argument that people use to argue against things
like ASLR... :|

The point is to minimize the risk where possible, with good defaults for *all*
users. It's not a great thing to just point at users and go "well you
should have
done a better job containerizing/sandboxing/etc." IMO.

> I think first ffmpeg should not be able to access sensitive data.
> Then none of our probe functions should succeed on the average
> sensitive text file. If one does, we should look into making it not
> detect that.

The succeed because libavformat loves probing based on file extenions.

> Also it may seem counter-intuitive but adding a probe function which
> is designed to succeed on sensitive text files may be more reliable
> to stop their detection than to disable probe functions.

This is just adding more heuristics to probing, though

> The probe system is basically doing differential probing. That is
> the one of 2 that has the higher score wins.
> So it may be more effective to add a function that that returns a
> high score intentionally for a null demuxer than to try to stop
> every function that returns more than 0

Wouldn't this break a lot of currently "working" detection of
bad/broken files that
users seem to rely on? (I don't think they should rely on it, but
that's just my opinion.)

> if such probe code is added, it also could maybe warn the admin about a
> potential misconfiguration that allows probing to reach to sensitive
> data.

[...]

- Derek
Rostislav Pehlivanov May 10, 2018, 11:21 p.m.
On 10 May 2018 at 23:27, Paul B Mahol <onemda@gmail.com> wrote:

> On 5/11/18, wm4 <nfxjfg@googlemail.com> wrote:
> > On Thu, 10 May 2018 16:44:59 +0100
> > Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> >
> >> These demuxers have probes that mainly probe based on file extension,
> >> and map to codec IDs that render text as video. The result is that
> >> ffmpeg will, by default, happily render, for example, .txt files
> >> as images. This is not exactly a good security practice, an only
> >> makes it easier for potential attackers to gain the contents of
> >> system files.
> >>
> >> Disable building these by default.
> >>
> >> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> >> ---
> >
> > +1
> >
> > You should send a patch that disables all those useless game demuxers
> > too. They only cause security issues and bloated library sizes.
>
> Against.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

I agree with Paul, game demuxers are useful, don't bloat much and can be
fixed.
Rostislav Pehlivanov May 10, 2018, 11:23 p.m.
On 11 May 2018 at 00:04, Paul B Mahol <onemda@gmail.com> wrote:

> On 5/11/18, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> >> Disabling demuxers by default does not seem to be a good idea to me.
> >
> > So rendering arbitrary text files by default seems like a good idea in
> > comparsion?
>
> That is useful feature.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

No, it isn't. Its not useful at all, you have no control over how they're
rendered and they're always rendered using CGA fonts from the 80's. They're
also very easily misprobed.
Paul B Mahol May 10, 2018, 11:26 p.m.
On 5/11/18, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
>> please correct me if iam wrong, theres quite a bit iam guessing here
>> IIUC the problem is that in your usecase
>> 1. ffmpeg has access to sensitive files
>> 2. one of these files can be opened by an attacker with ffmpeg
>> 2b. This involves the file being probed as a supported format
>
> It is "probed" as file extension mostly. .txt is one of these
> extensions, which 99.999999999999999%
> of the time, is not a multimedia file, for example.
>
>> 3. The file is then demuxed, decoded, encoded, muxed
>> 4. the result is given back to the attacker
>>
>> This patchset removes one of the demuxers involved in the attack
>
> Not removed. Disabled. As discussed in other replies, making it manual
> also seems
> fine to me. Rendering arbitrary text files as images (as in, rendering
> the text using
> a built in font, to an image with that text in it) isn't exactly a
> great and secure default
> behavior, especially for the world's most commont text file extension...
>
>> The first problem of this patchset is that it does not provide any
>> evidence of how the other demuxers probe functions can trigger on
>> random text files.
>
> ffmpeg -i something.txt a.png
>
> But really, it is not necssarily an attack on its own (it could be),
> but it makes any
> other attack vastly easier to exploit. For example, that HLS stuff
> avformat had fixed
> last year could have pointed at various .txt files. I don't really
> understand why the
> concept of "rendering arbitrary text files as images" is not obviously bad?
>
>> for example idf_probe() requires, the first 12 bytes of the file  to
>> match exactly and some of these are not text. So a attack which depends
>> on the probing of sensitive text data to succeed should not work with it
>
> it is not specifci to probing. It's choosing e.g. the ansi decoder based off
> the
> .txt file extension, for example. Or the bintext decoder based off the
> .bin extension.
>
>> The second problem i see is that the patch disables the demuxers, while
>> disabling the probe functions affected should have the same effect
>> security wise but is a smaller step in respect to disabling.
>
> As noted in my reply to Marton and Gyan, I'm fine with this approach,
> as the implications
> are the same.
>
>> The third problem i see is that really once you read sensitive data
>> and pass something back to the user you already lost.
>
> [...]
>
>> A text demuxer and decoder makes it easier and it makes it much easier
>> to demonstrate the attack in a flashy way. But having the probe code
>> access the sensitive data even if none succeeds already makes quite
>> a bit of internal state (caches, branch prediction, deallocated memory,
>> ...)
>> be contaminated with sensitive information. Its hard to ensure that
>> none of this can be recovered by the attacker.
>
> Ah the classic "well it can technically be beaten, you may as well make it
> as easy as possible" argument that people use to argue against things
> like ASLR... :|
>
> The point is to minimize the risk where possible, with good defaults for
> *all*
> users. It's not a great thing to just point at users and go "well you
> should have
> done a better job containerizing/sandboxing/etc." IMO.
>
>> I think first ffmpeg should not be able to access sensitive data.
>> Then none of our probe functions should succeed on the average
>> sensitive text file. If one does, we should look into making it not
>> detect that.
>
> The succeed because libavformat loves probing based on file extenions.
>
>> Also it may seem counter-intuitive but adding a probe function which
>> is designed to succeed on sensitive text files may be more reliable
>> to stop their detection than to disable probe functions.
>
> This is just adding more heuristics to probing, though
>
>> The probe system is basically doing differential probing. That is
>> the one of 2 that has the higher score wins.
>> So it may be more effective to add a function that that returns a
>> high score intentionally for a null demuxer than to try to stop
>> every function that returns more than 0
>
> Wouldn't this break a lot of currently "working" detection of
> bad/broken files that
> users seem to rely on? (I don't think they should rely on it, but
> that's just my opinion.)
>
>> if such probe code is added, it also could maybe warn the admin about a
>> potential misconfiguration that allows probing to reach to sensitive
>> data.
>
> [...]

If you think you are fixing security issue you are very wrong.
Derek Buitenhuis May 10, 2018, 11:31 p.m.
> If you think you are fixing security issue you are very wrong.

You've nailed that "saying what you think" part of communication, but
need to word a little
on the all important "saying why you think that" part. Keep
practicing, you can do it. I
believe in you.

- Derek
Paul B Mahol May 10, 2018, 11:35 p.m.
On 5/11/18, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
>> If you think you are fixing security issue you are very wrong.
>
> You've nailed that "saying what you think" part of communication, but
> need to word a little
> on the all important "saying why you think that" part. Keep
> practicing, you can do it. I
> believe in you.

I do not have time to explain security basics.
Get a decent book and read it from a start to an end.
wm4 May 10, 2018, 11:36 p.m.
On Fri, 11 May 2018 00:21:37 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 10 May 2018 at 23:27, Paul B Mahol <onemda@gmail.com> wrote:
> 
> > On 5/11/18, wm4 <nfxjfg@googlemail.com> wrote:  
> > > On Thu, 10 May 2018 16:44:59 +0100
> > > Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> > >  
> > >> These demuxers have probes that mainly probe based on file extension,
> > >> and map to codec IDs that render text as video. The result is that
> > >> ffmpeg will, by default, happily render, for example, .txt files
> > >> as images. This is not exactly a good security practice, an only
> > >> makes it easier for potential attackers to gain the contents of
> > >> system files.
> > >>
> > >> Disable building these by default.
> > >>
> > >> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> > >> ---  
> > >
> > > +1
> > >
> > > You should send a patch that disables all those useless game demuxers
> > > too. They only cause security issues and bloated library sizes.  
> >
> > Against.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >  
> 
> I agree with Paul, game demuxers are useful, don't bloat much and can be
> fixed.

Experience shows that it's always the obscure features which cause
security issues. Regarding the bloat: these small things add up a lot,
and suddenly you have hundreds of demuxers. It's hard to filter them
out manually, and why make each user do that? Many of these game formats
in particular probably have something like under a dozen files in the
universe that exist at all (such as the files included in a particular
game release).
Derek Buitenhuis May 10, 2018, 11:41 p.m.
On Fri, May 11, 2018 at 12:35 AM, Paul B Mahol <onemda@gmail.com> wrote:
> I do not have time to explain security basics.
> Get a decent book and read it from a start to an end.

Speaking frankly for a second: Why do people put up with this sort of
crud on this
mailing list?

Insult-laden fact-less response. It's insane. It's like I'm flashing
back to ffmpeg-devel
in 2006.

- Derek
Derek Buitenhuis May 10, 2018, 11:42 p.m.
On Fri, May 11, 2018 at 12:41 AM, Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
> Speaking frankly for a second: Why do people put up with this sort of
> crud on this
> mailing list?

Unrelatedly, sorry for the broken linebreaks. Bad MUA...

- Derek
wm4 May 10, 2018, 11:44 p.m.
On Fri, 11 May 2018 00:41:20 +0100
Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:

> On Fri, May 11, 2018 at 12:35 AM, Paul B Mahol <onemda@gmail.com> wrote:
> > I do not have time to explain security basics.
> > Get a decent book and read it from a start to an end.  
> 
> Speaking frankly for a second: Why do people put up with this sort of
> crud on this
> mailing list?
> 
> Insult-laden fact-less response. It's insane. It's like I'm flashing
> back to ffmpeg-devel
> in 2006.

Because there are no consequences, no leadership, no conflict
arbitration. It's a joke of a project.
Derek Buitenhuis May 10, 2018, 11:44 p.m.
On Fri, May 11, 2018 at 12:36 AM, wm4 <nfxjfg@googlemail.com> wrote:
> Experience shows that it's always the obscure features which cause
> security issues. Regarding the bloat: these small things add up a lot,
> and suddenly you have hundreds of demuxers. It's hard to filter them
> out manually, and why make each user do that? Many of these game formats
> in particular probably have something like under a dozen files in the
> universe that exist at all (such as the files included in a particular
> game release).

I think this is a tangental battle for another day. Not really
related to the patch/RFC at hand.

- Derek
James Darnley May 10, 2018, 11:49 p.m.
On 2018-05-11 00:57, Derek Buitenhuis wrote:
>> Disabling demuxers by default does not seem to be a good idea to me.
> 
> So rendering arbitrary text files by default seems like a good idea in
> comparsion?

I want to argue some more so here you go: it isn't "by default".

It gets rendered because you asked for it to be rendered.  You asked for
/etc/passwd to be rendered so ffmpeg did that.  It produced a nice 4K
video of the file with all your secrets clearly legible in it.  Why do
you care?  Surely nobody will see it.  Surely you're not going to upload
this file to the public Internet.

I don't care that you do encode any random file that someone uploads to
you.  I don't care that you do put the results on the public net.  I do
care a little that ffmpeg understands playlist files but not in the same
way you do.  I do care a little that ffmpeg does so much magic for you
but not in the same way you do.

I haven't tried to stand in the way of other bad changes to ffmpeg (like
the fact that the flac muxer will now mux video streams) and I won't try
to stand in the way of this one.
Rostislav Pehlivanov May 10, 2018, 11:53 p.m.
On 11 May 2018 at 00:44, wm4 <nfxjfg@googlemail.com> wrote:

> On Fri, 11 May 2018 00:41:20 +0100
> Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
>
> > On Fri, May 11, 2018 at 12:35 AM, Paul B Mahol <onemda@gmail.com> wrote:
> > > I do not have time to explain security basics.
> > > Get a decent book and read it from a start to an end.
> >
> > Speaking frankly for a second: Why do people put up with this sort of
> > crud on this
> > mailing list?
> >
> > Insult-laden fact-less response. It's insane. It's like I'm flashing
> > back to ffmpeg-devel
> > in 2006.
>
> Because there are no consequences


There are consequeces, the consequences being broken design and/or features


no leadership


Just because there are many people determining what happens doesn't mean no
one is leading.


no conflict arbitration.
>

There is conflict arbitration, we're doing it right now in fact.
wm4 May 11, 2018, 12:02 a.m.
On Fri, 11 May 2018 00:53:16 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 11 May 2018 at 00:44, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Fri, 11 May 2018 00:41:20 +0100
> > Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote:
> >  
> > > On Fri, May 11, 2018 at 12:35 AM, Paul B Mahol <onemda@gmail.com> wrote:  
> > > > I do not have time to explain security basics.
> > > > Get a decent book and read it from a start to an end.  
> > >
> > > Speaking frankly for a second: Why do people put up with this sort of
> > > crud on this
> > > mailing list?
> > >
> > > Insult-laden fact-less response. It's insane. It's like I'm flashing
> > > back to ffmpeg-devel
> > > in 2006.  
> >
> > Because there are no consequences  
> 
> 
> There are consequeces, the consequences being broken design and/or features
> 
> 
> no leadership
> 
> 
> Just because there are many people determining what happens doesn't mean no
> one is leading.
> 
> 
> no conflict arbitration.
> >  
> 
> There is conflict arbitration, we're doing it right now in fact.

Yeah, we can just flame each other as hard as we can every time, until
someone gives up out of boredom or disgust. This is why ffmpeg is one
of the worst open source projects around.
Derek Buitenhuis May 11, 2018, 12:18 a.m.
On Fri, May 11, 2018 at 12:49 AM, James Darnley <james.darnley@gmail.com> wrote:
> I want to argue some more so here you go: it isn't "by default".

Strange definition of default, but OK.

> It gets rendered because you asked for it to be rendered.  You asked for
> /etc/passwd to be rendered so ffmpeg did that.  It produced a nice 4K
> video of the file with all your secrets clearly legible in it.  Why do
> you care?  Surely nobody will see it.  Surely you're not going to upload
> this file to the public Internet.

I think you might lack some imagination on how people use the API,
nd how people use the FFmpeg cli. This for example, pre-HLS CVE patches,
let someone upload a m3u8 that pointed to e.g. /something/password.txt
and have it rendered. It's not a all-or-nothing sort of thing.

> I don't care that you do encode any random file that someone uploads to
> you.  I don't care that you do put the results on the public net.  I do
> care a little that ffmpeg understands playlist files but not in the same
> way you do.  I do care a little that ffmpeg does so much magic for you
> but not in the same way you do.

So basically you don't care at all about making ffmpeg less exploitable by
various users. Great.

Gotta say, this is one mighty poor attitude towards security. But OK.

> I haven't tried to stand in the way of other bad changes to ffmpeg (like
> the fact that the flac muxer will now mux video streams) and I won't try
> to stand in the way of this one.

Fine. Patch dropped. Have a good day/night. I don't have the mental
energy to deal with this endless infighting, feces-flinging, and insane
approach to safety/sanity. I'm not going to spend my spare time
smashing my head against a wall and being insulted.

This will be my last response in the thread.

- Derek
wm4 May 11, 2018, 12:31 a.m.
On Fri, 11 May 2018 01:49:58 +0200
James Darnley <james.darnley@gmail.com> wrote:

> ...

Security in ffmpeg sure is weird. On one hand we get all kinds of crazy
stuff that's supposed to mitigate... something (like whitelists), on
the other hand we get this.

> I haven't tried to stand in the way of other bad changes to ffmpeg (like
> the fact that the flac muxer will now mux video streams) and I won't try
> to stand in the way of this one.

That sounds insane, but it's probably about cover art. Only ffmpeg is
insane enough to treat cover art as single-frame video stream. (And
as an API user I've really tried hard to make this work, but it remains
a painful special case.)

So, not sure why you claim it's a "bad change" to add support for that
in the flac muxer. It merely follows the ffmpeg API.

Anyway, I guess D. B. is right and this thread is a fucking waste of
time, so goodbye.
Michael Niedermayer May 12, 2018, 12:23 p.m.
On Fri, May 11, 2018 at 12:14:47AM +0100, Derek Buitenhuis wrote:
> > please correct me if iam wrong, theres quite a bit iam guessing here
> > IIUC the problem is that in your usecase
> > 1. ffmpeg has access to sensitive files
> > 2. one of these files can be opened by an attacker with ffmpeg
> > 2b. This involves the file being probed as a supported format
> 
> It is "probed" as file extension mostly. .txt is one of these
> extensions, which 99.999999999999999%
> of the time, is not a multimedia file, for example.
> 
> > 3. The file is then demuxed, decoded, encoded, muxed
> > 4. the result is given back to the attacker
> >
> > This patchset removes one of the demuxers involved in the attack
> 
> Not removed. Disabled. As discussed in other replies, making it manual
> also seems
> fine to me. Rendering arbitrary text files as images (as in, rendering
> the text using
> a built in font, to an image with that text in it) isn't exactly a
> great and secure default
> behavior, especially for the world's most commont text file extension...

Do you think a document converter should by default not allow to open
multimedia files and embed them into documents ?
(not saying i think it should or should not)

What you suggest is similar, a multimedia converter by default
(not) supporting reading documents.

You later in the mail compare this to ASLR, i dont think this is at
the same level. ASLR provides broad protection, disabling
the .txt/.bin demuxers (which btw does not prevent other demuxers to open
.txt/.bin files) does not seem to be a comparable feature.

Of course every time you disable a feature the attack surface decreases,
in that sense it makes sense to disable all unneeded features whenever
security matters.
Iam not sure we know what is "unneeded" to the end user.

also where are security relevant *.txt files ? i looked at my /etc
and there where none.


> 
> > The first problem of this patchset is that it does not provide any
> > evidence of how the other demuxers probe functions can trigger on
> > random text files.
> 
> ffmpeg -i something.txt a.png
> 
> But really, it is not necssarily an attack on its own (it could be),
> but it makes any
> other attack vastly easier to exploit. For example, that HLS stuff
> avformat had fixed
> last year could have pointed at various .txt files. I don't really
> understand why the
> concept of "rendering arbitrary text files as images" is not obviously bad?

as long as we support rawvideo input by default, disabling text input does
not gain much security. rawvideo is multimedia, noone suggested to disable
it.
The probing / extension based detection, yes changes there should be able
to gain some security and we should possibly tighten this down where theres
an agreement.

as i was curious i looked at my /etc/ and /etc/*/ and from
3580 files there are 929 matches for 'Probing .* score:'

      1 mpeg
      3 svg_pipe
      9 png_pipe
     10 bin
     12 image2
     25 mpegts
     39 lrc
    830 mp3
    
and for 'probed with size|detected only with low score of'

      3 svg_pipe
      9 png_pipe
     24 mpegts
     29 lrc
    161 mp3
    
from the stuff above, the svg and png are correct detected
  

From this it appears to me that file extension while its an issue is not
the only and based on this, not the biggest

[...]

Patch hide | download patch | download mbox

diff --git a/Changelog b/Changelog
index d442ced..e3f8e83 100644
--- a/Changelog
+++ b/Changelog
@@ -6,6 +6,7 @@  version <next>:
 - tmix filter
 - amplify filter
 - fftdnoiz filter
+- unsafe demuxers that render text files now disabled by default
 
 
 version 4.0:
diff --git a/configure b/configure
index a1f13a7..2f2805e 100755
--- a/configure
+++ b/configure
@@ -107,6 +107,7 @@  Configuration options:
   --enable-small           optimize for size instead of speed
   --disable-runtime-cpudetect disable detecting CPU capabilities at runtime (smaller binary)
   --enable-gray            enable full grayscale support (slower color)
+  --enable-unsafe-demuxers enable unsafe-by-default demuxers
   --disable-swscale-alpha  disable alpha channel support in swscale
   --disable-all            disable building components, libraries and programs
   --disable-autodetect     disable automatically detected external libraries [no]
@@ -1784,6 +1785,7 @@  FEATURE_LIST="
     small
     static
     swscale_alpha
+    unsafe_demuxers
 "
 
 LIBRARY_LIST="
@@ -3100,6 +3102,7 @@  videotoolbox_encoder_deps="videotoolbox VTCompressionSessionPrepareToEncodeFrame
 
 # demuxers / muxers
 ac3_demuxer_select="ac3_parser"
+adf_demuxer_deps="unsafe_demuxers"
 aiff_muxer_select="iso_media"
 asf_demuxer_select="riffdec"
 asf_o_demuxer_select="riffdec"
@@ -3107,6 +3110,7 @@  asf_muxer_select="riffenc"
 asf_stream_muxer_select="asf_muxer"
 avi_demuxer_select="iso_media riffdec exif"
 avi_muxer_select="riffenc"
+bintext_demuxer_deps="unsafe_demuxers"
 caf_demuxer_select="iso_media riffdec"
 caf_muxer_select="iso_media"
 dash_muxer_select="mp4_muxer"
@@ -3124,6 +3128,7 @@  flac_demuxer_select="flac_parser"
 hds_muxer_select="flv_muxer"
 hls_muxer_select="mpegts_muxer"
 hls_muxer_suggest="gcrypt openssl"
+idf_demuxer_deps="unsafe_demuxers"
 image2_alias_pix_demuxer_select="image2_demuxer"
 image2_brender_pix_demuxer_select="image2_demuxer"
 ipod_muxer_select="mov_muxer"
@@ -3167,6 +3172,7 @@  swf_demuxer_suggest="zlib"
 tak_demuxer_select="tak_parser"
 tg2_muxer_select="mov_muxer"
 tgp_muxer_select="mov_muxer"
+tty_demuxer_deps="unsafe_demuxers"
 vobsub_demuxer_select="mpegps_demuxer"
 w64_demuxer_select="wav_demuxer"
 w64_muxer_select="wav_muxer"
@@ -3176,6 +3182,7 @@  webm_muxer_select="iso_media riffenc"
 webm_dash_manifest_demuxer_select="matroska_demuxer"
 wtv_demuxer_select="mpegts_demuxer riffdec"
 wtv_muxer_select="mpegts_muxer riffenc"
+xbin_demuxer_deps="unsafe_demuxers"
 xmv_demuxer_select="riffdec"
 xwma_demuxer_select="riffdec"
 
diff --git a/tests/fate.sh b/tests/fate.sh
index 0edee7f..6a99d66 100755
--- a/tests/fate.sh
+++ b/tests/fate.sh
@@ -49,6 +49,7 @@  configure()(
         --enable-gpl                                                    \
         --enable-memory-poisoning                                       \
         --enable-avresample                                             \
+        --enable-unsafe-demuxers                                        \
         ${ignore_tests:+--ignore-tests="$ignore_tests"}                 \
         ${arch:+--arch=$arch}                                           \
         ${cpu:+--cpu="$cpu"}                                            \