| Message ID | 1525967099-28195-1-git-send-email-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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.
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
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
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
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
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
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. [...]
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
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?
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.
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.
> 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
> 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
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.
> 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
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.
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.
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.
> 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
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.
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).
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
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
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.
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
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.
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.
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.
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
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.
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 [...]
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"} \
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(+)