[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 04:21:56 EEST 2021
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.
> >> 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 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".
> >
>
> _______________________________________________
> 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