[FFmpeg-devel] [PATCH v3 2/2] avformat: add data_size for ff_hex_to_data()
lance.lmwang at gmail.com
lance.lmwang at gmail.com
Tue May 11 09:27:16 EEST 2021
On Tue, May 11, 2021 at 05:15:46AM +0200, Andreas Rheinhardt wrote:
> 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.)
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
--
Thanks,
Limin Wang
More information about the ffmpeg-devel
mailing list