| Message ID | 20230702193010.11654-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/hlsenc: fall back to av_get_random_seed() when generating AES128 key | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Marton Balint <cus@passwd.hu> 于2023年7月3日周一 03:30写道： > > It should be OK to use av_get_random_seed() to generate the key instead of > using openSSL/Gcrypt functions. This removes the hard dependancy of those libs > for key generation functionality. > > Fixes ticket #10441. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavformat/hlsenc.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 1e0848ce3d..0b22c71186 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -40,6 +40,7 @@ > #include "libavutil/intreadwrite.h" > #include "libavutil/opt.h" > #include "libavutil/log.h" > +#include "libavutil/random_seed.h" > #include "libavutil/time.h" > #include "libavutil/time_internal.h" > > @@ -710,18 +711,18 @@ fail: > return ret; > } > > -static int randomize(uint8_t *buf, int len) > +static void randomize(uint8_t *buf, int len) > { > #if CONFIG_GCRYPT > gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); > - return 0; > + return; > #elif CONFIG_OPENSSL > if (RAND_bytes(buf, len)) > - return 0; > -#else > - return AVERROR(ENOSYS); > + return; > #endif > - return AVERROR(EINVAL); > + av_assert0(len % 4 == 0); > + for (int i = 0; i < len; i += 4) > + AV_WB32(buf + i, av_get_random_seed()); > } > > static int do_encrypt(AVFormatContext *s, VariantStream *vs) > @@ -775,10 +776,7 @@ static int do_encrypt(AVFormatContext *s, VariantStream *vs) > if (!*hls->key_string) { > AVDictionary *options = NULL; > if (!hls->key) { > - if ((ret = randomize(key, sizeof(key))) < 0) { > - av_log(s, AV_LOG_ERROR, "Cannot generate a strong random key\n"); > - return ret; > - } > + randomize(key, sizeof(key)); > } else { > memcpy(key, hls->key, sizeof(key)); > } Hi Marton, Should remove braces too. I cannot sure how to make it more simpler as !hls->key ? randomize(key, sizeof(key)) : memcpy(key, hls->key, sizeof(key)); ? Thanks Steven
On Mon, 3 Jul 2023, Steven Liu wrote: > Marton Balint <cus@passwd.hu> 于2023年7月3日周一 03:30写道： >> >> It should be OK to use av_get_random_seed() to generate the key instead of >> using openSSL/Gcrypt functions. This removes the hard dependancy of those libs >> for key generation functionality. >> >> Fixes ticket #10441. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- [...] >> static int do_encrypt(AVFormatContext *s, VariantStream *vs) >> @@ -775,10 +776,7 @@ static int do_encrypt(AVFormatContext *s, VariantStream *vs) >> if (!*hls->key_string) { >> AVDictionary *options = NULL; >> if (!hls->key) { >> - if ((ret = randomize(key, sizeof(key))) < 0) { >> - av_log(s, AV_LOG_ERROR, "Cannot generate a strong random key\n"); >> - return ret; >> - } >> + randomize(key, sizeof(key)); >> } else { >> memcpy(key, hls->key, sizeof(key)); >> } > Hi Marton, > > Should remove braces too. I cannot sure how to make it more simpler as > !hls->key ? randomize(key, sizeof(key)) : memcpy(key, hls->key, > sizeof(key)); ? I will just remove the braces, looks more readable to me than using the ternary operator. Will apply the series, thanks. Marton
On 7/2/2023 4:30 PM, Marton Balint wrote: > It should be OK to use av_get_random_seed() to generate the key instead of > using openSSL/Gcrypt functions. This removes the hard dependancy of those libs > for key generation functionality. > > Fixes ticket #10441. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavformat/hlsenc.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > index 1e0848ce3d..0b22c71186 100644 > --- a/libavformat/hlsenc.c > +++ b/libavformat/hlsenc.c > @@ -40,6 +40,7 @@ > #include "libavutil/intreadwrite.h" > #include "libavutil/opt.h" > #include "libavutil/log.h" > +#include "libavutil/random_seed.h" > #include "libavutil/time.h" > #include "libavutil/time_internal.h" > > @@ -710,18 +711,18 @@ fail: > return ret; > } > > -static int randomize(uint8_t *buf, int len) > +static void randomize(uint8_t *buf, int len) > { > #if CONFIG_GCRYPT > gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); > - return 0; > + return; > #elif CONFIG_OPENSSL > if (RAND_bytes(buf, len)) > - return 0; > -#else > - return AVERROR(ENOSYS); > + return; > #endif > - return AVERROR(EINVAL); > + av_assert0(len % 4 == 0); > + for (int i = 0; i < len; i += 4) > + AV_WB32(buf + i, av_get_random_seed()); Maybe instead use a PRNG, like the following: AVLFG c; av_lfg_init(&c, av_get_random_seed()); for (int i = 0; i < len; i += 4) AV_WB32(buf + i, av_lfg_get(&c));
Quoting James Almer (2023-07-03 21:33:04) > On 7/2/2023 4:30 PM, Marton Balint wrote: > > It should be OK to use av_get_random_seed() to generate the key instead of > > using openSSL/Gcrypt functions. This removes the hard dependancy of those libs > > for key generation functionality. > > > > Fixes ticket #10441. > > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > --- > > libavformat/hlsenc.c | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c > > index 1e0848ce3d..0b22c71186 100644 > > --- a/libavformat/hlsenc.c > > +++ b/libavformat/hlsenc.c > > @@ -40,6 +40,7 @@ > > #include "libavutil/intreadwrite.h" > > #include "libavutil/opt.h" > > #include "libavutil/log.h" > > +#include "libavutil/random_seed.h" > > #include "libavutil/time.h" > > #include "libavutil/time_internal.h" > > > > @@ -710,18 +711,18 @@ fail: > > return ret; > > } > > > > -static int randomize(uint8_t *buf, int len) > > +static void randomize(uint8_t *buf, int len) > > { > > #if CONFIG_GCRYPT > > gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); > > - return 0; > > + return; > > #elif CONFIG_OPENSSL > > if (RAND_bytes(buf, len)) > > - return 0; > > -#else > > - return AVERROR(ENOSYS); > > + return; > > #endif > > - return AVERROR(EINVAL); > > + av_assert0(len % 4 == 0); > > + for (int i = 0; i < len; i += 4) > > + AV_WB32(buf + i, av_get_random_seed()); > > Maybe instead use a PRNG, like the following: > > AVLFG c; > av_lfg_init(&c, av_get_random_seed()); > for (int i = 0; i < len; i += 4) > AV_WB32(buf + i, av_lfg_get(&c)); We really REALLY should not be taking any shortcuts when generating keys. Ideally we shouldn't be generating them ourselves in the first place, as we are not a crypto library. This patch seems like a step backward to me.
On Mon, 3 Jul 2023, James Almer wrote: > On 7/2/2023 4:30 PM, Marton Balint wrote: >> It should be OK to use av_get_random_seed() to generate the key instead of >> using openSSL/Gcrypt functions. This removes the hard dependancy of those >> libs >> for key generation functionality. >> >> Fixes ticket #10441. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavformat/hlsenc.c | 18 ++++++++---------- >> 1 file changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >> index 1e0848ce3d..0b22c71186 100644 >> --- a/libavformat/hlsenc.c >> +++ b/libavformat/hlsenc.c >> @@ -40,6 +40,7 @@ >> #include "libavutil/intreadwrite.h" >> #include "libavutil/opt.h" >> #include "libavutil/log.h" >> +#include "libavutil/random_seed.h" >> #include "libavutil/time.h" >> #include "libavutil/time_internal.h" >> >> @@ -710,18 +711,18 @@ fail: >> return ret; >> } >> >> -static int randomize(uint8_t *buf, int len) >> +static void randomize(uint8_t *buf, int len) >> { >> #if CONFIG_GCRYPT >> gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); >> - return 0; >> + return; >> #elif CONFIG_OPENSSL >> if (RAND_bytes(buf, len)) >> - return 0; >> -#else >> - return AVERROR(ENOSYS); >> + return; >> #endif >> - return AVERROR(EINVAL); >> + av_assert0(len % 4 == 0); >> + for (int i = 0; i < len; i += 4) >> + AV_WB32(buf + i, av_get_random_seed()); > > Maybe instead use a PRNG, like the following: > > AVLFG c; > av_lfg_init(&c, av_get_random_seed()); > for (int i = 0; i < len; i += 4) > AV_WB32(buf + i, av_lfg_get(&c)); If randomize() were to be used for arbitrary lengths, I'd agree, but here it is only used for key generation (only 128-bit keys in the current code), so I think av_get_random_seed() for the whole keysize is fine, and more secure. Regards, Marton
On Mon, 3 Jul 2023, Anton Khirnov wrote: > Quoting James Almer (2023-07-03 21:33:04) >> On 7/2/2023 4:30 PM, Marton Balint wrote: >>> It should be OK to use av_get_random_seed() to generate the key instead of >>> using openSSL/Gcrypt functions. This removes the hard dependancy of those libs >>> for key generation functionality. >>> >>> Fixes ticket #10441. >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> libavformat/hlsenc.c | 18 ++++++++---------- >>> 1 file changed, 8 insertions(+), 10 deletions(-) >>> >>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c >>> index 1e0848ce3d..0b22c71186 100644 >>> --- a/libavformat/hlsenc.c >>> +++ b/libavformat/hlsenc.c >>> @@ -40,6 +40,7 @@ >>> #include "libavutil/intreadwrite.h" >>> #include "libavutil/opt.h" >>> #include "libavutil/log.h" >>> +#include "libavutil/random_seed.h" >>> #include "libavutil/time.h" >>> #include "libavutil/time_internal.h" >>> >>> @@ -710,18 +711,18 @@ fail: >>> return ret; >>> } >>> >>> -static int randomize(uint8_t *buf, int len) >>> +static void randomize(uint8_t *buf, int len) >>> { >>> #if CONFIG_GCRYPT >>> gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); >>> - return 0; >>> + return; >>> #elif CONFIG_OPENSSL >>> if (RAND_bytes(buf, len)) >>> - return 0; >>> -#else >>> - return AVERROR(ENOSYS); >>> + return; >>> #endif >>> - return AVERROR(EINVAL); >>> + av_assert0(len % 4 == 0); >>> + for (int i = 0; i < len; i += 4) >>> + AV_WB32(buf + i, av_get_random_seed()); >> >> Maybe instead use a PRNG, like the following: >> >> AVLFG c; >> av_lfg_init(&c, av_get_random_seed()); >> for (int i = 0; i < len; i += 4) >> AV_WB32(buf + i, av_lfg_get(&c)); > > We really REALLY should not be taking any shortcuts when generating > keys. > > Ideally we shouldn't be generating them ourselves in the first place, as > we are not a crypto library. This patch seems like a step backward to > me. My patch use av_get_random_seed() which uses what the underlying OS provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for BSD/Mac. You really think that these are significantly worse than OpenSSL/GCrypt, so it should not be allowed to fallback to? Regards, Marton
Quoting Marton Balint (2023-07-03 22:54:41) > On Mon, 3 Jul 2023, Anton Khirnov wrote: > My patch use av_get_random_seed() which uses what the underlying OS > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > BSD/Mac. IOW it's a jungle of various paths, some of which are not guaranteed to be cryptographically secure. I see no such guarantees for arc4random() from a brief web search, and the fallback get_generic_seed() certainly is not either. Granted it's only used on obscure architectures, but still. The doxy even says > This function tries to provide a good seed at a best effort bases. > You really think that these are significantly worse than > OpenSSL/GCrypt, so it should not be allowed to fallback to? I think we should be using cryptographically secure PRNG for generating encryption keys, or fail when they are not available. If you want to get rid of the openssl dependency, IMO the best solution is a new int av_random(uint8_t* buf, size_t len); that guarantees either cryptographically secure randomness or an error.
On Mon, 3 Jul 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-07-03 22:54:41) >> On Mon, 3 Jul 2023, Anton Khirnov wrote: >> My patch use av_get_random_seed() which uses what the underlying OS >> provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for >> BSD/Mac. > > IOW it's a jungle of various paths, some of which are not guaranteed to > be cryptographically secure. I see no such guarantees for arc4random() It depends on OS and version most likely. > from a brief web search, and the fallback get_generic_seed() certainly > is not either. > Granted it's only used on obscure architectures, but > still. I am no expert on the subject, but even the generic code seems reasonably secure. It gathers entropy, it uses a crypto hash to get the output. And as you said, even that only used for obscure cases. > > The doxy even says >> This function tries to provide a good seed at a best effort bases. > >> You really think that these are significantly worse than >> OpenSSL/GCrypt, so it should not be allowed to fallback to? > > I think we should be using cryptographically secure PRNG for generating > encryption keys, or fail when they are not available. If you want to get > rid of the openssl dependency, IMO the best solution is a new > int av_random(uint8_t* buf, size_t len); > that guarantees either cryptographically secure randomness or an error. Sorry, seems a bit overdesign for me, so I won't be pursuing this further. Regards, Marton
On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > Quoting Marton Balint (2023-07-03 22:54:41) > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > My patch use av_get_random_seed() which uses what the underlying OS > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > BSD/Mac. > > IOW it's a jungle of various paths, some of which are not guaranteed to > be cryptographically secure. I see no such guarantees for arc4random() > from a brief web search, and the fallback get_generic_seed() certainly > is not either. Granted it's only used on obscure architectures, but > still. > > The doxy even says > > This function tries to provide a good seed at a best effort bases. > > > You really think that these are significantly worse than > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > I think we should be using cryptographically secure PRNG for generating > encryption keys, or fail when they are not available. If you want to get > rid of the openssl dependency, IMO the best solution is a new > int av_random(uint8_t* buf, size_t len); > that guarantees either cryptographically secure randomness or an error. "guarantees cryptographically secure randomness" ? If one defined "cryptographically secure" as "not broken publically as of today" Iam saying that as i think "guarantees" can be misleading in what it means thx [...]
Quoting Michael Niedermayer (2023-07-04 01:50:57) > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > Quoting Marton Balint (2023-07-03 22:54:41) > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > My patch use av_get_random_seed() which uses what the underlying OS > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > BSD/Mac. > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > be cryptographically secure. I see no such guarantees for arc4random() > > from a brief web search, and the fallback get_generic_seed() certainly > > is not either. Granted it's only used on obscure architectures, but > > still. > > > > The doxy even says > > > This function tries to provide a good seed at a best effort bases. > > > > > You really think that these are significantly worse than > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > I think we should be using cryptographically secure PRNG for generating > > encryption keys, or fail when they are not available. If you want to get > > rid of the openssl dependency, IMO the best solution is a new > > int av_random(uint8_t* buf, size_t len); > > that guarantees either cryptographically secure randomness or an error. > > "guarantees cryptographically secure randomness" ? > If one defined "cryptographically secure" as "not broken publically as of today" > > Iam saying that as i think "guarantees" can be misleading in what it means I feel your snark is very much misplaced. I recall way more instances of broken crypto caused by overconfident non-experts with an attitude like yours ("those silly crypto libraries, broken all the time, how hard can it be really") than by actual vulnerabilities in actual crypto libraries. In fact the highest-profile break I remember (Debian key entropy bug) was caused precisely by non-experts fiddling with code they did not understand.
On Tue, 4 Jul 2023 at 06:54, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() > for > > > > BSD/Mac. > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > be cryptographically secure. I see no such guarantees for arc4random() > > > from a brief web search, and the fallback get_generic_seed() certainly > > > is not either. Granted it's only used on obscure architectures, but > > > still. > > > > > > The doxy even says > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > You really think that these are significantly worse than > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > I think we should be using cryptographically secure PRNG for generating > > > encryption keys, or fail when they are not available. If you want to > get > > > rid of the openssl dependency, IMO the best solution is a new > > > int av_random(uint8_t* buf, size_t len); > > > that guarantees either cryptographically secure randomness or an error. > > > > "guarantees cryptographically secure randomness" ? > > If one defined "cryptographically secure" as "not broken publically as > of today" > > > > Iam saying that as i think "guarantees" can be misleading in what it > means > > I feel your snark is very much misplaced. > > I recall way more instances of broken crypto caused by overconfident > non-experts with an attitude like yours ("those silly crypto libraries, > broken all the time, how hard can it be really") than by actual > vulnerabilities in actual crypto libraries. > > In fact the highest-profile break I remember (Debian key entropy bug) > was caused precisely by non-experts fiddling with code they did not > understand. > +1 Kieran
On 7/4/2023 2:54 AM, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-04 01:50:57) >> On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: >>> Quoting Marton Balint (2023-07-03 22:54:41) >>>> On Mon, 3 Jul 2023, Anton Khirnov wrote: >>>> My patch use av_get_random_seed() which uses what the underlying OS >>>> provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for >>>> BSD/Mac. >>> >>> IOW it's a jungle of various paths, some of which are not guaranteed to >>> be cryptographically secure. I see no such guarantees for arc4random() >>> from a brief web search, and the fallback get_generic_seed() certainly >>> is not either. Granted it's only used on obscure architectures, but >>> still. >>> >>> The doxy even says >>>> This function tries to provide a good seed at a best effort bases. >>> >>>> You really think that these are significantly worse than >>>> OpenSSL/GCrypt, so it should not be allowed to fallback to? >>> >>> I think we should be using cryptographically secure PRNG for generating >>> encryption keys, or fail when they are not available. If you want to get >>> rid of the openssl dependency, IMO the best solution is a new >>> int av_random(uint8_t* buf, size_t len); >>> that guarantees either cryptographically secure randomness or an error. >> >> "guarantees cryptographically secure randomness" ? >> If one defined "cryptographically secure" as "not broken publically as of today" >> >> Iam saying that as i think "guarantees" can be misleading in what it means > > I feel your snark is very much misplaced. > > I recall way more instances of broken crypto caused by overconfident > non-experts with an attitude like yours ("those silly crypto libraries, > broken all the time, how hard can it be really") than by actual > vulnerabilities in actual crypto libraries. > > In fact the highest-profile break I remember (Debian key entropy bug) > was caused precisely by non-experts fiddling with code they did not > understand. Maybe the gcrypt and openssl API calls used here can instead be moved to av_get_random_seed(), which would reduce (or outright remove) the cases /dev/random or get_generic_seed() are called and result in essentially no changes to this functionality here?
Quoting James Almer (2023-07-04 16:37:03) > On 7/4/2023 2:54 AM, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-07-04 01:50:57) > >> On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > >>> Quoting Marton Balint (2023-07-03 22:54:41) > >>>> On Mon, 3 Jul 2023, Anton Khirnov wrote: > >>>> My patch use av_get_random_seed() which uses what the underlying OS > >>>> provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > >>>> BSD/Mac. > >>> > >>> IOW it's a jungle of various paths, some of which are not guaranteed to > >>> be cryptographically secure. I see no such guarantees for arc4random() > >>> from a brief web search, and the fallback get_generic_seed() certainly > >>> is not either. Granted it's only used on obscure architectures, but > >>> still. > >>> > >>> The doxy even says > >>>> This function tries to provide a good seed at a best effort bases. > >>> > >>>> You really think that these are significantly worse than > >>>> OpenSSL/GCrypt, so it should not be allowed to fallback to? > >>> > >>> I think we should be using cryptographically secure PRNG for generating > >>> encryption keys, or fail when they are not available. If you want to get > >>> rid of the openssl dependency, IMO the best solution is a new > >>> int av_random(uint8_t* buf, size_t len); > >>> that guarantees either cryptographically secure randomness or an error. > >> > >> "guarantees cryptographically secure randomness" ? > >> If one defined "cryptographically secure" as "not broken publically as of today" > >> > >> Iam saying that as i think "guarantees" can be misleading in what it means > > > > I feel your snark is very much misplaced. > > > > I recall way more instances of broken crypto caused by overconfident > > non-experts with an attitude like yours ("those silly crypto libraries, > > broken all the time, how hard can it be really") than by actual > > vulnerabilities in actual crypto libraries. > > > > In fact the highest-profile break I remember (Debian key entropy bug) > > was caused precisely by non-experts fiddling with code they did not > > understand. > > Maybe the gcrypt and openssl API calls used here can instead be moved to > av_get_random_seed(), which would reduce (or outright remove) the cases > /dev/random or get_generic_seed() are called and result in essentially I see nothing wrong with using /dev/random, it's probably the most trustworthy source on most machines. Though on linux it's probably even better to use getrandom() where available.
On 7/3/2023 6:52 PM, Marton Balint wrote: > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > >> Quoting Marton Balint (2023-07-03 22:54:41) >>> On Mon, 3 Jul 2023, Anton Khirnov wrote: >>> My patch use av_get_random_seed() which uses what the underlying OS >>> provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for >>> BSD/Mac. >> >> IOW it's a jungle of various paths, some of which are not guaranteed to >> be cryptographically secure. I see no such guarantees for arc4random() > > It depends on OS and version most likely. > >> from a brief web search, and the fallback get_generic_seed() certainly >> is not either. >> Granted it's only used on obscure architectures, but >> still. > > I am no expert on the subject, but even the generic code seems > reasonably secure. It gathers entropy, it uses a crypto hash to get the > output. And as you said, even that only used for obscure cases. > >> >> The doxy even says >>> This function tries to provide a good seed at a best effort bases. >> >>> You really think that these are significantly worse than >>> OpenSSL/GCrypt, so it should not be allowed to fallback to? >> >> I think we should be using cryptographically secure PRNG for generating >> encryption keys, or fail when they are not available. If you want to get >> rid of the openssl dependency, IMO the best solution is a new >> int av_random(uint8_t* buf, size_t len); >> that guarantees either cryptographically secure randomness or an error. > > Sorry, seems a bit overdesign for me, so I won't be pursuing this further. I just sent a patchset implementing a new av_random() function that uses the existing av_get_random_seed() entropy sources, and adds the ones used by hlsenc too. If that goes in, then this patchset can go in as well afterwards.
On Tue, 4 Jul 2023, James Almer wrote: > On 7/3/2023 6:52 PM, Marton Balint wrote: >> >> >> On Mon, 3 Jul 2023, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2023-07-03 22:54:41) >>>> On Mon, 3 Jul 2023, Anton Khirnov wrote: >>>> My patch use av_get_random_seed() which uses what the underlying OS >>>> provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for >>>> BSD/Mac. >>> >>> IOW it's a jungle of various paths, some of which are not guaranteed to >>> be cryptographically secure. I see no such guarantees for arc4random() >> >> It depends on OS and version most likely. >> >>> from a brief web search, and the fallback get_generic_seed() certainly >>> is not either. >>> Granted it's only used on obscure architectures, but >>> still. >> >> I am no expert on the subject, but even the generic code seems reasonably >> secure. It gathers entropy, it uses a crypto hash to get the output. And >> as you said, even that only used for obscure cases. >> >>> >>> The doxy even says >>>> This function tries to provide a good seed at a best effort bases. >>> >>>> You really think that these are significantly worse than >>>> OpenSSL/GCrypt, so it should not be allowed to fallback to? >>> >>> I think we should be using cryptographically secure PRNG for generating >>> encryption keys, or fail when they are not available. If you want to get >>> rid of the openssl dependency, IMO the best solution is a new >>> int av_random(uint8_t* buf, size_t len); >>> that guarantees either cryptographically secure randomness or an error. >> >> Sorry, seems a bit overdesign for me, so I won't be pursuing this further. > > I just sent a patchset implementing a new av_random() function that uses the > existing av_get_random_seed() entropy sources, and adds the ones used by > hlsenc too. > If that goes in, then this patchset can go in as well afterwards. Thanks! I will rework the patch to use av_random() then. Regards, Marton
On Tue, Jul 04, 2023 at 07:54:06AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > > BSD/Mac. > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > be cryptographically secure. I see no such guarantees for arc4random() > > > from a brief web search, and the fallback get_generic_seed() certainly > > > is not either. Granted it's only used on obscure architectures, but > > > still. > > > > > > The doxy even says > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > You really think that these are significantly worse than > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > I think we should be using cryptographically secure PRNG for generating > > > encryption keys, or fail when they are not available. If you want to get > > > rid of the openssl dependency, IMO the best solution is a new > > > int av_random(uint8_t* buf, size_t len); > > > that guarantees either cryptographically secure randomness or an error. > > > > "guarantees cryptographically secure randomness" ? > > If one defined "cryptographically secure" as "not broken publically as of today" > > > > Iam saying that as i think "guarantees" can be misleading in what it means > > I feel your snark is very much misplaced. > > I recall way more instances of broken crypto caused by overconfident > non-experts with an attitude like yours ("those silly crypto libraries, > broken all the time, how hard can it be really") than by actual > vulnerabilities in actual crypto libraries. > > In fact the highest-profile break I remember (Debian key entropy bug) > was caused precisely by non-experts fiddling with code they did not > understand. There is no snark here, at least that was not the intend Also what you say in these 2 paragraphs is true but isnt really related to what i said or meant to say these cryptographically secure PRNGS are secure as long as the currently used components and assumtations they are build on havnt been broken. Can i do better? no. but that doesnt mean that these are going to be unbroken in 30 years. just look 30 years in the past what percentage of what was believed to be secure 30 years ago has been broken today. or 50 or 100years thats really what i meant thx [...]
Quoting Michael Niedermayer (2023-07-05 01:50:12) > On Tue, Jul 04, 2023 at 07:54:06AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > > > BSD/Mac. > > > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > > be cryptographically secure. I see no such guarantees for arc4random() > > > > from a brief web search, and the fallback get_generic_seed() certainly > > > > is not either. Granted it's only used on obscure architectures, but > > > > still. > > > > > > > > The doxy even says > > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > > > You really think that these are significantly worse than > > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > > > I think we should be using cryptographically secure PRNG for generating > > > > encryption keys, or fail when they are not available. If you want to get > > > > rid of the openssl dependency, IMO the best solution is a new > > > > int av_random(uint8_t* buf, size_t len); > > > > that guarantees either cryptographically secure randomness or an error. > > > > > > "guarantees cryptographically secure randomness" ? > > > If one defined "cryptographically secure" as "not broken publically as of today" > > > > > > Iam saying that as i think "guarantees" can be misleading in what it means > > > > I feel your snark is very much misplaced. > > > > > I recall way more instances of broken crypto caused by overconfident > > non-experts with an attitude like yours ("those silly crypto libraries, > > broken all the time, how hard can it be really") than by actual > > vulnerabilities in actual crypto libraries. > > > > In fact the highest-profile break I remember (Debian key entropy bug) > > was caused precisely by non-experts fiddling with code they did not > > understand. > > There is no snark here, at least that was not the intend > Also what you say in these 2 paragraphs is true but isnt really > related to what i said or meant to say > > these cryptographically secure PRNGS are secure as long as the > currently used components and assumtations they are build on havnt > been broken. > Can i do better? no. but that doesnt mean that these > are going to be unbroken in 30 years. > just look 30 years in the past what percentage of what was believed to > be secure 30 years ago has been broken today. or 50 or 100years > thats really what i meant I still don't see what point are you trying to make here. Yes, any practical cryptographic algorithm could potentially be broken at some point. And everything in real life is imperfect, because we do not live in the world of ideal forms. But I don't see what practical steps could or should be taken in response to this.
On Wed, Jul 05, 2023 at 11:22:44AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-05 01:50:12) > > On Tue, Jul 04, 2023 at 07:54:06AM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > > > > BSD/Mac. > > > > > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > > > be cryptographically secure. I see no such guarantees for arc4random() > > > > > from a brief web search, and the fallback get_generic_seed() certainly > > > > > is not either. Granted it's only used on obscure architectures, but > > > > > still. > > > > > > > > > > The doxy even says > > > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > > > > > You really think that these are significantly worse than > > > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > > > > > I think we should be using cryptographically secure PRNG for generating > > > > > encryption keys, or fail when they are not available. If you want to get > > > > > rid of the openssl dependency, IMO the best solution is a new > > > > > int av_random(uint8_t* buf, size_t len); > > > > > that guarantees either cryptographically secure randomness or an error. > > > > > > > > "guarantees cryptographically secure randomness" ? > > > > If one defined "cryptographically secure" as "not broken publically as of today" > > > > > > > > Iam saying that as i think "guarantees" can be misleading in what it means > > > > > > I feel your snark is very much misplaced. > > > > > > > > I recall way more instances of broken crypto caused by overconfident > > > non-experts with an attitude like yours ("those silly crypto libraries, > > > broken all the time, how hard can it be really") than by actual > > > vulnerabilities in actual crypto libraries. > > > > > > In fact the highest-profile break I remember (Debian key entropy bug) > > > was caused precisely by non-experts fiddling with code they did not > > > understand. > > > > There is no snark here, at least that was not the intend > > Also what you say in these 2 paragraphs is true but isnt really > > related to what i said or meant to say > > > > these cryptographically secure PRNGS are secure as long as the > > currently used components and assumtations they are build on havnt > > been broken. > > Can i do better? no. but that doesnt mean that these > > are going to be unbroken in 30 years. > > just look 30 years in the past what percentage of what was believed to > > be secure 30 years ago has been broken today. or 50 or 100years > > thats really what i meant > > I still don't see what point are you trying to make here. > Yes, any practical cryptographic algorithm could potentially be broken > at some point. And everything in real life is imperfect, because we do > not live in the world of ideal forms. > But I don't see what practical steps could or should be taken in > response to this. for us i dont know but a user could instead of putting critical data in a system that might be broken in 30 years just write it down on paper and burn and grind the paper when its not needed anymore (which may or may not be an option) nothing is perfect but there are methods to transfer and destroy data which have a long track record of being secure and are simple. I think we should not make it sound like encrypting with these random numbers is as good as not storing/transmitting or using bits from fliping a real fair coin thx [...]
Quoting Michael Niedermayer (2023-07-06 00:54:47) > On Wed, Jul 05, 2023 at 11:22:44AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2023-07-05 01:50:12) > > > On Tue, Jul 04, 2023 at 07:54:06AM +0200, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > > > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > > > > > BSD/Mac. > > > > > > > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > > > > be cryptographically secure. I see no such guarantees for arc4random() > > > > > > from a brief web search, and the fallback get_generic_seed() certainly > > > > > > is not either. Granted it's only used on obscure architectures, but > > > > > > still. > > > > > > > > > > > > The doxy even says > > > > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > > > > > > > You really think that these are significantly worse than > > > > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > > > > > > > I think we should be using cryptographically secure PRNG for generating > > > > > > encryption keys, or fail when they are not available. If you want to get > > > > > > rid of the openssl dependency, IMO the best solution is a new > > > > > > int av_random(uint8_t* buf, size_t len); > > > > > > that guarantees either cryptographically secure randomness or an error. > > > > > > > > > > "guarantees cryptographically secure randomness" ? > > > > > If one defined "cryptographically secure" as "not broken publically as of today" > > > > > > > > > > Iam saying that as i think "guarantees" can be misleading in what it means > > > > > > > > I feel your snark is very much misplaced. > > > > > > > > > > > I recall way more instances of broken crypto caused by overconfident > > > > non-experts with an attitude like yours ("those silly crypto libraries, > > > > broken all the time, how hard can it be really") than by actual > > > > vulnerabilities in actual crypto libraries. > > > > > > > > In fact the highest-profile break I remember (Debian key entropy bug) > > > > was caused precisely by non-experts fiddling with code they did not > > > > understand. > > > > > > There is no snark here, at least that was not the intend > > > Also what you say in these 2 paragraphs is true but isnt really > > > related to what i said or meant to say > > > > > > these cryptographically secure PRNGS are secure as long as the > > > currently used components and assumtations they are build on havnt > > > been broken. > > > Can i do better? no. but that doesnt mean that these > > > are going to be unbroken in 30 years. > > > just look 30 years in the past what percentage of what was believed to > > > be secure 30 years ago has been broken today. or 50 or 100years > > > thats really what i meant > > > > I still don't see what point are you trying to make here. > > Yes, any practical cryptographic algorithm could potentially be broken > > at some point. And everything in real life is imperfect, because we do > > not live in the world of ideal forms. > > > But I don't see what practical steps could or should be taken in > > response to this. > > for us i dont know but a user could > instead of putting critical data in a system that might be broken in 30 years > just write it down on paper and burn and grind the paper when its not needed anymore > (which may or may not be an option) > > nothing is perfect but there are methods to transfer and destroy data which have a > long track record of being secure and are simple. > > I think we should not make it sound like encrypting with these random numbers > is as good as not storing/transmitting or using bits from fliping a real fair coin We are not claiming that. We are claiming that the random numbers generated are (to the best of our ability, and that of the underlying libraries we rely on) cryptographically secure. This means suitable for use in state of the art cryptographic algorithms like AES. I do not think it makes sense to mistrust CSPRNGs, yet still trust AES.
On Thu, 6 Jul 2023, 08:52 Anton Khirnov, <anton@khirnov.net> wrote: > > We are not claiming that. We are claiming that the random numbers > generated are (to the best of our ability, and that of the underlying > libraries we rely on) cryptographically secure. This means suitable for > use in state of the art cryptographic algorithms like AES. > I do not think it makes sense to mistrust CSPRNGs, yet still trust AES. Agreed, I don't think it's ffmpeg's job to get into a philosophical discussion about the state of the art of random number generation. Kieran > ". >
On Thu, Jul 06, 2023 at 09:52:12AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-06 00:54:47) > > On Wed, Jul 05, 2023 at 11:22:44AM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2023-07-05 01:50:12) > > > > On Tue, Jul 04, 2023 at 07:54:06AM +0200, Anton Khirnov wrote: > > > > > Quoting Michael Niedermayer (2023-07-04 01:50:57) > > > > > > On Mon, Jul 03, 2023 at 11:09:54PM +0200, Anton Khirnov wrote: > > > > > > > Quoting Marton Balint (2023-07-03 22:54:41) > > > > > > > > On Mon, 3 Jul 2023, Anton Khirnov wrote: > > > > > > > > My patch use av_get_random_seed() which uses what the underlying OS > > > > > > > > provides, BCrypt for Windows, /dev/urandom for Linux, arc4random() for > > > > > > > > BSD/Mac. > > > > > > > > > > > > > > IOW it's a jungle of various paths, some of which are not guaranteed to > > > > > > > be cryptographically secure. I see no such guarantees for arc4random() > > > > > > > from a brief web search, and the fallback get_generic_seed() certainly > > > > > > > is not either. Granted it's only used on obscure architectures, but > > > > > > > still. > > > > > > > > > > > > > > The doxy even says > > > > > > > > This function tries to provide a good seed at a best effort bases. > > > > > > > > > > > > > > > You really think that these are significantly worse than > > > > > > > > OpenSSL/GCrypt, so it should not be allowed to fallback to? > > > > > > > > > > > > > > I think we should be using cryptographically secure PRNG for generating > > > > > > > encryption keys, or fail when they are not available. If you want to get > > > > > > > rid of the openssl dependency, IMO the best solution is a new > > > > > > > int av_random(uint8_t* buf, size_t len); > > > > > > > that guarantees either cryptographically secure randomness or an error. > > > > > > > > > > > > "guarantees cryptographically secure randomness" ? > > > > > > If one defined "cryptographically secure" as "not broken publically as of today" > > > > > > > > > > > > Iam saying that as i think "guarantees" can be misleading in what it means > > > > > > > > > > I feel your snark is very much misplaced. > > > > > > > > > > > > > > I recall way more instances of broken crypto caused by overconfident > > > > > non-experts with an attitude like yours ("those silly crypto libraries, > > > > > broken all the time, how hard can it be really") than by actual > > > > > vulnerabilities in actual crypto libraries. > > > > > > > > > > In fact the highest-profile break I remember (Debian key entropy bug) > > > > > was caused precisely by non-experts fiddling with code they did not > > > > > understand. > > > > > > > > There is no snark here, at least that was not the intend > > > > Also what you say in these 2 paragraphs is true but isnt really > > > > related to what i said or meant to say > > > > > > > > these cryptographically secure PRNGS are secure as long as the > > > > currently used components and assumtations they are build on havnt > > > > been broken. > > > > Can i do better? no. but that doesnt mean that these > > > > are going to be unbroken in 30 years. > > > > just look 30 years in the past what percentage of what was believed to > > > > be secure 30 years ago has been broken today. or 50 or 100years > > > > thats really what i meant > > > > > > I still don't see what point are you trying to make here. > > > Yes, any practical cryptographic algorithm could potentially be broken > > > at some point. And everything in real life is imperfect, because we do > > > not live in the world of ideal forms. > > > > > But I don't see what practical steps could or should be taken in > > > response to this. > > > > for us i dont know but a user could > > instead of putting critical data in a system that might be broken in 30 years > > just write it down on paper and burn and grind the paper when its not needed anymore > > (which may or may not be an option) > > > > nothing is perfect but there are methods to transfer and destroy data which have a > > long track record of being secure and are simple. > > > > I think we should not make it sound like encrypting with these random numbers > > is as good as not storing/transmitting or using bits from fliping a real fair coin > > We are not claiming that. We are claiming that the random numbers > generated are (to the best of our ability, and that of the underlying > libraries we rely on) cryptographically secure. This means suitable for > use in state of the art cryptographic algorithms like AES. > I do not think it makes sense to mistrust CSPRNGs, yet still trust AES. The litteral wording was "that guarantees either cryptographically secure randomness or an error." that was what i refered to. the wording used now: "to the best of our ability, and that of the underlying libraries we rely on) cryptographically secure." is perfectly fine with me. I would have the same issue if someone said AES gurantees ... thx [...]
Quoting Michael Niedermayer (2023-07-07 02:55:46) > > The litteral wording was > "that guarantees either cryptographically secure randomness or an error." > > that was what i refered to. > > the wording used now: > "to the best of our ability, and that of the underlying libraries we rely on) cryptographically secure." > > is perfectly fine with me. > I would have the same issue if someone said AES gurantees ... IMO the two formulations are equivalent whenever it comes to practical computing. An algorithm can be mathematically proven to be sound*, but any practical computing scheme on actual hardware is always subject to software bugs, system misconfiguration, hardware bugs, hardware failure, etc. We use similar wording in other documentation, where e.g. we might guarantee that some function returns a NULL-terminated string or so. That guarantee is always under the implicit condition that there are no bugs and the code runs in the expected environment. The same considerations apply here. * assuming there are no bugs in your proof
On Fri, Jul 07, 2023 at 10:05:50AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2023-07-07 02:55:46) > > > > The litteral wording was > > "that guarantees either cryptographically secure randomness or an error." > > > > that was what i refered to. > > > > the wording used now: > > "to the best of our ability, and that of the underlying libraries we rely on) cryptographically secure." > > > > is perfectly fine with me. > > I would have the same issue if someone said AES gurantees ... > > IMO the two formulations are equivalent whenever it comes to practical > computing. An algorithm can be mathematically proven to be sound*, but > any practical computing scheme on actual hardware is always subject to > software bugs, system misconfiguration, hardware bugs, hardware failure, > etc. > > We use similar wording in other documentation, where e.g. we might > guarantee that some function returns a NULL-terminated string or so. > That guarantee is always under the implicit condition that there are no > bugs and the code runs in the expected environment. The same > considerations apply here. Theres a big difference between a bug in our implementation And us claiming some cryptographic primitive is secure. It was said previously that we shouldnt do things we lack the experties on and rather delegate to cryptographic libraries writen and audited by experts. (where it matters for security not just for playback) But claiming CSPRNG or AES or anything else is guranteed secure is exactly such a claim that is not within our experties. If you claim your code produces a null terminated string that i believe you (within the bounds you mentioned) but if you tell me AES will always be secure i wont believe you that unless you have the mathemtical proofs to back that up (and i read and understood them). Now all that flawlessness with security primitives from proper security libs and stuff needs to be taken with a grain of salt too. just 4 months ago i found 2 issues with teh random number generator in the hardware password manager that i use. So yeah maybe people feels iam too nitpicky here but honestly id rather be nitpicky on security stuff thx [...]
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c index 1e0848ce3d..0b22c71186 100644 --- a/libavformat/hlsenc.c +++ b/libavformat/hlsenc.c @@ -40,6 +40,7 @@ #include "libavutil/intreadwrite.h" #include "libavutil/opt.h" #include "libavutil/log.h" +#include "libavutil/random_seed.h" #include "libavutil/time.h" #include "libavutil/time_internal.h" @@ -710,18 +711,18 @@ fail: return ret; } -static int randomize(uint8_t *buf, int len) +static void randomize(uint8_t *buf, int len) { #if CONFIG_GCRYPT gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM); - return 0; + return; #elif CONFIG_OPENSSL if (RAND_bytes(buf, len)) - return 0; -#else - return AVERROR(ENOSYS); + return; #endif - return AVERROR(EINVAL); + av_assert0(len % 4 == 0); + for (int i = 0; i < len; i += 4) + AV_WB32(buf + i, av_get_random_seed()); } static int do_encrypt(AVFormatContext *s, VariantStream *vs) @@ -775,10 +776,7 @@ static int do_encrypt(AVFormatContext *s, VariantStream *vs) if (!*hls->key_string) { AVDictionary *options = NULL; if (!hls->key) { - if ((ret = randomize(key, sizeof(key))) < 0) { - av_log(s, AV_LOG_ERROR, "Cannot generate a strong random key\n"); - return ret; - } + randomize(key, sizeof(key)); } else { memcpy(key, hls->key, sizeof(key)); }
It should be OK to use av_get_random_seed() to generate the key instead of using openSSL/Gcrypt functions. This removes the hard dependancy of those libs for key generation functionality. Fixes ticket #10441. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/hlsenc.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)