| Message ID | MN2PR04MB5981C190AAD2D247A6A08428BAAA9@MN2PR04MB5981.namprd04.prod.outlook.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [FFmpeg-devel,v5,1/7] libavformat/asf: Fix handling of byte array length values | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 9/29/2021 11:58 PM, Soft Works wrote: > The value_len is an uint32 not an int32 per spec. That > value must not be truncated, neither by casting to int, nor by any > conditional checks, because at the end of get_tag, this value is > needed to move forward in parsing. When the len value gets > modified, the parsing may break. > > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > v5: Split into pieces as requested > > libavformat/asfdec_f.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > index 076b5ab147..d017fae019 100644 > --- a/libavformat/asfdec_f.c > +++ b/libavformat/asfdec_f.c > @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int type, int type2_size) > } > } > > -static void get_tag(AVFormatContext *s, const char *key, int type, int len, int type2_size) > +static void get_tag(AVFormatContext *s, const char *key, int type, uint32_t len, int type2_size) There's an av_assert0() in this function to ensure len will never be bigger than (INT_MAX - 22) / 2. And len is used as argument for avio_read(), which takes an int, not an unsigned int. So this change is not ok. > { > ASFContext *asf = s->priv_data; > char *value = NULL; > @@ -528,7 +528,7 @@ static int asf_read_ext_stream_properties(AVFormatContext *s, int64_t size) > static int asf_read_content_desc(AVFormatContext *s, int64_t size) > { > AVIOContext *pb = s->pb; > - int len1, len2, len3, len4, len5; > + uint32_t len1, len2, len3, len4, len5; > > len1 = avio_rl16(pb); > len2 = avio_rl16(pb); > @@ -602,25 +602,23 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size) > { > AVIOContext *pb = s->pb; > ASFContext *asf = s->priv_data; > - int n, stream_num, name_len_utf16, name_len_utf8, value_len; > + int n, name_len_utf8; > + uint16_t stream_num, name_len_utf16, value_type; > + uint32_t value_len; > int ret, i; > n = avio_rl16(pb); > > for (i = 0; i < n; i++) { > uint8_t *name; > - int value_type; > > avio_rl16(pb); // lang_list_index > - stream_num = avio_rl16(pb); > - name_len_utf16 = avio_rl16(pb); > - value_type = avio_rl16(pb); /* value_type */ > - value_len = avio_rl32(pb); > + stream_num = (uint16_t)avio_rl16(pb); > + name_len_utf16 = (uint16_t)avio_rl16(pb); > + value_type = (uint16_t)avio_rl16(pb); /* value_type */ > + value_len = avio_rl32(pb); > > - if (value_len < 0 || value_len > UINT16_MAX) > - return AVERROR_INVALIDDATA; > - > - name_len_utf8 = 2*name_len_utf16 + 1; > - name = av_malloc(name_len_utf8); > + name_len_utf8 = 2 * name_len_utf16 + 1; > + name = av_malloc(name_len_utf8); Unrelated cosmetics. > if (!name) > return AVERROR(ENOMEM); > >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Thursday, 30 September 2021 05:53 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix > type of value_len > > On 9/29/2021 11:58 PM, Soft Works wrote: > > The value_len is an uint32 not an int32 per spec. That > > value must not be truncated, neither by casting to int, nor by any > > conditional checks, because at the end of get_tag, this value is > > needed to move forward in parsing. When the len value gets > > modified, the parsing may break. > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > --- > > v5: Split into pieces as requested > > > > libavformat/asfdec_f.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > index 076b5ab147..d017fae019 100644 > > --- a/libavformat/asfdec_f.c > > +++ b/libavformat/asfdec_f.c > > @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int > type, int type2_size) > > } > > } > > > > -static void get_tag(AVFormatContext *s, const char *key, int type, > int len, int type2_size) > > +static void get_tag(AVFormatContext *s, const char *key, int type, > uint32_t len, int type2_size) > > There's an av_assert0() in this function to ensure len will never be > bigger than (INT_MAX - 22) / 2. And len is used as argument for > avio_read(), which takes an int, not an unsigned int. So this change > is > not ok. You missed this part: if (required_bufferlen > INT32_MAX) { av_log(s, AV_LOG_VERBOSE, "Unable to handle values > INT32_MAX in tag %s.\n", key); goto finish; } which comes before avio_read. goto finish doesn't mean to error out. It just means that value cannot be processed (even though it's valid from the ASF spec). But parsing continues now without erroring, because now we can handle those values > INT32_MAX because we supply the correct value to avio_seek() (the last statement). Also: The division by two applies to Unicode only, that's why I'm applying it only to the Unicode case. The subtraction of '22' was not required, that's why it's removed. Kind regards, softworkz
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > James Almer > Sent: Thursday, 30 September 2021 05:53 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix > type of value_len > > On 9/29/2021 11:58 PM, Soft Works wrote: > > The value_len is an uint32 not an int32 per spec. That > > value must not be truncated, neither by casting to int, nor by any > > conditional checks, because at the end of get_tag, this value is > > needed to move forward in parsing. When the len value gets > > modified, the parsing may break. > > > > Signed-off-by: softworkz <softworkz@hotmail.com> > > --- > > v5: Split into pieces as requested > > > > libavformat/asfdec_f.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > index 076b5ab147..d017fae019 100644 > > --- a/libavformat/asfdec_f.c > > +++ b/libavformat/asfdec_f.c > > @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int > type, int type2_size) > > } > > } > > > > -static void get_tag(AVFormatContext *s, const char *key, int type, > int len, int type2_size) > > +static void get_tag(AVFormatContext *s, const char *key, int type, > uint32_t len, int type2_size) > > There's an av_assert0() in this function to ensure len will never be > bigger than (INT_MAX - 22) / 2. And len is used as argument for > avio_read(), which takes an int, not an unsigned int. So this change > is > not ok. The general situation is this: The ASF spec allows sizes of various elements in its format up to a maximum of UINT32_MAX. As you have correctly noted: we can't process those sizes because some of the internal functions are taking int32 parameters rather than uint32. (e.g. with avio_read, get_id3_tag, asf_read_picture, ...) This patch can't fix that part. But what it fixes is that such values between INT32_MAX and UINT32_MAX won't break ASF parsing anymore. We emit a message that there's some metadata value that we can't read, but we skip over it and continue parsing. That's one part of what this patch does. Kind regards, softworkz
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Soft Works > Sent: Thursday, 30 September 2021 04:59 > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix type > of value_len > > The value_len is an uint32 not an int32 per spec. That > value must not be truncated, neither by casting to int, nor by any > conditional checks, because at the end of get_tag, this value is > needed to move forward in parsing. When the len value gets > modified, the parsing may break. > > Signed-off-by: softworkz <softworkz@hotmail.com> > --- > v5: Split into pieces as requested > > libavformat/asfdec_f.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > index 076b5ab147..d017fae019 100644 > --- a/libavformat/asfdec_f.c > +++ b/libavformat/asfdec_f.c > @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int > type, int type2_size) > } > } > > -static void get_tag(AVFormatContext *s, const char *key, int type, > int len, int type2_size) > +static void get_tag(AVFormatContext *s, const char *key, int type, > uint32_t len, int type2_size) > { > ASFContext *asf = s->priv_data; > char *value = NULL; > @@ -528,7 +528,7 @@ static int > asf_read_ext_stream_properties(AVFormatContext *s, int64_t size) > static int asf_read_content_desc(AVFormatContext *s, int64_t size) > { > AVIOContext *pb = s->pb; > - int len1, len2, len3, len4, len5; > + uint32_t len1, len2, len3, len4, len5; > > len1 = avio_rl16(pb); > len2 = avio_rl16(pb); > @@ -602,25 +602,23 @@ static int asf_read_metadata(AVFormatContext > *s, int64_t size) > { > AVIOContext *pb = s->pb; > ASFContext *asf = s->priv_data; > - int n, stream_num, name_len_utf16, name_len_utf8, value_len; > + int n, name_len_utf8; > + uint16_t stream_num, name_len_utf16, value_type; > + uint32_t value_len; > int ret, i; > n = avio_rl16(pb); > > for (i = 0; i < n; i++) { > uint8_t *name; > - int value_type; > > avio_rl16(pb); // lang_list_index > - stream_num = avio_rl16(pb); > - name_len_utf16 = avio_rl16(pb); > - value_type = avio_rl16(pb); /* value_type */ > - value_len = avio_rl32(pb); > + stream_num = (uint16_t)avio_rl16(pb); > + name_len_utf16 = (uint16_t)avio_rl16(pb); > + value_type = (uint16_t)avio_rl16(pb); /* value_type */ > + value_len = avio_rl32(pb); > > - if (value_len < 0 || value_len > UINT16_MAX) > - return AVERROR_INVALIDDATA; The previous two lines, which were added about a year ago, are actually the origin of this patchset: A while ago after rebasing, I noticed that album image extraction from .wma didn't work anymore in certain cases, namely: image attachments > 32kB I found those two lines and wondered, checked against the ASF spec and noticed the other flaws. Regarding those two lines, I would suppose that they might have slipped in accidentally - at least the check against UINT_16_MAX. Kind regards, softworkz
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c index 076b5ab147..d017fae019 100644 --- a/libavformat/asfdec_f.c +++ b/libavformat/asfdec_f.c @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int type, int type2_size) } } -static void get_tag(AVFormatContext *s, const char *key, int type, int len, int type2_size) +static void get_tag(AVFormatContext *s, const char *key, int type, uint32_t len, int type2_size) { ASFContext *asf = s->priv_data; char *value = NULL; @@ -528,7 +528,7 @@ static int asf_read_ext_stream_properties(AVFormatContext *s, int64_t size) static int asf_read_content_desc(AVFormatContext *s, int64_t size) { AVIOContext *pb = s->pb; - int len1, len2, len3, len4, len5; + uint32_t len1, len2, len3, len4, len5; len1 = avio_rl16(pb); len2 = avio_rl16(pb); @@ -602,25 +602,23 @@ static int asf_read_metadata(AVFormatContext *s, int64_t size) { AVIOContext *pb = s->pb; ASFContext *asf = s->priv_data; - int n, stream_num, name_len_utf16, name_len_utf8, value_len; + int n, name_len_utf8; + uint16_t stream_num, name_len_utf16, value_type; + uint32_t value_len; int ret, i; n = avio_rl16(pb); for (i = 0; i < n; i++) { uint8_t *name; - int value_type; avio_rl16(pb); // lang_list_index - stream_num = avio_rl16(pb); - name_len_utf16 = avio_rl16(pb); - value_type = avio_rl16(pb); /* value_type */ - value_len = avio_rl32(pb); + stream_num = (uint16_t)avio_rl16(pb); + name_len_utf16 = (uint16_t)avio_rl16(pb); + value_type = (uint16_t)avio_rl16(pb); /* value_type */ + value_len = avio_rl32(pb); - if (value_len < 0 || value_len > UINT16_MAX) - return AVERROR_INVALIDDATA; - - name_len_utf8 = 2*name_len_utf16 + 1; - name = av_malloc(name_len_utf8); + name_len_utf8 = 2 * name_len_utf16 + 1; + name = av_malloc(name_len_utf8); if (!name) return AVERROR(ENOMEM);
The value_len is an uint32 not an int32 per spec. That value must not be truncated, neither by casting to int, nor by any conditional checks, because at the end of get_tag, this value is needed to move forward in parsing. When the len value gets modified, the parsing may break. Signed-off-by: softworkz <softworkz@hotmail.com> --- v5: Split into pieces as requested libavformat/asfdec_f.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)