[FFmpeg-devel,v3,2/2] avformat: add data_size for ff_hex_to_data()

From: Limin Wang <lance.lmwang@gmail.com>

This prevents OOM in case of data buffer size is insufficient.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/hls.c          | 4 +++-
 libavformat/internal.h     | 6 ++++--
 libavformat/rtpdec_latm.c  | 6 ++++--
 libavformat/rtpdec_mpeg4.c | 6 ++++--
 libavformat/utils.c        | 7 +++++--
 5 files changed, 20 insertions(+), 9 deletions(-)

lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> This prevents OOM in case of data buffer size is insufficient.

OOM?

> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/hls.c          | 4 +++-
>  libavformat/internal.h     | 6 ++++--
>  libavformat/rtpdec_latm.c  | 6 ++++--
>  libavformat/rtpdec_mpeg4.c | 6 ++++--
>  libavformat/utils.c        | 7 +++++--
>  5 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 8fc6924..d7d0387 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
>              if (!strcmp(info.method, "SAMPLE-AES"))
>                  key_type = KEY_SAMPLE_AES;
>              if (!av_strncasecmp(info.iv, "0x", 2)) {
> -                ff_hex_to_data(iv, info.iv + 2);
> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> +                if (ret < 0)
> +                    goto fail;
>                  has_iv = 1;
>              }
>              av_strlcpy(key, info.uri, sizeof(key));
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index d57e63c..3192aca 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
>   * digits is ignored.
>   *
>   * @param data if non-null, the parsed data is written to this pointer
> + * @param data_size the data buffer size
>   * @param p the string to parse
> - * @return the number of bytes written (or to be written, if data is null)
> + * @return the number of bytes written (or to be written, if data is null),
> + *  or < 0 in case data_size is insufficient if data isn't null.
>   */
> -int ff_hex_to_data(uint8_t *data, const char *p);
> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);

This is unnecessary, as none of the callers need it: The rtpdec users
call ff_hex_to_data() twice, once to get the necessary size, once to
write the data. And the hls buffer is already big enough. I only see two
things that could be improved: Return size_t in ff_hex_to_data() as
that's the proper type (this includes checks in the callers for whether
the return type fit into the type of the extradata size)) and making the
size of the iv automatically match the needed size of (struct keyinfo).iv.

>  
>  /**
>   * Add packet to an AVFormatContext's packet_buffer list, determining its
> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> index 104a00a..cf1d581 100644
> --- a/libavformat/rtpdec_latm.c
> +++ b/libavformat/rtpdec_latm.c
> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
>  
>  static int parse_fmtp_config(AVStream *st, const char *value)
>  {
> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
>      GetBitContext gb;
>      uint8_t *config;
>      int audio_mux_version, same_time_framing, num_programs, num_layers;
> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
>      if (!config)
>          return AVERROR(ENOMEM);
> -    ff_hex_to_data(config, value);
> +    ret = ff_hex_to_data(config, len, value);
> +    if (ret < 0)
> +        return ret;
>      init_get_bits(&gb, config, len*8);
>      audio_mux_version = get_bits(&gb, 1);
>      same_time_framing = get_bits(&gb, 1);
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 34c7950..27751df 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
>  {
>      /* decode the hexa encoded parameter */
> -    int len = ff_hex_to_data(NULL, value), ret;
> +    int len = ff_hex_to_data(NULL, 0, value), ret;
>  
>      if ((ret = ff_alloc_extradata(par, len)) < 0)
>          return ret;
> -    ff_hex_to_data(par->extradata, value);
> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> +    if (ret < 0)
> +        return ret;>      return 0;
>  }
>  
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 9228313..dfe9f4c 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
>      return buff;
>  }
>  
> -int ff_hex_to_data(uint8_t *data, const char *p)
> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
>  {
>      int c, len, v;
>  
> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
>              break;
>          v = (v << 4) | c;
>          if (v & 0x100) {
> -            if (data)
> +            if (data) {
> +                if (len >= data_size)
> +                    return AVERROR(ERANGE);
>                  data[len] = v;
> +            }
>              len++;
>              v = 1;
>          }
>

On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > This prevents OOM in case of data buffer size is insufficient.
> 
> OOM?
Yes, it's invalid Out Of Memory access, not no memory available. 
What's your suggestion?

> 
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/hls.c          | 4 +++-
> >  libavformat/internal.h     | 6 ++++--
> >  libavformat/rtpdec_latm.c  | 6 ++++--
> >  libavformat/rtpdec_mpeg4.c | 6 ++++--
> >  libavformat/utils.c        | 7 +++++--
> >  5 files changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index 8fc6924..d7d0387 100644
> > --- a/libavformat/hls.c
> > +++ b/libavformat/hls.c
> > @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
> >              if (!strcmp(info.method, "SAMPLE-AES"))
> >                  key_type = KEY_SAMPLE_AES;
> >              if (!av_strncasecmp(info.iv, "0x", 2)) {
> > -                ff_hex_to_data(iv, info.iv + 2);
> > +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> > +                if (ret < 0)
> > +                    goto fail;
> >                  has_iv = 1;
> >              }
> >              av_strlcpy(key, info.uri, sizeof(key));
> > diff --git a/libavformat/internal.h b/libavformat/internal.h
> > index d57e63c..3192aca 100644
> > --- a/libavformat/internal.h
> > +++ b/libavformat/internal.h
> > @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
> >   * digits is ignored.
> >   *
> >   * @param data if non-null, the parsed data is written to this pointer
> > + * @param data_size the data buffer size
> >   * @param p the string to parse
> > - * @return the number of bytes written (or to be written, if data is null)
> > + * @return the number of bytes written (or to be written, if data is null),
> > + *  or < 0 in case data_size is insufficient if data isn't null.
> >   */
> > -int ff_hex_to_data(uint8_t *data, const char *p);
> > +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
> 
> This is unnecessary, as none of the callers need it: The rtpdec users
> call ff_hex_to_data() twice, once to get the necessary size, once to
> write the data. And the hls buffer is already big enough. I only see two
> things that could be improved: Return size_t in ff_hex_to_data() as
> that's the proper type (this includes checks in the callers for whether
> the return type fit into the type of the extradata size)) and making the
> size of the iv automatically match the needed size of (struct keyinfo).iv.
> 
> >  
> >  /**
> >   * Add packet to an AVFormatContext's packet_buffer list, determining its
> > diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> > index 104a00a..cf1d581 100644
> > --- a/libavformat/rtpdec_latm.c
> > +++ b/libavformat/rtpdec_latm.c
> > @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
> >  
> >  static int parse_fmtp_config(AVStream *st, const char *value)
> >  {
> > -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> > +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
> >      GetBitContext gb;
> >      uint8_t *config;
> >      int audio_mux_version, same_time_framing, num_programs, num_layers;
> > @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
> >      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
> >      if (!config)
> >          return AVERROR(ENOMEM);
> > -    ff_hex_to_data(config, value);
> > +    ret = ff_hex_to_data(config, len, value);
> > +    if (ret < 0)
> > +        return ret;
> >      init_get_bits(&gb, config, len*8);
> >      audio_mux_version = get_bits(&gb, 1);
> >      same_time_framing = get_bits(&gb, 1);
> > diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> > index 34c7950..27751df 100644
> > --- a/libavformat/rtpdec_mpeg4.c
> > +++ b/libavformat/rtpdec_mpeg4.c
> > @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
> >  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
> >  {
> >      /* decode the hexa encoded parameter */
> > -    int len = ff_hex_to_data(NULL, value), ret;
> > +    int len = ff_hex_to_data(NULL, 0, value), ret;
> >  
> >      if ((ret = ff_alloc_extradata(par, len)) < 0)
> >          return ret;
> > -    ff_hex_to_data(par->extradata, value);
> > +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> > +    if (ret < 0)
> > +        return ret;>      return 0;
> >  }
> >  
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 9228313..dfe9f4c 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
> >      return buff;
> >  }
> >  
> > -int ff_hex_to_data(uint8_t *data, const char *p)
> > +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
> >  {
> >      int c, len, v;
> >  
> > @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
> >              break;
> >          v = (v << 4) | c;
> >          if (v & 0x100) {
> > -            if (data)
> > +            if (data) {
> > +                if (len >= data_size)
> > +                    return AVERROR(ERANGE);
> >                  data[len] = v;
> > +            }
> >              len++;
> >              v = 1;
> >          }
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

lance.lmwang@gmail.com:
> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> This prevents OOM in case of data buffer size is insufficient.
>>
>> OOM?
> Yes, it's invalid Out Of Memory access, not no memory available. 
> What's your suggestion?
> 

What you mean is commonly called "buffer overflow"; OOM exclusively
means "no memory available".
I already told you what I think should be done below.

>>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>>  libavformat/hls.c          | 4 +++-
>>>  libavformat/internal.h     | 6 ++++--
>>>  libavformat/rtpdec_latm.c  | 6 ++++--
>>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
>>>  libavformat/utils.c        | 7 +++++--
>>>  5 files changed, 20 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>> index 8fc6924..d7d0387 100644
>>> --- a/libavformat/hls.c
>>> +++ b/libavformat/hls.c
>>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
>>>              if (!strcmp(info.method, "SAMPLE-AES"))
>>>                  key_type = KEY_SAMPLE_AES;
>>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
>>> -                ff_hex_to_data(iv, info.iv + 2);
>>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
>>> +                if (ret < 0)
>>> +                    goto fail;
>>>                  has_iv = 1;
>>>              }
>>>              av_strlcpy(key, info.uri, sizeof(key));
>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>> index d57e63c..3192aca 100644
>>> --- a/libavformat/internal.h
>>> +++ b/libavformat/internal.h
>>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
>>>   * digits is ignored.
>>>   *
>>>   * @param data if non-null, the parsed data is written to this pointer
>>> + * @param data_size the data buffer size
>>>   * @param p the string to parse
>>> - * @return the number of bytes written (or to be written, if data is null)
>>> + * @return the number of bytes written (or to be written, if data is null),
>>> + *  or < 0 in case data_size is insufficient if data isn't null.
>>>   */
>>> -int ff_hex_to_data(uint8_t *data, const char *p);
>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
>>
>> This is unnecessary, as none of the callers need it: The rtpdec users
>> call ff_hex_to_data() twice, once to get the necessary size, once to
>> write the data. And the hls buffer is already big enough. I only see two
>> things that could be improved: Return size_t in ff_hex_to_data() as
>> that's the proper type (this includes checks in the callers for whether
>> the return type fit into the type of the extradata size)) and making the
>> size of the iv automatically match the needed size of (struct keyinfo).iv.
>>
>>>  
>>>  /**
>>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
>>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
>>> index 104a00a..cf1d581 100644
>>> --- a/libavformat/rtpdec_latm.c
>>> +++ b/libavformat/rtpdec_latm.c
>>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
>>>  
>>>  static int parse_fmtp_config(AVStream *st, const char *value)
>>>  {
>>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
>>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
>>>      GetBitContext gb;
>>>      uint8_t *config;
>>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
>>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
>>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
>>>      if (!config)
>>>          return AVERROR(ENOMEM);
>>> -    ff_hex_to_data(config, value);
>>> +    ret = ff_hex_to_data(config, len, value);
>>> +    if (ret < 0)
>>> +        return ret;
>>>      init_get_bits(&gb, config, len*8);
>>>      audio_mux_version = get_bits(&gb, 1);
>>>      same_time_framing = get_bits(&gb, 1);
>>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
>>> index 34c7950..27751df 100644
>>> --- a/libavformat/rtpdec_mpeg4.c
>>> +++ b/libavformat/rtpdec_mpeg4.c
>>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
>>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
>>>  {
>>>      /* decode the hexa encoded parameter */
>>> -    int len = ff_hex_to_data(NULL, value), ret;
>>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
>>>  
>>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
>>>          return ret;
>>> -    ff_hex_to_data(par->extradata, value);
>>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
>>> +    if (ret < 0)
>>> +        return ret;>      return 0;
>>>  }
>>>  
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index 9228313..dfe9f4c 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
>>>      return buff;
>>>  }
>>>  
>>> -int ff_hex_to_data(uint8_t *data, const char *p)
>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
>>>  {
>>>      int c, len, v;
>>>  
>>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
>>>              break;
>>>          v = (v << 4) | c;
>>>          if (v & 0x100) {
>>> -            if (data)
>>> +            if (data) {
>>> +                if (len >= data_size)
>>> +                    return AVERROR(ERANGE);
>>>                  data[len] = v;
>>> +            }
>>>              len++;
>>>              v = 1;
>>>          }
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>> This prevents OOM in case of data buffer size is insufficient.
> >>
> >> OOM?
> > Yes, it's invalid Out Of Memory access, not no memory available. 
> > What's your suggestion?
> > 
> 
> What you mean is commonly called "buffer overflow"; OOM exclusively
> means "no memory available".
> I already told you what I think should be done below.

Sorry, I didn't notice that.

> 
> >>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>> ---
> >>>  libavformat/hls.c          | 4 +++-
> >>>  libavformat/internal.h     | 6 ++++--
> >>>  libavformat/rtpdec_latm.c  | 6 ++++--
> >>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
> >>>  libavformat/utils.c        | 7 +++++--
> >>>  5 files changed, 20 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>> index 8fc6924..d7d0387 100644
> >>> --- a/libavformat/hls.c
> >>> +++ b/libavformat/hls.c
> >>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
> >>>              if (!strcmp(info.method, "SAMPLE-AES"))
> >>>                  key_type = KEY_SAMPLE_AES;
> >>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
> >>> -                ff_hex_to_data(iv, info.iv + 2);
> >>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> >>> +                if (ret < 0)
> >>> +                    goto fail;
> >>>                  has_iv = 1;
> >>>              }
> >>>              av_strlcpy(key, info.uri, sizeof(key));
> >>> diff --git a/libavformat/internal.h b/libavformat/internal.h
> >>> index d57e63c..3192aca 100644
> >>> --- a/libavformat/internal.h
> >>> +++ b/libavformat/internal.h
> >>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
> >>>   * digits is ignored.
> >>>   *
> >>>   * @param data if non-null, the parsed data is written to this pointer
> >>> + * @param data_size the data buffer size
> >>>   * @param p the string to parse
> >>> - * @return the number of bytes written (or to be written, if data is null)
> >>> + * @return the number of bytes written (or to be written, if data is null),
> >>> + *  or < 0 in case data_size is insufficient if data isn't null.
> >>>   */
> >>> -int ff_hex_to_data(uint8_t *data, const char *p);
> >>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
> >>
> >> This is unnecessary, as none of the callers need it: The rtpdec users
> >> call ff_hex_to_data() twice, once to get the necessary size, once to
> >> write the data. And the hls buffer is already big enough. I only see two

Yes, most of caller is call ff_hex_to_data() twice, but hls is using iv[16],
We can't assume the string is 128bit only as cracker can change the m3u8 and
make the buffer overflow. For the data may be array, so I prefer to add the
memory overflow check. In theory, it's big enough, but we can't assume it.


> >> things that could be improved: Return size_t in ff_hex_to_data() as
> >> that's the proper type (this includes checks in the callers for whether
> >> the return type fit into the type of the extradata size)) and making the
> >> size of the iv automatically match the needed size of (struct keyinfo).iv.

Maybe I think it's better to alloc in ff_hex_to_data function and return the
buffer directly.

> >>
> >>>  
> >>>  /**
> >>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
> >>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> >>> index 104a00a..cf1d581 100644
> >>> --- a/libavformat/rtpdec_latm.c
> >>> +++ b/libavformat/rtpdec_latm.c
> >>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
> >>>  
> >>>  static int parse_fmtp_config(AVStream *st, const char *value)
> >>>  {
> >>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> >>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
> >>>      GetBitContext gb;
> >>>      uint8_t *config;
> >>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
> >>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
> >>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>      if (!config)
> >>>          return AVERROR(ENOMEM);
> >>> -    ff_hex_to_data(config, value);
> >>> +    ret = ff_hex_to_data(config, len, value);
> >>> +    if (ret < 0)
> >>> +        return ret;
> >>>      init_get_bits(&gb, config, len*8);
> >>>      audio_mux_version = get_bits(&gb, 1);
> >>>      same_time_framing = get_bits(&gb, 1);
> >>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> >>> index 34c7950..27751df 100644
> >>> --- a/libavformat/rtpdec_mpeg4.c
> >>> +++ b/libavformat/rtpdec_mpeg4.c
> >>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
> >>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
> >>>  {
> >>>      /* decode the hexa encoded parameter */
> >>> -    int len = ff_hex_to_data(NULL, value), ret;
> >>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
> >>>  
> >>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
> >>>          return ret;
> >>> -    ff_hex_to_data(par->extradata, value);
> >>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> >>> +    if (ret < 0)
> >>> +        return ret;>      return 0;
> >>>  }
> >>>  
> >>> diff --git a/libavformat/utils.c b/libavformat/utils.c
> >>> index 9228313..dfe9f4c 100644
> >>> --- a/libavformat/utils.c
> >>> +++ b/libavformat/utils.c
> >>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
> >>>      return buff;
> >>>  }
> >>>  
> >>> -int ff_hex_to_data(uint8_t *data, const char *p)
> >>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
> >>>  {
> >>>      int c, len, v;
> >>>  
> >>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
> >>>              break;
> >>>          v = (v << 4) | c;
> >>>          if (v & 0x100) {
> >>> -            if (data)
> >>> +            if (data) {
> >>> +                if (len >= data_size)
> >>> +                    return AVERROR(ERANGE);
> >>>                  data[len] = v;
> >>> +            }
> >>>              len++;
> >>>              v = 1;
> >>>          }
> >>>
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

lance.lmwang@gmail.com:
> On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>>>> lance.lmwang@gmail.com:
>>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>>
>>>>> This prevents OOM in case of data buffer size is insufficient.
>>>>
>>>> OOM?
>>> Yes, it's invalid Out Of Memory access, not no memory available. 
>>> What's your suggestion?
>>>
>>
>> What you mean is commonly called "buffer overflow"; OOM exclusively
>> means "no memory available".
>> I already told you what I think should be done below.
> 
> Sorry, I didn't notice that.
> 
>>
>>>>
>>>>>
>>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>>>> ---
>>>>>  libavformat/hls.c          | 4 +++-
>>>>>  libavformat/internal.h     | 6 ++++--
>>>>>  libavformat/rtpdec_latm.c  | 6 ++++--
>>>>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
>>>>>  libavformat/utils.c        | 7 +++++--
>>>>>  5 files changed, 20 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>>>> index 8fc6924..d7d0387 100644
>>>>> --- a/libavformat/hls.c
>>>>> +++ b/libavformat/hls.c
>>>>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
>>>>>              if (!strcmp(info.method, "SAMPLE-AES"))
>>>>>                  key_type = KEY_SAMPLE_AES;
>>>>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
>>>>> -                ff_hex_to_data(iv, info.iv + 2);
>>>>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
>>>>> +                if (ret < 0)
>>>>> +                    goto fail;
>>>>>                  has_iv = 1;
>>>>>              }
>>>>>              av_strlcpy(key, info.uri, sizeof(key));
>>>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>>>> index d57e63c..3192aca 100644
>>>>> --- a/libavformat/internal.h
>>>>> +++ b/libavformat/internal.h
>>>>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
>>>>>   * digits is ignored.
>>>>>   *
>>>>>   * @param data if non-null, the parsed data is written to this pointer
>>>>> + * @param data_size the data buffer size
>>>>>   * @param p the string to parse
>>>>> - * @return the number of bytes written (or to be written, if data is null)
>>>>> + * @return the number of bytes written (or to be written, if data is null),
>>>>> + *  or < 0 in case data_size is insufficient if data isn't null.
>>>>>   */
>>>>> -int ff_hex_to_data(uint8_t *data, const char *p);
>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
>>>>
>>>> This is unnecessary, as none of the callers need it: The rtpdec users
>>>> call ff_hex_to_data() twice, once to get the necessary size, once to
>>>> write the data. And the hls buffer is already big enough. I only see two
> 
> Yes, most of caller is call ff_hex_to_data() twice, but hls is using iv[16],
> We can't assume the string is 128bit only as cracker can change the m3u8 and
> make the buffer overflow. For the data may be array, so I prefer to add the
> memory overflow check. In theory, it's big enough, but we can't assume it.
> 

How could this happen? Even if the m3u8 is changed concurrently,
ff_parse_key_value() will always write a NUL-terminated string into
info.iv and said string won't change even if the m3u8 is changed.

> 
>>>> things that could be improved: Return size_t in ff_hex_to_data() as
>>>> that's the proper type (this includes checks in the callers for whether
>>>> the return type fit into the type of the extradata size)) and making the
>>>> size of the iv automatically match the needed size of (struct keyinfo).iv.
> 
> Maybe I think it's better to alloc in ff_hex_to_data function and return the
> buffer directly.
> 

This has several drawbacks: The user might know an upper bound of the
buffer size in advance, so that an allocation is unnecessary; the user
might not want a huge buffer at all (this happens with the rtp users:
they should reject lengths that don't fit into an int (anything that
comes close to that bound is probably bullshit anyway)); or the user has
special needs (this also happens with rtp: they need it as extradata,
i.e. it needs to be padded).

>>>>
>>>>>  
>>>>>  /**
>>>>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
>>>>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
>>>>> index 104a00a..cf1d581 100644
>>>>> --- a/libavformat/rtpdec_latm.c
>>>>> +++ b/libavformat/rtpdec_latm.c
>>>>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
>>>>>  
>>>>>  static int parse_fmtp_config(AVStream *st, const char *value)
>>>>>  {
>>>>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
>>>>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
>>>>>      GetBitContext gb;
>>>>>      uint8_t *config;
>>>>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
>>>>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
>>>>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>      if (!config)
>>>>>          return AVERROR(ENOMEM);
>>>>> -    ff_hex_to_data(config, value);
>>>>> +    ret = ff_hex_to_data(config, len, value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>>      init_get_bits(&gb, config, len*8);
>>>>>      audio_mux_version = get_bits(&gb, 1);
>>>>>      same_time_framing = get_bits(&gb, 1);
>>>>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
>>>>> index 34c7950..27751df 100644
>>>>> --- a/libavformat/rtpdec_mpeg4.c
>>>>> +++ b/libavformat/rtpdec_mpeg4.c
>>>>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
>>>>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
>>>>>  {
>>>>>      /* decode the hexa encoded parameter */
>>>>> -    int len = ff_hex_to_data(NULL, value), ret;
>>>>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
>>>>>  
>>>>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
>>>>>          return ret;
>>>>> -    ff_hex_to_data(par->extradata, value);
>>>>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;>      return 0;
>>>>>  }
>>>>>  
>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>>> index 9228313..dfe9f4c 100644
>>>>> --- a/libavformat/utils.c
>>>>> +++ b/libavformat/utils.c
>>>>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
>>>>>      return buff;
>>>>>  }
>>>>>  
>>>>> -int ff_hex_to_data(uint8_t *data, const char *p)
>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
>>>>>  {
>>>>>      int c, len, v;
>>>>>  
>>>>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
>>>>>              break;
>>>>>          v = (v << 4) | c;
>>>>>          if (v & 0x100) {
>>>>> -            if (data)
>>>>> +            if (data) {
>>>>> +                if (len >= data_size)
>>>>> +                    return AVERROR(ERANGE);
>>>>>                  data[len] = v;
>>>>> +            }
>>>>>              len++;
>>>>>              v = 1;
>>>>>          }
>>>>>
>>>>

On Tue, May 11, 2021 at 03:45:51AM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
> >>>> lance.lmwang@gmail.com:
> >>>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>>
> >>>>> This prevents OOM in case of data buffer size is insufficient.
> >>>>
> >>>> OOM?
> >>> Yes, it's invalid Out Of Memory access, not no memory available. 
> >>> What's your suggestion?
> >>>
> >>
> >> What you mean is commonly called "buffer overflow"; OOM exclusively
> >> means "no memory available".
> >> I already told you what I think should be done below.
> > 
> > Sorry, I didn't notice that.
> > 
> >>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>> ---
> >>>>>  libavformat/hls.c          | 4 +++-
> >>>>>  libavformat/internal.h     | 6 ++++--
> >>>>>  libavformat/rtpdec_latm.c  | 6 ++++--
> >>>>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
> >>>>>  libavformat/utils.c        | 7 +++++--
> >>>>>  5 files changed, 20 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>>>> index 8fc6924..d7d0387 100644
> >>>>> --- a/libavformat/hls.c
> >>>>> +++ b/libavformat/hls.c
> >>>>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
> >>>>>              if (!strcmp(info.method, "SAMPLE-AES"))
> >>>>>                  key_type = KEY_SAMPLE_AES;
> >>>>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
> >>>>> -                ff_hex_to_data(iv, info.iv + 2);
> >>>>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> >>>>> +                if (ret < 0)
> >>>>> +                    goto fail;
> >>>>>                  has_iv = 1;
> >>>>>              }
> >>>>>              av_strlcpy(key, info.uri, sizeof(key));
> >>>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
> >>>>> index d57e63c..3192aca 100644
> >>>>> --- a/libavformat/internal.h
> >>>>> +++ b/libavformat/internal.h
> >>>>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
> >>>>>   * digits is ignored.
> >>>>>   *
> >>>>>   * @param data if non-null, the parsed data is written to this pointer
> >>>>> + * @param data_size the data buffer size
> >>>>>   * @param p the string to parse
> >>>>> - * @return the number of bytes written (or to be written, if data is null)
> >>>>> + * @return the number of bytes written (or to be written, if data is null),
> >>>>> + *  or < 0 in case data_size is insufficient if data isn't null.
> >>>>>   */
> >>>>> -int ff_hex_to_data(uint8_t *data, const char *p);
> >>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
> >>>>
> >>>> This is unnecessary, as none of the callers need it: The rtpdec users
> >>>> call ff_hex_to_data() twice, once to get the necessary size, once to
> >>>> write the data. And the hls buffer is already big enough. I only see two
> > 
> > Yes, most of caller is call ff_hex_to_data() twice, but hls is using iv[16],
> > We can't assume the string is 128bit only as cracker can change the m3u8 and
> > make the buffer overflow. For the data may be array, so I prefer to add the
> > memory overflow check. In theory, it's big enough, but we can't assume it.
> > 
> 
> How could this happen? Even if the m3u8 is changed concurrently,
> ff_parse_key_value() will always write a NUL-terminated string into
> info.iv and said string won't change even if the m3u8 is changed.

I'm not talk about change the m3u8 after playing, in fact it can be changed before play the m3u8.
For example below is one sample, the size of IV is 16 always, but if cracker will change with
extra data, I think it'll make memory overflow.
#EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dce
-> add extra aaaa 
#EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dceaaaa


> 
> > 
> >>>> things that could be improved: Return size_t in ff_hex_to_data() as
> >>>> that's the proper type (this includes checks in the callers for whether
> >>>> the return type fit into the type of the extradata size)) and making the
> >>>> size of the iv automatically match the needed size of (struct keyinfo).iv.
> > 
> > Maybe I think it's better to alloc in ff_hex_to_data function and return the
> > buffer directly.
> > 
> 
> This has several drawbacks: The user might know an upper bound of the
> buffer size in advance, so that an allocation is unnecessary; the user
> might not want a huge buffer at all (this happens with the rtp users:
> they should reject lengths that don't fit into an int (anything that
> comes close to that bound is probably bullshit anyway)); or the user has
> special needs (this also happens with rtp: they need it as extradata,
> i.e. it needs to be padded).
> 
> >>>>
> >>>>>  
> >>>>>  /**
> >>>>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
> >>>>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> >>>>> index 104a00a..cf1d581 100644
> >>>>> --- a/libavformat/rtpdec_latm.c
> >>>>> +++ b/libavformat/rtpdec_latm.c
> >>>>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
> >>>>>  
> >>>>>  static int parse_fmtp_config(AVStream *st, const char *value)
> >>>>>  {
> >>>>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> >>>>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
> >>>>>      GetBitContext gb;
> >>>>>      uint8_t *config;
> >>>>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
> >>>>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
> >>>>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>      if (!config)
> >>>>>          return AVERROR(ENOMEM);
> >>>>> -    ff_hex_to_data(config, value);
> >>>>> +    ret = ff_hex_to_data(config, len, value);
> >>>>> +    if (ret < 0)
> >>>>> +        return ret;
> >>>>>      init_get_bits(&gb, config, len*8);
> >>>>>      audio_mux_version = get_bits(&gb, 1);
> >>>>>      same_time_framing = get_bits(&gb, 1);
> >>>>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> >>>>> index 34c7950..27751df 100644
> >>>>> --- a/libavformat/rtpdec_mpeg4.c
> >>>>> +++ b/libavformat/rtpdec_mpeg4.c
> >>>>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
> >>>>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
> >>>>>  {
> >>>>>      /* decode the hexa encoded parameter */
> >>>>> -    int len = ff_hex_to_data(NULL, value), ret;
> >>>>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
> >>>>>  
> >>>>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
> >>>>>          return ret;
> >>>>> -    ff_hex_to_data(par->extradata, value);
> >>>>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> >>>>> +    if (ret < 0)
> >>>>> +        return ret;>      return 0;
> >>>>>  }
> >>>>>  
> >>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
> >>>>> index 9228313..dfe9f4c 100644
> >>>>> --- a/libavformat/utils.c
> >>>>> +++ b/libavformat/utils.c
> >>>>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
> >>>>>      return buff;
> >>>>>  }
> >>>>>  
> >>>>> -int ff_hex_to_data(uint8_t *data, const char *p)
> >>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
> >>>>>  {
> >>>>>      int c, len, v;
> >>>>>  
> >>>>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
> >>>>>              break;
> >>>>>          v = (v << 4) | c;
> >>>>>          if (v & 0x100) {
> >>>>> -            if (data)
> >>>>> +            if (data) {
> >>>>> +                if (len >= data_size)
> >>>>> +                    return AVERROR(ERANGE);
> >>>>>                  data[len] = v;
> >>>>> +            }
> >>>>>              len++;
> >>>>>              v = 1;
> >>>>>          }
> >>>>>
> >>>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

lance.lmwang@gmail.com:
> On Tue, May 11, 2021 at 03:45:51AM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
>>>> lance.lmwang@gmail.com:
>>>>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>>>>>> lance.lmwang@gmail.com:
>>>>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>>>>
>>>>>>> This prevents OOM in case of data buffer size is insufficient.
>>>>>>
>>>>>> OOM?
>>>>> Yes, it's invalid Out Of Memory access, not no memory available. 
>>>>> What's your suggestion?
>>>>>
>>>>
>>>> What you mean is commonly called "buffer overflow"; OOM exclusively
>>>> means "no memory available".
>>>> I already told you what I think should be done below.
>>>
>>> Sorry, I didn't notice that.
>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>>>>>> ---
>>>>>>>  libavformat/hls.c          | 4 +++-
>>>>>>>  libavformat/internal.h     | 6 ++++--
>>>>>>>  libavformat/rtpdec_latm.c  | 6 ++++--
>>>>>>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
>>>>>>>  libavformat/utils.c        | 7 +++++--
>>>>>>>  5 files changed, 20 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>>>>>> index 8fc6924..d7d0387 100644
>>>>>>> --- a/libavformat/hls.c
>>>>>>> +++ b/libavformat/hls.c
>>>>>>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
>>>>>>>              if (!strcmp(info.method, "SAMPLE-AES"))
>>>>>>>                  key_type = KEY_SAMPLE_AES;
>>>>>>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
>>>>>>> -                ff_hex_to_data(iv, info.iv + 2);
>>>>>>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
>>>>>>> +                if (ret < 0)
>>>>>>> +                    goto fail;
>>>>>>>                  has_iv = 1;
>>>>>>>              }
>>>>>>>              av_strlcpy(key, info.uri, sizeof(key));
>>>>>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>>>>>> index d57e63c..3192aca 100644
>>>>>>> --- a/libavformat/internal.h
>>>>>>> +++ b/libavformat/internal.h
>>>>>>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
>>>>>>>   * digits is ignored.
>>>>>>>   *
>>>>>>>   * @param data if non-null, the parsed data is written to this pointer
>>>>>>> + * @param data_size the data buffer size
>>>>>>>   * @param p the string to parse
>>>>>>> - * @return the number of bytes written (or to be written, if data is null)
>>>>>>> + * @return the number of bytes written (or to be written, if data is null),
>>>>>>> + *  or < 0 in case data_size is insufficient if data isn't null.
>>>>>>>   */
>>>>>>> -int ff_hex_to_data(uint8_t *data, const char *p);
>>>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
>>>>>>
>>>>>> This is unnecessary, as none of the callers need it: The rtpdec users
>>>>>> call ff_hex_to_data() twice, once to get the necessary size, once to
>>>>>> write the data. And the hls buffer is already big enough. I only see two
>>>
>>> Yes, most of caller is call ff_hex_to_data() twice, but hls is using iv[16],
>>> We can't assume the string is 128bit only as cracker can change the m3u8 and
>>> make the buffer overflow. For the data may be array, so I prefer to add the
>>> memory overflow check. In theory, it's big enough, but we can't assume it.
>>>
>>
>> How could this happen? Even if the m3u8 is changed concurrently,
>> ff_parse_key_value() will always write a NUL-terminated string into
>> info.iv and said string won't change even if the m3u8 is changed.
> 
> I'm not talk about change the m3u8 after playing, in fact it can be changed before play the m3u8.
> For example below is one sample, the size of IV is 16 always, but if cracker will change with
> extra data, I think it'll make memory overflow.
> #EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dce
> -> add extra aaaa 
> #EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dceaaaa

ff_parse_key_value() already ensures not to write beyond the end of the
buffer of info.iv and to zero-terminate the buffer (if it is used at
all): In your second example, info.iv will be truncated; the "aaaa"
won't be copied; if it were otherwise, then there would already be a
buffer overflow in ff_parse_key_value().
(If you want to check for truncation, you would need to increase the
size of info.iv by one and check whether the second-to-last-element of
the array is != NUL.)

> 
> 
>>
>>>
>>>>>> things that could be improved: Return size_t in ff_hex_to_data() as
>>>>>> that's the proper type (this includes checks in the callers for whether
>>>>>> the return type fit into the type of the extradata size)) and making the
>>>>>> size of the iv automatically match the needed size of (struct keyinfo).iv.
>>>
>>> Maybe I think it's better to alloc in ff_hex_to_data function and return the
>>> buffer directly.
>>>
>>
>> This has several drawbacks: The user might know an upper bound of the
>> buffer size in advance, so that an allocation is unnecessary; the user
>> might not want a huge buffer at all (this happens with the rtp users:
>> they should reject lengths that don't fit into an int (anything that
>> comes close to that bound is probably bullshit anyway)); or the user has
>> special needs (this also happens with rtp: they need it as extradata,
>> i.e. it needs to be padded).
>>
>>>>>>
>>>>>>>  
>>>>>>>  /**
>>>>>>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
>>>>>>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
>>>>>>> index 104a00a..cf1d581 100644
>>>>>>> --- a/libavformat/rtpdec_latm.c
>>>>>>> +++ b/libavformat/rtpdec_latm.c
>>>>>>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
>>>>>>>  
>>>>>>>  static int parse_fmtp_config(AVStream *st, const char *value)
>>>>>>>  {
>>>>>>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
>>>>>>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
>>>>>>>      GetBitContext gb;
>>>>>>>      uint8_t *config;
>>>>>>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
>>>>>>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
>>>>>>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>      if (!config)
>>>>>>>          return AVERROR(ENOMEM);
>>>>>>> -    ff_hex_to_data(config, value);
>>>>>>> +    ret = ff_hex_to_data(config, len, value);
>>>>>>> +    if (ret < 0)
>>>>>>> +        return ret;
>>>>>>>      init_get_bits(&gb, config, len*8);
>>>>>>>      audio_mux_version = get_bits(&gb, 1);
>>>>>>>      same_time_framing = get_bits(&gb, 1);
>>>>>>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
>>>>>>> index 34c7950..27751df 100644
>>>>>>> --- a/libavformat/rtpdec_mpeg4.c
>>>>>>> +++ b/libavformat/rtpdec_mpeg4.c
>>>>>>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
>>>>>>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
>>>>>>>  {
>>>>>>>      /* decode the hexa encoded parameter */
>>>>>>> -    int len = ff_hex_to_data(NULL, value), ret;
>>>>>>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
>>>>>>>  
>>>>>>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
>>>>>>>          return ret;
>>>>>>> -    ff_hex_to_data(par->extradata, value);
>>>>>>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
>>>>>>> +    if (ret < 0)
>>>>>>> +        return ret;>      return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>>>>> index 9228313..dfe9f4c 100644
>>>>>>> --- a/libavformat/utils.c
>>>>>>> +++ b/libavformat/utils.c
>>>>>>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
>>>>>>>      return buff;
>>>>>>>  }
>>>>>>>  
>>>>>>> -int ff_hex_to_data(uint8_t *data, const char *p)
>>>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
>>>>>>>  {
>>>>>>>      int c, len, v;
>>>>>>>  
>>>>>>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
>>>>>>>              break;
>>>>>>>          v = (v << 4) | c;
>>>>>>>          if (v & 0x100) {
>>>>>>> -            if (data)
>>>>>>> +            if (data) {
>>>>>>> +                if (len >= data_size)
>>>>>>> +                    return AVERROR(ERANGE);
>>>>>>>                  data[len] = v;
>>>>>>> +            }
>>>>>>>              len++;
>>>>>>>              v = 1;
>>>>>>>          }
>>>>>>>
>>>>>>
>>

lance.lmwang@gmail.com (12021-05-11):
> Maybe I think it's better to alloc in ff_hex_to_data function and return the
> buffer directly.

Then certainly you are wrong. You should never suggest to use a dynamic
allocation when the code can be written with automatic allocation.
Please remember that in the future. FFmpeg is a C project that aims to
be efficient, not Python or Java.

Regards,

On Tue, May 11, 2021 at 05:15:46AM +0200, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > On Tue, May 11, 2021 at 03:45:51AM +0200, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
> >>>> lance.lmwang@gmail.com:
> >>>>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
> >>>>>> lance.lmwang@gmail.com:
> >>>>>>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>>>>>
> >>>>>>> This prevents OOM in case of data buffer size is insufficient.
> >>>>>>
> >>>>>> OOM?
> >>>>> Yes, it's invalid Out Of Memory access, not no memory available. 
> >>>>> What's your suggestion?
> >>>>>
> >>>>
> >>>> What you mean is commonly called "buffer overflow"; OOM exclusively
> >>>> means "no memory available".
> >>>> I already told you what I think should be done below.
> >>>
> >>> Sorry, I didn't notice that.
> >>>
> >>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>>>> ---
> >>>>>>>  libavformat/hls.c          | 4 +++-
> >>>>>>>  libavformat/internal.h     | 6 ++++--
> >>>>>>>  libavformat/rtpdec_latm.c  | 6 ++++--
> >>>>>>>  libavformat/rtpdec_mpeg4.c | 6 ++++--
> >>>>>>>  libavformat/utils.c        | 7 +++++--
> >>>>>>>  5 files changed, 20 insertions(+), 9 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>>>>>> index 8fc6924..d7d0387 100644
> >>>>>>> --- a/libavformat/hls.c
> >>>>>>> +++ b/libavformat/hls.c
> >>>>>>> @@ -800,7 +800,9 @@ static int parse_playlist(HLSContext *c, const char *url,
> >>>>>>>              if (!strcmp(info.method, "SAMPLE-AES"))
> >>>>>>>                  key_type = KEY_SAMPLE_AES;
> >>>>>>>              if (!av_strncasecmp(info.iv, "0x", 2)) {
> >>>>>>> -                ff_hex_to_data(iv, info.iv + 2);
> >>>>>>> +                ret = ff_hex_to_data(iv, sizeof(iv), info.iv + 2);
> >>>>>>> +                if (ret < 0)
> >>>>>>> +                    goto fail;
> >>>>>>>                  has_iv = 1;
> >>>>>>>              }
> >>>>>>>              av_strlcpy(key, info.uri, sizeof(key));
> >>>>>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
> >>>>>>> index d57e63c..3192aca 100644
> >>>>>>> --- a/libavformat/internal.h
> >>>>>>> +++ b/libavformat/internal.h
> >>>>>>> @@ -423,10 +423,12 @@ char *ff_data_to_hex(char *buf, const uint8_t *src, int size, int lowercase);
> >>>>>>>   * digits is ignored.
> >>>>>>>   *
> >>>>>>>   * @param data if non-null, the parsed data is written to this pointer
> >>>>>>> + * @param data_size the data buffer size
> >>>>>>>   * @param p the string to parse
> >>>>>>> - * @return the number of bytes written (or to be written, if data is null)
> >>>>>>> + * @return the number of bytes written (or to be written, if data is null),
> >>>>>>> + *  or < 0 in case data_size is insufficient if data isn't null.
> >>>>>>>   */
> >>>>>>> -int ff_hex_to_data(uint8_t *data, const char *p);
> >>>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p);
> >>>>>>
> >>>>>> This is unnecessary, as none of the callers need it: The rtpdec users
> >>>>>> call ff_hex_to_data() twice, once to get the necessary size, once to
> >>>>>> write the data. And the hls buffer is already big enough. I only see two
> >>>
> >>> Yes, most of caller is call ff_hex_to_data() twice, but hls is using iv[16],
> >>> We can't assume the string is 128bit only as cracker can change the m3u8 and
> >>> make the buffer overflow. For the data may be array, so I prefer to add the
> >>> memory overflow check. In theory, it's big enough, but we can't assume it.
> >>>
> >>
> >> How could this happen? Even if the m3u8 is changed concurrently,
> >> ff_parse_key_value() will always write a NUL-terminated string into
> >> info.iv and said string won't change even if the m3u8 is changed.
> > 
> > I'm not talk about change the m3u8 after playing, in fact it can be changed before play the m3u8.
> > For example below is one sample, the size of IV is 16 always, but if cracker will change with
> > extra data, I think it'll make memory overflow.
> > #EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dce
> > -> add extra aaaa 
> > #EXT-X-KEY:METHOD=AES-128,URI="https://license-server/video.key",IV=0x7b84a718bbac5e2053d64b3295ca2dceaaaa
> 
> ff_parse_key_value() already ensures not to write beyond the end of the
> buffer of info.iv and to zero-terminate the buffer (if it is used at
> all): In your second example, info.iv will be truncated; the "aaaa"
> won't be copied; if it were otherwise, then there would already be a
> buffer overflow in ff_parse_key_value().
> (If you want to check for truncation, you would need to increase the
> size of info.iv by one and check whether the second-to-last-element of
> the array is != NUL.)

Sorry, you're right, ff_parse_key_value() have checked the sizeof
info.iv. So it'll be truncated.  Please ignore the patch, I'll remove 
the first patch for the checking also.

> 
> > 
> > 
> >>
> >>>
> >>>>>> things that could be improved: Return size_t in ff_hex_to_data() as
> >>>>>> that's the proper type (this includes checks in the callers for whether
> >>>>>> the return type fit into the type of the extradata size)) and making the
> >>>>>> size of the iv automatically match the needed size of (struct keyinfo).iv.
> >>>
> >>> Maybe I think it's better to alloc in ff_hex_to_data function and return the
> >>> buffer directly.
> >>>
> >>
> >> This has several drawbacks: The user might know an upper bound of the
> >> buffer size in advance, so that an allocation is unnecessary; the user
> >> might not want a huge buffer at all (this happens with the rtp users:
> >> they should reject lengths that don't fit into an int (anything that
> >> comes close to that bound is probably bullshit anyway)); or the user has
> >> special needs (this also happens with rtp: they need it as extradata,
> >> i.e. it needs to be padded).
> >>
> >>>>>>
> >>>>>>>  
> >>>>>>>  /**
> >>>>>>>   * Add packet to an AVFormatContext's packet_buffer list, determining its
> >>>>>>> diff --git a/libavformat/rtpdec_latm.c b/libavformat/rtpdec_latm.c
> >>>>>>> index 104a00a..cf1d581 100644
> >>>>>>> --- a/libavformat/rtpdec_latm.c
> >>>>>>> +++ b/libavformat/rtpdec_latm.c
> >>>>>>> @@ -91,7 +91,7 @@ static int latm_parse_packet(AVFormatContext *ctx, PayloadContext *data,
> >>>>>>>  
> >>>>>>>  static int parse_fmtp_config(AVStream *st, const char *value)
> >>>>>>>  {
> >>>>>>> -    int len = ff_hex_to_data(NULL, value), i, ret = 0;
> >>>>>>> +    int len = ff_hex_to_data(NULL, 0, value), i, ret = 0;
> >>>>>>>      GetBitContext gb;
> >>>>>>>      uint8_t *config;
> >>>>>>>      int audio_mux_version, same_time_framing, num_programs, num_layers;
> >>>>>>> @@ -100,7 +100,9 @@ static int parse_fmtp_config(AVStream *st, const char *value)
> >>>>>>>      config = av_mallocz(len + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>>>      if (!config)
> >>>>>>>          return AVERROR(ENOMEM);
> >>>>>>> -    ff_hex_to_data(config, value);
> >>>>>>> +    ret = ff_hex_to_data(config, len, value);
> >>>>>>> +    if (ret < 0)
> >>>>>>> +        return ret;
> >>>>>>>      init_get_bits(&gb, config, len*8);
> >>>>>>>      audio_mux_version = get_bits(&gb, 1);
> >>>>>>>      same_time_framing = get_bits(&gb, 1);
> >>>>>>> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> >>>>>>> index 34c7950..27751df 100644
> >>>>>>> --- a/libavformat/rtpdec_mpeg4.c
> >>>>>>> +++ b/libavformat/rtpdec_mpeg4.c
> >>>>>>> @@ -112,11 +112,13 @@ static void close_context(PayloadContext *data)
> >>>>>>>  static int parse_fmtp_config(AVCodecParameters *par, const char *value)
> >>>>>>>  {
> >>>>>>>      /* decode the hexa encoded parameter */
> >>>>>>> -    int len = ff_hex_to_data(NULL, value), ret;
> >>>>>>> +    int len = ff_hex_to_data(NULL, 0, value), ret;
> >>>>>>>  
> >>>>>>>      if ((ret = ff_alloc_extradata(par, len)) < 0)
> >>>>>>>          return ret;
> >>>>>>> -    ff_hex_to_data(par->extradata, value);
> >>>>>>> +    ret = ff_hex_to_data(par->extradata, par->extradata_size, value);
> >>>>>>> +    if (ret < 0)
> >>>>>>> +        return ret;>      return 0;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
> >>>>>>> index 9228313..dfe9f4c 100644
> >>>>>>> --- a/libavformat/utils.c
> >>>>>>> +++ b/libavformat/utils.c
> >>>>>>> @@ -4768,7 +4768,7 @@ char *ff_data_to_hex(char *buff, const uint8_t *src, int s, int lowercase)
> >>>>>>>      return buff;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> -int ff_hex_to_data(uint8_t *data, const char *p)
> >>>>>>> +int ff_hex_to_data(uint8_t *data, int data_size, const char *p)
> >>>>>>>  {
> >>>>>>>      int c, len, v;
> >>>>>>>  
> >>>>>>> @@ -4787,8 +4787,11 @@ int ff_hex_to_data(uint8_t *data, const char *p)
> >>>>>>>              break;
> >>>>>>>          v = (v << 4) | c;
> >>>>>>>          if (v & 0x100) {
> >>>>>>> -            if (data)
> >>>>>>> +            if (data) {
> >>>>>>> +                if (len >= data_size)
> >>>>>>> +                    return AVERROR(ERANGE);
> >>>>>>>                  data[len] = v;
> >>>>>>> +            }
> >>>>>>>              len++;
> >>>>>>>              v = 1;
> >>>>>>>          }
> >>>>>>>
> >>>>>>
> >>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".