Message ID | 20170728134705.20300-1-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
On 7/28/2017 10:47 AM, Michael Niedermayer wrote: > Fixes: out of array accesses > Fixes: crash-9238fa9e8d4fde3beda1f279626f53812cb001cb-SEGV > > Found-by: JunDong Xie of Ant-financial Light-Year Security Lab > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/rtmppkt.c | 60 ++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 40 insertions(+), 20 deletions(-) > > diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c > index 833a3dbade..752d92a42b 100644 > --- a/libavformat/rtmppkt.c > +++ b/libavformat/rtmppkt.c > @@ -433,50 +433,70 @@ void ff_rtmp_packet_destroy(RTMPPacket *pkt) > pkt->size = 0; > } > > -int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) > +static int ff_amf_tag_skip(GetByteContext *gb) It's static, so no ff_ prefix. > { > - const uint8_t *base = data; > AMFDataType type; > unsigned nb = -1; > int parse_key = 1; > > - if (data >= data_end) > + if (bytestream2_get_bytes_left(gb) < 1) > return -1; > - switch ((type = *data++)) { > - case AMF_DATA_TYPE_NUMBER: return 9; > - case AMF_DATA_TYPE_BOOL: return 2; > - case AMF_DATA_TYPE_STRING: return 3 + AV_RB16(data); > - case AMF_DATA_TYPE_LONG_STRING: return 5 + AV_RB32(data); > - case AMF_DATA_TYPE_NULL: return 1; > - case AMF_DATA_TYPE_DATE: return 11; > + > + switch ((type = bytestream2_get_byte(gb))) { type = bytestream2_get_byte(gb); switch(type) { > + case AMF_DATA_TYPE_NUMBER: bytestream2_get_be64(gb); return 0; > + case AMF_DATA_TYPE_BOOL: bytestream2_get_byte(gb); return 0; > + case AMF_DATA_TYPE_STRING: > + bytestream2_skip(gb, bytestream2_get_be16(gb)); > + return 0; > + case AMF_DATA_TYPE_LONG_STRING: > + bytestream2_skip(gb, bytestream2_get_be32(gb)); > + return 0; > + case AMF_DATA_TYPE_NULL: return 0; > + case AMF_DATA_TYPE_DATE: bytestream2_skip(gb, 10); return 0; Use separate lines for all the above or reorder them, for readability's sake. > case AMF_DATA_TYPE_ARRAY: > parse_key = 0; > case AMF_DATA_TYPE_MIXEDARRAY: > - nb = bytestream_get_be32(&data); > + nb = bytestream2_get_be32(gb); > case AMF_DATA_TYPE_OBJECT: > while (nb-- > 0 || type != AMF_DATA_TYPE_ARRAY) { > int t; > if (parse_key) { > - int size = bytestream_get_be16(&data); > + int size = bytestream2_get_be16(gb); > if (!size) { > - data++; > + bytestream2_get_byte(gb); > break; > } > - if (size < 0 || size >= data_end - data) > + if (size < 0 || size >= bytestream2_get_bytes_left(gb)) > return -1; > - data += size; > + bytestream2_skip(gb, size); > } > - t = ff_amf_tag_size(data, data_end); > - if (t < 0 || t >= data_end - data) > + t = ff_amf_tag_skip(gb); > + if (t < 0 || bytestream2_get_bytes_left(gb) <= 0) > return -1; > - data += t; > } > - return data - base; > - case AMF_DATA_TYPE_OBJECT_END: return 1; > + return 0; > + case AMF_DATA_TYPE_OBJECT_END: return 0; > default: return -1; > } > } > > +int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) > +{ > + GetByteContext gb; > + int ret; > + > + if (data >= data_end) > + return -1; > + > + bytestream2_init(&gb, data, data_end - data); > + > + ret = ff_amf_tag_skip(&gb); > + if (ret < 0 || bytestream2_get_bytes_left(&gb) <= 0) > + return -1; > + av_assert0(bytestream2_tell(&gb) >= 0 && bytestream2_tell(&gb) <= data_end - data); > + return bytestream2_tell(&gb); > +} > + > int ff_amf_get_field_value(const uint8_t *data, const uint8_t *data_end, > const uint8_t *name, uint8_t *dst, int dst_size) > { >
On Fri, Jul 28, 2017 at 07:11:35PM -0300, James Almer wrote: > On 7/28/2017 10:47 AM, Michael Niedermayer wrote: > > Fixes: out of array accesses > > Fixes: crash-9238fa9e8d4fde3beda1f279626f53812cb001cb-SEGV > > > > Found-by: JunDong Xie of Ant-financial Light-Year Security Lab > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/rtmppkt.c | 60 ++++++++++++++++++++++++++++++++++----------------- > > 1 file changed, 40 insertions(+), 20 deletions(-) > > > > diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c > > index 833a3dbade..752d92a42b 100644 > > --- a/libavformat/rtmppkt.c > > +++ b/libavformat/rtmppkt.c > > @@ -433,50 +433,70 @@ void ff_rtmp_packet_destroy(RTMPPacket *pkt) > > pkt->size = 0; > > } > > > > -int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) > > +static int ff_amf_tag_skip(GetByteContext *gb) > > It's static, so no ff_ prefix. > > > { > > - const uint8_t *base = data; > > AMFDataType type; > > unsigned nb = -1; > > int parse_key = 1; > > > > - if (data >= data_end) > > + if (bytestream2_get_bytes_left(gb) < 1) > > return -1; > > - switch ((type = *data++)) { > > - case AMF_DATA_TYPE_NUMBER: return 9; > > - case AMF_DATA_TYPE_BOOL: return 2; > > - case AMF_DATA_TYPE_STRING: return 3 + AV_RB16(data); > > - case AMF_DATA_TYPE_LONG_STRING: return 5 + AV_RB32(data); > > - case AMF_DATA_TYPE_NULL: return 1; > > - case AMF_DATA_TYPE_DATE: return 11; > > + > > + switch ((type = bytestream2_get_byte(gb))) { > > type = bytestream2_get_byte(gb); > switch(type) { > > > + case AMF_DATA_TYPE_NUMBER: bytestream2_get_be64(gb); return 0; > > + case AMF_DATA_TYPE_BOOL: bytestream2_get_byte(gb); return 0; > > + case AMF_DATA_TYPE_STRING: > > + bytestream2_skip(gb, bytestream2_get_be16(gb)); > > + return 0; > > + case AMF_DATA_TYPE_LONG_STRING: > > + bytestream2_skip(gb, bytestream2_get_be32(gb)); > > + return 0; > > + case AMF_DATA_TYPE_NULL: return 0; > > + case AMF_DATA_TYPE_DATE: bytestream2_skip(gb, 10); return 0; > > Use separate lines for all the above or reorder them, for readability's > sake. changes made applied thanks [...]
diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c index 833a3dbade..752d92a42b 100644 --- a/libavformat/rtmppkt.c +++ b/libavformat/rtmppkt.c @@ -433,50 +433,70 @@ void ff_rtmp_packet_destroy(RTMPPacket *pkt) pkt->size = 0; } -int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) +static int ff_amf_tag_skip(GetByteContext *gb) { - const uint8_t *base = data; AMFDataType type; unsigned nb = -1; int parse_key = 1; - if (data >= data_end) + if (bytestream2_get_bytes_left(gb) < 1) return -1; - switch ((type = *data++)) { - case AMF_DATA_TYPE_NUMBER: return 9; - case AMF_DATA_TYPE_BOOL: return 2; - case AMF_DATA_TYPE_STRING: return 3 + AV_RB16(data); - case AMF_DATA_TYPE_LONG_STRING: return 5 + AV_RB32(data); - case AMF_DATA_TYPE_NULL: return 1; - case AMF_DATA_TYPE_DATE: return 11; + + switch ((type = bytestream2_get_byte(gb))) { + case AMF_DATA_TYPE_NUMBER: bytestream2_get_be64(gb); return 0; + case AMF_DATA_TYPE_BOOL: bytestream2_get_byte(gb); return 0; + case AMF_DATA_TYPE_STRING: + bytestream2_skip(gb, bytestream2_get_be16(gb)); + return 0; + case AMF_DATA_TYPE_LONG_STRING: + bytestream2_skip(gb, bytestream2_get_be32(gb)); + return 0; + case AMF_DATA_TYPE_NULL: return 0; + case AMF_DATA_TYPE_DATE: bytestream2_skip(gb, 10); return 0; case AMF_DATA_TYPE_ARRAY: parse_key = 0; case AMF_DATA_TYPE_MIXEDARRAY: - nb = bytestream_get_be32(&data); + nb = bytestream2_get_be32(gb); case AMF_DATA_TYPE_OBJECT: while (nb-- > 0 || type != AMF_DATA_TYPE_ARRAY) { int t; if (parse_key) { - int size = bytestream_get_be16(&data); + int size = bytestream2_get_be16(gb); if (!size) { - data++; + bytestream2_get_byte(gb); break; } - if (size < 0 || size >= data_end - data) + if (size < 0 || size >= bytestream2_get_bytes_left(gb)) return -1; - data += size; + bytestream2_skip(gb, size); } - t = ff_amf_tag_size(data, data_end); - if (t < 0 || t >= data_end - data) + t = ff_amf_tag_skip(gb); + if (t < 0 || bytestream2_get_bytes_left(gb) <= 0) return -1; - data += t; } - return data - base; - case AMF_DATA_TYPE_OBJECT_END: return 1; + return 0; + case AMF_DATA_TYPE_OBJECT_END: return 0; default: return -1; } } +int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) +{ + GetByteContext gb; + int ret; + + if (data >= data_end) + return -1; + + bytestream2_init(&gb, data, data_end - data); + + ret = ff_amf_tag_skip(&gb); + if (ret < 0 || bytestream2_get_bytes_left(&gb) <= 0) + return -1; + av_assert0(bytestream2_tell(&gb) >= 0 && bytestream2_tell(&gb) <= data_end - data); + return bytestream2_tell(&gb); +} + int ff_amf_get_field_value(const uint8_t *data, const uint8_t *data_end, const uint8_t *name, uint8_t *dst, int dst_size) {
Fixes: out of array accesses Fixes: crash-9238fa9e8d4fde3beda1f279626f53812cb001cb-SEGV Found-by: JunDong Xie of Ant-financial Light-Year Security Lab Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/rtmppkt.c | 60 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 20 deletions(-)