[FFmpeg-devel,v3] avformat/hlsenc: reimplement randomize of hls use av_get_random_seed

Submitted by Steven Liu on June 4, 2018, 2:55 a.m.

Details

Message ID 20180604025547.19018-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu June 4, 2018, 2:55 a.m.
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 configure            |  1 -
 libavformat/hlsenc.c | 27 ++++++++++++---------------
 2 files changed, 12 insertions(+), 16 deletions(-)

Comments

Thomas Volkert June 4, 2018, 10:16 a.m.
On 04.06.2018 04:55, Steven Liu wrote:
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  configure            |  1 -
>  libavformat/hlsenc.c | 27 ++++++++++++---------------
>  2 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/configure b/configure
> index 22eeca22a5..a3d0f5837a 100755
> --- a/configure
> +++ b/configure
> @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
>  flac_demuxer_select="flac_parser"
>  hds_muxer_select="flv_muxer"
>  hls_muxer_select="mpegts_muxer"
> -hls_muxer_suggest="gcrypt openssl"
>  image2_alias_pix_demuxer_select="image2_demuxer"
>  image2_brender_pix_demuxer_select="image2_demuxer"
>  ipod_muxer_select="mov_muxer"
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 2268f898b0..c04c561586 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -27,12 +27,6 @@
>  #include <unistd.h>
>  #endif
>  
> -#if CONFIG_GCRYPT
> -#include <gcrypt.h>
> -#elif CONFIG_OPENSSL
> -#include <openssl/rand.h>
> -#endif
> -
>  #include "libavutil/avassert.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/parseutils.h"
> @@ -569,18 +563,21 @@ fail:
>      return ret;
>  }
>  
> +
>  static int randomize(uint8_t *buf, int len)
>  {
> -#if CONFIG_GCRYPT
> -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
> +    uint32_t tmp_number[4];
> +    int i = 0;
> +
> +    if (len != 16)
> +        return AVERROR(EINVAL);
> +
> +    for (i = 0; i < 4; i++)
> +        tmp_number[i] = av_get_random_seed();
> +
> +    memcpy(buf, tmp_number, len);
> +
>      return 0;
> -#elif CONFIG_OPENSSL
> -    if (RAND_bytes(buf, len))
> -        return 0;
> -#else
> -    return AVERROR(ENOSYS);
> -#endif
> -    return AVERROR(EINVAL);
>  }
>  
>  static int do_encrypt(AVFormatContext *s, VariantStream *vs)

Does av_get_random_seed() provide the same random quality level as
gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?


Best regards,
Thomas.
Hendrik Leppkes June 4, 2018, 10:43 a.m.
On Mon, Jun 4, 2018 at 12:16 PM, Thomas Volkert <silvo@gmx.net> wrote:
>
> On 04.06.2018 04:55, Steven Liu wrote:
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  configure            |  1 -
>>  libavformat/hlsenc.c | 27 ++++++++++++---------------
>>  2 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 22eeca22a5..a3d0f5837a 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
>>  flac_demuxer_select="flac_parser"
>>  hds_muxer_select="flv_muxer"
>>  hls_muxer_select="mpegts_muxer"
>> -hls_muxer_suggest="gcrypt openssl"
>>  image2_alias_pix_demuxer_select="image2_demuxer"
>>  image2_brender_pix_demuxer_select="image2_demuxer"
>>  ipod_muxer_select="mov_muxer"
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 2268f898b0..c04c561586 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -27,12 +27,6 @@
>>  #include <unistd.h>
>>  #endif
>>
>> -#if CONFIG_GCRYPT
>> -#include <gcrypt.h>
>> -#elif CONFIG_OPENSSL
>> -#include <openssl/rand.h>
>> -#endif
>> -
>>  #include "libavutil/avassert.h"
>>  #include "libavutil/mathematics.h"
>>  #include "libavutil/parseutils.h"
>> @@ -569,18 +563,21 @@ fail:
>>      return ret;
>>  }
>>
>> +
>>  static int randomize(uint8_t *buf, int len)
>>  {
>> -#if CONFIG_GCRYPT
>> -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
>> +    uint32_t tmp_number[4];
>> +    int i = 0;
>> +
>> +    if (len != 16)
>> +        return AVERROR(EINVAL);
>> +
>> +    for (i = 0; i < 4; i++)
>> +        tmp_number[i] = av_get_random_seed();
>> +
>> +    memcpy(buf, tmp_number, len);
>> +
>>      return 0;
>> -#elif CONFIG_OPENSSL
>> -    if (RAND_bytes(buf, len))
>> -        return 0;
>> -#else
>> -    return AVERROR(ENOSYS);
>> -#endif
>> -    return AVERROR(EINVAL);
>>  }
>>
>>  static int do_encrypt(AVFormatContext *s, VariantStream *vs)
>
> Does av_get_random_seed() provide the same random quality level as
> gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?
>

I can't really comment on quality, but av_get_random_seed does not use
any cryptographic randomness sources - unless you're on Windows.
On Linux it'll read /dev/urandom or /dev/random or make up its own
seed based on time as a hash, depending on whats available.

If  av_get_random_seed is supposed to be used cryptographically, it
should probably be expanded to use crypto APIs - although it being in
avutil might make that annoying linking-wise.

- Hendrik
Steven Liu June 4, 2018, 12:22 p.m.
2018-06-04 18:16 GMT+08:00 Thomas Volkert <silvo@gmx.net>:
>
> On 04.06.2018 04:55, Steven Liu wrote:
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  configure            |  1 -
>>  libavformat/hlsenc.c | 27 ++++++++++++---------------
>>  2 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 22eeca22a5..a3d0f5837a 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
>>  flac_demuxer_select="flac_parser"
>>  hds_muxer_select="flv_muxer"
>>  hls_muxer_select="mpegts_muxer"
>> -hls_muxer_suggest="gcrypt openssl"
>>  image2_alias_pix_demuxer_select="image2_demuxer"
>>  image2_brender_pix_demuxer_select="image2_demuxer"
>>  ipod_muxer_select="mov_muxer"
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 2268f898b0..c04c561586 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -27,12 +27,6 @@
>>  #include <unistd.h>
>>  #endif
>>
>> -#if CONFIG_GCRYPT
>> -#include <gcrypt.h>
>> -#elif CONFIG_OPENSSL
>> -#include <openssl/rand.h>
>> -#endif
>> -
>>  #include "libavutil/avassert.h"
>>  #include "libavutil/mathematics.h"
>>  #include "libavutil/parseutils.h"
>> @@ -569,18 +563,21 @@ fail:
>>      return ret;
>>  }
>>
>> +
>>  static int randomize(uint8_t *buf, int len)
>>  {
>> -#if CONFIG_GCRYPT
>> -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
>> +    uint32_t tmp_number[4];
>> +    int i = 0;
>> +
>> +    if (len != 16)
>> +        return AVERROR(EINVAL);
>> +
>> +    for (i = 0; i < 4; i++)
>> +        tmp_number[i] = av_get_random_seed();
>> +
>> +    memcpy(buf, tmp_number, len);
>> +
>>      return 0;
>> -#elif CONFIG_OPENSSL
>> -    if (RAND_bytes(buf, len))
>> -        return 0;
>> -#else
>> -    return AVERROR(ENOSYS);
>> -#endif
>> -    return AVERROR(EINVAL);
>>  }
>>
>>  static int do_encrypt(AVFormatContext *s, VariantStream *vs)
>
> Does av_get_random_seed() provide the same random quality level as
> gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?
>
I think the randomize in hls is same as other module using
av_get_random_seed, so maybe use av_get_random_seed is ok here.

Thanks

Steven

>
> Best regards,
> Thomas.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer June 4, 2018, 1:29 p.m.
On Mon, Jun 04, 2018 at 12:43:41PM +0200, Hendrik Leppkes wrote:
> On Mon, Jun 4, 2018 at 12:16 PM, Thomas Volkert <silvo@gmx.net> wrote:
> >
> > On 04.06.2018 04:55, Steven Liu wrote:
> >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> >> ---
> >>  configure            |  1 -
> >>  libavformat/hlsenc.c | 27 ++++++++++++---------------
> >>  2 files changed, 12 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/configure b/configure
> >> index 22eeca22a5..a3d0f5837a 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
> >>  flac_demuxer_select="flac_parser"
> >>  hds_muxer_select="flv_muxer"
> >>  hls_muxer_select="mpegts_muxer"
> >> -hls_muxer_suggest="gcrypt openssl"
> >>  image2_alias_pix_demuxer_select="image2_demuxer"
> >>  image2_brender_pix_demuxer_select="image2_demuxer"
> >>  ipod_muxer_select="mov_muxer"
> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> >> index 2268f898b0..c04c561586 100644
> >> --- a/libavformat/hlsenc.c
> >> +++ b/libavformat/hlsenc.c
> >> @@ -27,12 +27,6 @@
> >>  #include <unistd.h>
> >>  #endif
> >>
> >> -#if CONFIG_GCRYPT
> >> -#include <gcrypt.h>
> >> -#elif CONFIG_OPENSSL
> >> -#include <openssl/rand.h>
> >> -#endif
> >> -
> >>  #include "libavutil/avassert.h"
> >>  #include "libavutil/mathematics.h"
> >>  #include "libavutil/parseutils.h"
> >> @@ -569,18 +563,21 @@ fail:
> >>      return ret;
> >>  }
> >>
> >> +
> >>  static int randomize(uint8_t *buf, int len)
> >>  {
> >> -#if CONFIG_GCRYPT
> >> -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
> >> +    uint32_t tmp_number[4];
> >> +    int i = 0;
> >> +
> >> +    if (len != 16)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    for (i = 0; i < 4; i++)
> >> +        tmp_number[i] = av_get_random_seed();
> >> +
> >> +    memcpy(buf, tmp_number, len);
> >> +
> >>      return 0;
> >> -#elif CONFIG_OPENSSL
> >> -    if (RAND_bytes(buf, len))
> >> -        return 0;
> >> -#else
> >> -    return AVERROR(ENOSYS);
> >> -#endif
> >> -    return AVERROR(EINVAL);
> >>  }
> >>
> >>  static int do_encrypt(AVFormatContext *s, VariantStream *vs)
> >
> > Does av_get_random_seed() provide the same random quality level as
> > gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?
> >
> 
> I can't really comment on quality, but av_get_random_seed does not use
> any cryptographic randomness sources - unless you're on Windows.
> On Linux it'll read /dev/urandom or /dev/random or make up its own
> seed based on time as a hash, depending on whats available.
> 
> If  av_get_random_seed is supposed to be used cryptographically, it
> should probably be expanded to use crypto APIs - although it being in
> avutil might make that annoying linking-wise.

is it just me or does something here feel wrong, i dont mean anything
related to our code but rather the apparent general acceptance that
generation of good random numbers requires non system crypto libs.

IM(H)O its the job os the OS to provide unbiased random numbers.
the OS also has all the hardware to its disposal to gather entropy,
its in a better position to do this than a user level lib.


[...]
Michael Niedermayer June 4, 2018, 1:38 p.m.
On Mon, Jun 04, 2018 at 12:16:36PM +0200, Thomas Volkert wrote:
> 
> On 04.06.2018 04:55, Steven Liu wrote:
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  configure            |  1 -
> >  libavformat/hlsenc.c | 27 ++++++++++++---------------
> >  2 files changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 22eeca22a5..a3d0f5837a 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
> >  flac_demuxer_select="flac_parser"
> >  hds_muxer_select="flv_muxer"
> >  hls_muxer_select="mpegts_muxer"
> > -hls_muxer_suggest="gcrypt openssl"
> >  image2_alias_pix_demuxer_select="image2_demuxer"
> >  image2_brender_pix_demuxer_select="image2_demuxer"
> >  ipod_muxer_select="mov_muxer"
> > diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> > index 2268f898b0..c04c561586 100644
> > --- a/libavformat/hlsenc.c
> > +++ b/libavformat/hlsenc.c
> > @@ -27,12 +27,6 @@
> >  #include <unistd.h>
> >  #endif
> >  
> > -#if CONFIG_GCRYPT
> > -#include <gcrypt.h>
> > -#elif CONFIG_OPENSSL
> > -#include <openssl/rand.h>
> > -#endif
> > -
> >  #include "libavutil/avassert.h"
> >  #include "libavutil/mathematics.h"
> >  #include "libavutil/parseutils.h"
> > @@ -569,18 +563,21 @@ fail:
> >      return ret;
> >  }
> >  
> > +
> >  static int randomize(uint8_t *buf, int len)
> >  {
> > -#if CONFIG_GCRYPT
> > -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
> > +    uint32_t tmp_number[4];
> > +    int i = 0;
> > +
> > +    if (len != 16)
> > +        return AVERROR(EINVAL);
> > +
> > +    for (i = 0; i < 4; i++)
> > +        tmp_number[i] = av_get_random_seed();
> > +
> > +    memcpy(buf, tmp_number, len);
> > +
> >      return 0;
> > -#elif CONFIG_OPENSSL
> > -    if (RAND_bytes(buf, len))
> > -        return 0;
> > -#else
> > -    return AVERROR(ENOSYS);
> > -#endif
> > -    return AVERROR(EINVAL);
> >  }
> >  
> >  static int do_encrypt(AVFormatContext *s, VariantStream *vs)
> 
> Does av_get_random_seed() provide the same random quality level as
> gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?

we need a (preferrably single) API to get good random numbers from.
all libs may need to access it so libavutil is probably the place for it
and av_get_random_seed() is already there ...

having a list of #if/elif/else accesing multiple sources of randomness
should if it is needed be in av_get_random_seed() not duplicated every
time a random number is needed.

on a different topic, if someone wants to improve av_get_random_seed() in a
way different from adding libs as sources of randomness

data from image sensors (v4l* for example) and PCM audio. Should be
usefull sources of entropy.
Ive seen CCD imagers of phones being used as particle detectors. So theres
seems to be some evidence behind this generating actual truely random numbers

Thanks

[...]
Thomas Volkert June 4, 2018, 2:22 p.m.
On 04.06.2018 15:38, Michael Niedermayer wrote:
> On Mon, Jun 04, 2018 at 12:16:36PM +0200, Thomas Volkert wrote:
>> On 04.06.2018 04:55, Steven Liu wrote:
>>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>>> ---
>>>  configure            |  1 -
>>>  libavformat/hlsenc.c | 27 ++++++++++++---------------
>>>  2 files changed, 12 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 22eeca22a5..a3d0f5837a 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3127,7 +3127,6 @@ fifo_muxer_deps="threads"
>>>  flac_demuxer_select="flac_parser"
>>>  hds_muxer_select="flv_muxer"
>>>  hls_muxer_select="mpegts_muxer"
>>> -hls_muxer_suggest="gcrypt openssl"
>>>  image2_alias_pix_demuxer_select="image2_demuxer"
>>>  image2_brender_pix_demuxer_select="image2_demuxer"
>>>  ipod_muxer_select="mov_muxer"
>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>> index 2268f898b0..c04c561586 100644
>>> --- a/libavformat/hlsenc.c
>>> +++ b/libavformat/hlsenc.c
>>> @@ -27,12 +27,6 @@
>>>  #include <unistd.h>
>>>  #endif
>>>  
>>> -#if CONFIG_GCRYPT
>>> -#include <gcrypt.h>
>>> -#elif CONFIG_OPENSSL
>>> -#include <openssl/rand.h>
>>> -#endif
>>> -
>>>  #include "libavutil/avassert.h"
>>>  #include "libavutil/mathematics.h"
>>>  #include "libavutil/parseutils.h"
>>> @@ -569,18 +563,21 @@ fail:
>>>      return ret;
>>>  }
>>>  
>>> +
>>>  static int randomize(uint8_t *buf, int len)
>>>  {
>>> -#if CONFIG_GCRYPT
>>> -    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
>>> +    uint32_t tmp_number[4];
>>> +    int i = 0;
>>> +
>>> +    if (len != 16)
>>> +        return AVERROR(EINVAL);
>>> +
>>> +    for (i = 0; i < 4; i++)
>>> +        tmp_number[i] = av_get_random_seed();
>>> +
>>> +    memcpy(buf, tmp_number, len);
>>> +
>>>      return 0;
>>> -#elif CONFIG_OPENSSL
>>> -    if (RAND_bytes(buf, len))
>>> -        return 0;
>>> -#else
>>> -    return AVERROR(ENOSYS);
>>> -#endif
>>> -    return AVERROR(EINVAL);
>>>  }
>>>  
>>>  static int do_encrypt(AVFormatContext *s, VariantStream *vs)
>> Does av_get_random_seed() provide the same random quality level as
>> gcry_randomize() / RAND_bytes() / mbedtls_havege_random() ?
> we need a (preferrably single) API to get good random numbers from.
> all libs may need to access it so libavutil is probably the place for it
> and av_get_random_seed() is already there ...
>
> having a list of #if/elif/else accesing multiple sources of randomness
> should if it is needed be in av_get_random_seed() not duplicated every
> time a random number is needed.
Yes, I agree completely. The differentiation between the implementations
should be hidden behind av_get_random_seed().
But we should be careful when simplifying code at this point.

Best regards,
Thomas.

Patch hide | download patch | download mbox

diff --git a/configure b/configure
index 22eeca22a5..a3d0f5837a 100755
--- a/configure
+++ b/configure
@@ -3127,7 +3127,6 @@  fifo_muxer_deps="threads"
 flac_demuxer_select="flac_parser"
 hds_muxer_select="flv_muxer"
 hls_muxer_select="mpegts_muxer"
-hls_muxer_suggest="gcrypt openssl"
 image2_alias_pix_demuxer_select="image2_demuxer"
 image2_brender_pix_demuxer_select="image2_demuxer"
 ipod_muxer_select="mov_muxer"
diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 2268f898b0..c04c561586 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -27,12 +27,6 @@ 
 #include <unistd.h>
 #endif
 
-#if CONFIG_GCRYPT
-#include <gcrypt.h>
-#elif CONFIG_OPENSSL
-#include <openssl/rand.h>
-#endif
-
 #include "libavutil/avassert.h"
 #include "libavutil/mathematics.h"
 #include "libavutil/parseutils.h"
@@ -569,18 +563,21 @@  fail:
     return ret;
 }
 
+
 static int randomize(uint8_t *buf, int len)
 {
-#if CONFIG_GCRYPT
-    gcry_randomize(buf, len, GCRY_VERY_STRONG_RANDOM);
+    uint32_t tmp_number[4];
+    int i = 0;
+
+    if (len != 16)
+        return AVERROR(EINVAL);
+
+    for (i = 0; i < 4; i++)
+        tmp_number[i] = av_get_random_seed();
+
+    memcpy(buf, tmp_number, len);
+
     return 0;
-#elif CONFIG_OPENSSL
-    if (RAND_bytes(buf, len))
-        return 0;
-#else
-    return AVERROR(ENOSYS);
-#endif
-    return AVERROR(EINVAL);
 }
 
 static int do_encrypt(AVFormatContext *s, VariantStream *vs)