| Message ID | 20230704185044.2154-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avutil/random_seed: add av_random() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, 4 Jul 2023, James Almer wrote: > Uses the existing code for av_get_random_seed() to return a buffer with > cryptographically secure random data, or an error if none could be generated. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > TODO: APIChanges entry and minor version bump. > > Also, if a new random.h header is prefered, i can move the prototype there. > > libavutil/random_seed.c | 46 ++++++++++++++++++++++++++--------------- > libavutil/random_seed.h | 12 +++++++++++ > 2 files changed, 41 insertions(+), 17 deletions(-) > > diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c > index 66dd504ef0..39fb27c5ad 100644 > --- a/libavutil/random_seed.c > +++ b/libavutil/random_seed.c > @@ -46,20 +46,20 @@ > #define TEST 0 > #endif > > -static int read_random(uint32_t *dst, const char *file) > -{ > #if HAVE_UNISTD_H > +static ssize_t read_random(uint8_t *dst, size_t len, const char *file) > +{ > int fd = avpriv_open(file, O_RDONLY); > - int err = -1; > + ssize_t err = -1; > > + if (len > SSIZE_MAX) > + return -1; > if (fd == -1) > return -1; > - err = read(fd, dst, sizeof(*dst)); > + err = read(fd, dst, len); > close(fd); > > return err; > -#else > - return -1; > #endif > } > > @@ -118,29 +118,41 @@ static uint32_t get_generic_seed(void) > return AV_RB32(digest) + AV_RB32(digest + 16); > } > > -uint32_t av_get_random_seed(void) > +int av_random(uint8_t* buf, size_t len) > { > - uint32_t seed; > - > #if HAVE_BCRYPT > BCRYPT_ALG_HANDLE algo_handle; > NTSTATUS ret = BCryptOpenAlgorithmProvider(&algo_handle, BCRYPT_RNG_ALGORITHM, > MS_PRIMITIVE_PROVIDER, 0); > if (BCRYPT_SUCCESS(ret)) { > - NTSTATUS ret = BCryptGenRandom(algo_handle, (UCHAR*)&seed, sizeof(seed), 0); > + NTSTATUS ret = BCryptGenRandom(algo_handle, (PUCHAR)buf, len, 0); > BCryptCloseAlgorithmProvider(algo_handle, 0); > if (BCRYPT_SUCCESS(ret)) > - return seed; > + return 0; > } > #endif > > #if HAVE_ARC4RANDOM > - return arc4random(); > + arc4random_buf(buf, len); > + return 0; > +#endif > + > +#if HAVE_UNISTD_H > + if (read_random(buf, len, "/dev/urandom") == len) > + return 0; > + if (read_random(buf, len, "/dev/random") == len) > + return 0; > #endif > > - if (read_random(&seed, "/dev/urandom") == sizeof(seed)) > - return seed; > - if (read_random(&seed, "/dev/random") == sizeof(seed)) > - return seed; > - return get_generic_seed(); > + return AVERROR_INVALIDDATA; Probably AVERROR(ENOSYS) is a more suiting error code for this. Regards, Marton
On 7/4/2023 4:34 PM, Marton Balint wrote: > > > On Tue, 4 Jul 2023, James Almer wrote: > >> Uses the existing code for av_get_random_seed() to return a buffer with >> cryptographically secure random data, or an error if none could be >> generated. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> TODO: APIChanges entry and minor version bump. >> >> Also, if a new random.h header is prefered, i can move the prototype >> there. >> >> libavutil/random_seed.c | 46 ++++++++++++++++++++++++++--------------- >> libavutil/random_seed.h | 12 +++++++++++ >> 2 files changed, 41 insertions(+), 17 deletions(-) >> >> diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c >> index 66dd504ef0..39fb27c5ad 100644 >> --- a/libavutil/random_seed.c >> +++ b/libavutil/random_seed.c >> @@ -46,20 +46,20 @@ >> #define TEST 0 >> #endif >> >> -static int read_random(uint32_t *dst, const char *file) >> -{ >> #if HAVE_UNISTD_H >> +static ssize_t read_random(uint8_t *dst, size_t len, const char *file) >> +{ >> int fd = avpriv_open(file, O_RDONLY); >> - int err = -1; >> + ssize_t err = -1; >> >> + if (len > SSIZE_MAX) >> + return -1; >> if (fd == -1) >> return -1; >> - err = read(fd, dst, sizeof(*dst)); >> + err = read(fd, dst, len); >> close(fd); >> >> return err; >> -#else >> - return -1; >> #endif >> } >> >> @@ -118,29 +118,41 @@ static uint32_t get_generic_seed(void) >> return AV_RB32(digest) + AV_RB32(digest + 16); >> } >> >> -uint32_t av_get_random_seed(void) >> +int av_random(uint8_t* buf, size_t len) >> { >> - uint32_t seed; >> - >> #if HAVE_BCRYPT >> BCRYPT_ALG_HANDLE algo_handle; >> NTSTATUS ret = BCryptOpenAlgorithmProvider(&algo_handle, >> BCRYPT_RNG_ALGORITHM, >> MS_PRIMITIVE_PROVIDER, 0); >> if (BCRYPT_SUCCESS(ret)) { >> - NTSTATUS ret = BCryptGenRandom(algo_handle, (UCHAR*)&seed, >> sizeof(seed), 0); >> + NTSTATUS ret = BCryptGenRandom(algo_handle, (PUCHAR)buf, len, >> 0); >> BCryptCloseAlgorithmProvider(algo_handle, 0); >> if (BCRYPT_SUCCESS(ret)) >> - return seed; >> + return 0; >> } >> #endif >> >> #if HAVE_ARC4RANDOM >> - return arc4random(); >> + arc4random_buf(buf, len); >> + return 0; >> +#endif >> + >> +#if HAVE_UNISTD_H >> + if (read_random(buf, len, "/dev/urandom") == len) >> + return 0; >> + if (read_random(buf, len, "/dev/random") == len) >> + return 0; >> #endif >> >> - if (read_random(&seed, "/dev/urandom") == sizeof(seed)) >> - return seed; >> - if (read_random(&seed, "/dev/random") == sizeof(seed)) >> - return seed; >> - return get_generic_seed(); >> + return AVERROR_INVALIDDATA; > > Probably AVERROR(ENOSYS) is a more suiting error code for this. Not if any of the functions above were called but failed to fill the buffer. I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and return AVERROR_INVALIDDATA outside.
> > Not if any of the functions above were called but failed to fill the buffer. > > I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and > return AVERROR_INVALIDDATA outside. AVERROR_INVALIDDATA is defined as 'Invalid data found when processing input'. This function does not process any input, so IMO that code never makes sense for it. I'd say make it ENOSYS, AVERROR_UNKNOWN, or keep around one of individual method errors.
On 7/4/2023 4:59 PM, Anton Khirnov wrote: >> >> Not if any of the functions above were called but failed to fill the buffer. >> >> I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and >> return AVERROR_INVALIDDATA outside. > > AVERROR_INVALIDDATA is defined as 'Invalid data found when processing > input'. > This function does not process any input, so IMO that code never makes > sense for it. > > I'd say make it ENOSYS, AVERROR_UNKNOWN, or keep around one of > individual method errors. For the cases read() is used for /dev/random/, i can return AVERROR(errno), given the doxy states read() returns -1 on error and sets errno to some value. Although if it succeeds and returns a value smaller than len i would need to return AVERROR_UNKNOWN like you suggested. RAND_bytes from OpenSSL returns 0 or -1 on error, so nothing i can propagate.
Quoting James Almer (2023-07-04 22:08:40) > On 7/4/2023 4:59 PM, Anton Khirnov wrote: > >> > >> Not if any of the functions above were called but failed to fill the buffer. > >> > >> I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and > >> return AVERROR_INVALIDDATA outside. > > > > AVERROR_INVALIDDATA is defined as 'Invalid data found when processing > > input'. > > This function does not process any input, so IMO that code never makes > > sense for it. > > > > I'd say make it ENOSYS, AVERROR_UNKNOWN, or keep around one of > > individual method errors. > > For the cases read() is used for /dev/random/, i can return > AVERROR(errno), given the doxy states read() returns -1 on error and > sets errno to some value. Although if it succeeds and returns a value > smaller than len i would need to return AVERROR_UNKNOWN like you suggested. I wonder if we should be trying /dev/random at all. I cannot think of any remotely sane configuration where /dev/urandom fails, but /dev/random works. So it only makes sense to use /dev/urandom.
Quoting James Almer (2023-07-04 22:08:40) > > RAND_bytes from OpenSSL returns 0 or -1 on error, so nothing i can > propagate. That's AVERROR_EXTERNAL then.
On 7/4/2023 5:14 PM, Anton Khirnov wrote: > Quoting James Almer (2023-07-04 22:08:40) >> On 7/4/2023 4:59 PM, Anton Khirnov wrote: >>>> >>>> Not if any of the functions above were called but failed to fill the buffer. >>>> >>>> I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and >>>> return AVERROR_INVALIDDATA outside. >>> >>> AVERROR_INVALIDDATA is defined as 'Invalid data found when processing >>> input'. >>> This function does not process any input, so IMO that code never makes >>> sense for it. >>> >>> I'd say make it ENOSYS, AVERROR_UNKNOWN, or keep around one of >>> individual method errors. >> >> For the cases read() is used for /dev/random/, i can return >> AVERROR(errno), given the doxy states read() returns -1 on error and >> sets errno to some value. Although if it succeeds and returns a value >> smaller than len i would need to return AVERROR_UNKNOWN like you suggested. > > I wonder if we should be trying /dev/random at all. I cannot think of > any remotely sane configuration where /dev/urandom fails, but > /dev/random works. So it only makes sense to use /dev/urandom. I think the problem is that one may not generate the requested amount of bytes.
Quoting James Almer (2023-07-04 22:18:40) > On 7/4/2023 5:14 PM, Anton Khirnov wrote: > > Quoting James Almer (2023-07-04 22:08:40) > >> On 7/4/2023 4:59 PM, Anton Khirnov wrote: > >>>> > >>>> Not if any of the functions above were called but failed to fill the buffer. > >>>> > >>>> I could add return AVERROR(ENOSYS) to the HAVE_UNISTD_H block, and > >>>> return AVERROR_INVALIDDATA outside. > >>> > >>> AVERROR_INVALIDDATA is defined as 'Invalid data found when processing > >>> input'. > >>> This function does not process any input, so IMO that code never makes > >>> sense for it. > >>> > >>> I'd say make it ENOSYS, AVERROR_UNKNOWN, or keep around one of > >>> individual method errors. > >> > >> For the cases read() is used for /dev/random/, i can return > >> AVERROR(errno), given the doxy states read() returns -1 on error and > >> sets errno to some value. Although if it succeeds and returns a value > >> smaller than len i would need to return AVERROR_UNKNOWN like you suggested. > > > > I wonder if we should be trying /dev/random at all. I cannot think of > > any remotely sane configuration where /dev/urandom fails, but > > /dev/random works. So it only makes sense to use /dev/urandom. > > I think the problem is that one may not generate the requested amount of > bytes. It may happen that /dev/random blocks, but /dev/urandom should not. On Linux, the manpage says: Since Linux 3.16, a read(2) from /dev/urandom will return at most 32 MB. A read(2) from /dev/random will return at most 512 bytes (340 bytes be‐ fore Linux 2.6.12). so we may want to read in a loop if we want to handle callers requesting very large amounts of data (though I can't think of a valid use case for that). But however it may be, it should never happen that /dev/urandom is not usable, but /dev/random is.
diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c index 66dd504ef0..39fb27c5ad 100644 --- a/libavutil/random_seed.c +++ b/libavutil/random_seed.c @@ -46,20 +46,20 @@ #define TEST 0 #endif -static int read_random(uint32_t *dst, const char *file) -{ #if HAVE_UNISTD_H +static ssize_t read_random(uint8_t *dst, size_t len, const char *file) +{ int fd = avpriv_open(file, O_RDONLY); - int err = -1; + ssize_t err = -1; + if (len > SSIZE_MAX) + return -1; if (fd == -1) return -1; - err = read(fd, dst, sizeof(*dst)); + err = read(fd, dst, len); close(fd); return err; -#else - return -1; #endif } @@ -118,29 +118,41 @@ static uint32_t get_generic_seed(void) return AV_RB32(digest) + AV_RB32(digest + 16); } -uint32_t av_get_random_seed(void) +int av_random(uint8_t* buf, size_t len) { - uint32_t seed; - #if HAVE_BCRYPT BCRYPT_ALG_HANDLE algo_handle; NTSTATUS ret = BCryptOpenAlgorithmProvider(&algo_handle, BCRYPT_RNG_ALGORITHM, MS_PRIMITIVE_PROVIDER, 0); if (BCRYPT_SUCCESS(ret)) { - NTSTATUS ret = BCryptGenRandom(algo_handle, (UCHAR*)&seed, sizeof(seed), 0); + NTSTATUS ret = BCryptGenRandom(algo_handle, (PUCHAR)buf, len, 0); BCryptCloseAlgorithmProvider(algo_handle, 0); if (BCRYPT_SUCCESS(ret)) - return seed; + return 0; } #endif #if HAVE_ARC4RANDOM - return arc4random(); + arc4random_buf(buf, len); + return 0; +#endif + +#if HAVE_UNISTD_H + if (read_random(buf, len, "/dev/urandom") == len) + return 0; + if (read_random(buf, len, "/dev/random") == len) + return 0; #endif - if (read_random(&seed, "/dev/urandom") == sizeof(seed)) - return seed; - if (read_random(&seed, "/dev/random") == sizeof(seed)) - return seed; - return get_generic_seed(); + return AVERROR_INVALIDDATA; +} + +uint32_t av_get_random_seed(void) +{ + uint32_t seed; + + if (av_random((uint8_t *)&seed, sizeof(seed)) < 0) + return get_generic_seed(); + + return seed; } diff --git a/libavutil/random_seed.h b/libavutil/random_seed.h index 0462a048e0..ce982bb82f 100644 --- a/libavutil/random_seed.h +++ b/libavutil/random_seed.h @@ -36,6 +36,18 @@ */ uint32_t av_get_random_seed(void); +/** + * Generate cryptographically secure random data, i.e. suitable for use as + * encryption keys and similar. + * + * @param buf buffer into which the random data will be written + * @param len size of buf in bytes + * + * @retval 0 success, and len bytes of random data was written into buf, or + * a negative AVERROR code if random data could not be generated. + */ +int av_random(uint8_t* buf, size_t len); + /** * @} */
Uses the existing code for av_get_random_seed() to return a buffer with cryptographically secure random data, or an error if none could be generated. Signed-off-by: James Almer <jamrial@gmail.com> --- TODO: APIChanges entry and minor version bump. Also, if a new random.h header is prefered, i can move the prototype there. libavutil/random_seed.c | 46 ++++++++++++++++++++++++++--------------- libavutil/random_seed.h | 12 +++++++++++ 2 files changed, 41 insertions(+), 17 deletions(-)