lance.lmw...@gmail.com:
> On Tue, May 11, 2021 at 01:27:09AM +0200, Andreas Rheinhardt wrote:
>> lance.lmw...@gmail.com:
>>> On Mon, May 10, 2021 at 09:52:48PM +0200, Andreas Rheinhardt wrote:
>>>> lance.lmw...@gmail.com:
>>>>> From: Limin Wang <lance.lmw...@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.lmw...@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;
>>>>> }
>>>>>
>>>>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
