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

[Andreas Rheinhardt]

lance.lmwang at gmail.com:
> On Tue, May 11, 2021 at 03:45:51AM +0200, Andreas Rheinhardt wrote:
>> lance.lmwang at gmail.com:
>>> On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
>>>> lance.lmwang at gmail.com:
>>>>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>>>>>> lance.lmwang at gmail.com:
>>>>>>> From: Limin Wang <lance.lmwang at 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 at 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;
>>>>>>>          }
>>>>>>>
>>>>>>
>>