[FFmpeg-devel] [PATCH 1/2] avformat/hlsenc: fall back to av_get_random_seed() when generating AES128 key

[Marton Balint]

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