[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 05:39:30 EEST 2021
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
>
> >
> >>>> 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