CVE-2017-11665: Improper Input Validation (CWE-20)
value: The ff_amf_get_field_value function in libavformat/rtmppkt.c in FFmpeg 3.3.2 allows remote RTMP servers to cause a denial of service (Segmentation Violation and application crash) via a crafted stream.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 7.5
Published: 2017-07-27T12:29Z
Repository: https://github.com/FFmpeg/FFmpeg
Language: C
Files changed: libavformat/rtmppkt.c
Commit date: 2017-07-29 04:05:53 +0200
Commit message: avformat/rtmppkt: Convert ff_amf_get_field_value() to bytestream2

Fixes: out of array accesses

Found-by: JunDong Xie of Ant-financial Light-Year Security Lab
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

Diff: diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index 572e165498a..3ae605d2804 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -503,57 +503,74 @@ int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end)
         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,
+static int amf_get_field_value2(GetByteContext *gb,
                            const uint8_t *name, uint8_t *dst, int dst_size)
 {
     int namelen = strlen(name);
     int len;
 
-    while (*data != AMF_DATA_TYPE_OBJECT && data < data_end) {
-        len = ff_amf_tag_size(data, data_end);
-        if (len < 0)
-            len = data_end - data;
-        data += len;
+    while (bytestream2_peek_byte(gb) != AMF_DATA_TYPE_OBJECT && bytestream2_get_bytes_left(gb) > 0) {
+        int ret = amf_tag_skip(gb);
+        if (ret < 0)
+            return -1;
     }
-    if (data_end - data < 3)
+    if (bytestream2_get_bytes_left(gb) < 3)
         return -1;
-    data++;
+    bytestream2_get_byte(gb);
+
     for (;;) {
-        int size = bytestream_get_be16(&data);
+        int size = bytestream2_get_be16(gb);
         if (!size)
             break;
-        if (size < 0 || size >= data_end - data)
+        if (size < 0 || size >= bytestream2_get_bytes_left(gb))
             return -1;
-        data += size;
-        if (size == namelen && !memcmp(data-size, name, namelen)) {
-            switch (*data++) {
+        bytestream2_skip(gb, size);
+        if (size == namelen && !memcmp(gb->buffer-size, name, namelen)) {
+            switch (bytestream2_get_byte(gb)) {
             case AMF_DATA_TYPE_NUMBER:
-                snprintf(dst, dst_size, "%g", av_int2double(AV_RB64(data)));
+                snprintf(dst, dst_size, "%g", av_int2double(bytestream2_get_be64(gb)));
                 break;
             case AMF_DATA_TYPE_BOOL:
-                snprintf(dst, dst_size, "%s", *data ? "true" : "false");
+                snprintf(dst, dst_size, "%s", bytestream2_get_byte(gb) ? "true" : "false");
                 break;
             case AMF_DATA_TYPE_STRING:
-                len = bytestream_get_be16(&data);
-                av_strlcpy(dst, data, FFMIN(len+1, dst_size));
+                len = bytestream2_get_be16(gb);
+                if (dst_size < 1)
+                    return -1;
+                if (dst_size < len + 1)
+                    len = dst_size - 1;
+                bytestream2_get_buffer(gb, dst, len);
+                dst[len] = 0;
                 break;
             default:
                 return -1;
             }
             return 0;
         }
-        len = ff_amf_tag_size(data, data_end);
-        if (len < 0 || len >= data_end - data)
+        len = amf_tag_skip(gb);
+        if (len < 0 || bytestream2_get_bytes_left(gb) <= 0)
             return -1;
-        data += len;
     }
     return -1;
 }
 
+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)
+{
+    GetByteContext gb;
+
+    if (data >= data_end)
+        return -1;
+
+    bytestream2_init(&gb, data, data_end - data);
+
+    return amf_get_field_value2(&gb, name, dst, dst_size);
+}
+
 static const char* rtmp_packet_type(int type)
 {
     switch (type) {
     case RTMP_PT_CHUNK_SIZE:     return "chunk size";
     case RTMP_PT_BYTES_READ:     return "bytes read";

Vulnerable code: int ff_amf_get_field_value(const uint8_t *data, const uint8_t *data_end,
    while (*data != AMF_DATA_TYPE_OBJECT && data < data_end) {
        len = ff_amf_tag_size(data, data_end);
        if (len < 0)
            len = data_end - data;
        data += len;
    if (data_end - data < 3)
    data++;
        int size = bytestream_get_be16(&data);
        if (size < 0 || size >= data_end - data)
        data += size;
        if (size == namelen && !memcmp(data-size, name, namelen)) {
            switch (*data++) {
                snprintf(dst, dst_size, "%g", av_int2double(AV_RB64(data)));
                snprintf(dst, dst_size, "%s", *data ? "true" : "false");
                len = bytestream_get_be16(&data);
                av_strlcpy(dst, data, FFMIN(len+1, dst_size));
        len = ff_amf_tag_size(data, data_end);
        if (len < 0 || len >= data_end - data)
        data += len;
Fixed code: static int amf_get_field_value2(GetByteContext *gb,
    while (bytestream2_peek_byte(gb) != AMF_DATA_TYPE_OBJECT && bytestream2_get_bytes_left(gb) > 0) {
        int ret = amf_tag_skip(gb);
        if (ret < 0)
            return -1;
    if (bytestream2_get_bytes_left(gb) < 3)
    bytestream2_get_byte(gb);

        int size = bytestream2_get_be16(gb);
        if (size < 0 || size >= bytestream2_get_bytes_left(gb))
        bytestream2_skip(gb, size);
        if (size == namelen && !memcmp(gb->buffer-size, name, namelen)) {
            switch (bytestream2_get_byte(gb)) {
                snprintf(dst, dst_size, "%g", av_int2double(bytestream2_get_be64(gb)));
                snprintf(dst, dst_size, "%s", bytestream2_get_byte(gb) ? "true" : "false");
                len = bytestream2_get_be16(gb);
                if (dst_size < 1)
                    return -1;
                if (dst_size < len + 1)
                    len = dst_size - 1;
                bytestream2_get_buffer(gb, dst, len);
                dst[len] = 0;
        len = amf_tag_skip(gb);
        if (len < 0 || bytestream2_get_bytes_left(gb) <= 0)
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)
{
    GetByteContext gb;

    if (data >= data_end)
        return -1;

    bytestream2_init(&gb, data, data_end - data);

    return amf_get_field_value2(&gb, name, dst, dst_size);
}