diff mbox

[FFmpeg-devel,1/2] avformat/rtmppkt: Convert ff_amf_tag_size() to bytestream2

Message ID 20170728134705.20300-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer July 28, 2017, 1:47 p.m. UTC 
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(-)

Comments

James Almer July 28, 2017, 10:11 p.m. UTC | #1 
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)
>  {
>
Michael Niedermayer July 29, 2017, 1:53 a.m. UTC | #2 
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 mbox

Patch

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)
 {