CVE-2017-5483/SNMP: improve ASN.1 bounds checks

Kamil Frankowicz had found that truncated BE_STR and BE_SEQ ASN.1
elements could lead to an overread, from the source code it looked like
other ids could have this problem too. Move the checks introduced in
commit 72e501f out of the switch blocks to cover all ids by default.
This fixes GH#559 and GH#566.

diff --git a/print-snmp.c b/print-snmp.c
index 69b6a13f92a15c80dc80fb5d31dc1d7a5a88d547..1b096dcfe579289b90be9521dafaad86df1e136e 100644 (file)
--- a/print-snmp.c
+++ b/print-snmp.c
@@ -519,6 +519,7 @@ asn1_parse(netdissect_options *ndo,
                ND_PRINT((ndo, "[id?%c/%s/%d]", *Form[form], Class[class].name, id));
                return -1;
        }
                ND_PRINT((ndo, "[id?%c/%s/%d]", *Form[form], Class[class].name, id));
                return -1;
        }

+       ND_TCHECK2(*p, elem->asnlen);

 
        switch (form) {
        case PRIMITIVE:
 
        switch (form) {
        case PRIMITIVE:


@@ -539,7 +540,6 @@ asn1_parse(netdissect_options *ndo,
                                        ND_PRINT((ndo, "[asnlen=0]"));
                                        return -1;
                                }
                                        ND_PRINT((ndo, "[asnlen=0]"));
                                        return -1;
                                }

-                               ND_TCHECK2(*p, elem->asnlen);

                                if (*p & ASN_BIT8)      /* negative */
                                        data = -1;
                                for (i = elem->asnlen; i-- > 0; p++)
                                if (*p & ASN_BIT8)      /* negative */
                                        data = -1;
                                for (i = elem->asnlen; i-- > 0; p++)


@@ -577,7 +577,6 @@ asn1_parse(netdissect_options *ndo,
                        case GAUGE:
                        case TIMETICKS: {
                                register uint32_t data;
                        case GAUGE:
                        case TIMETICKS: {
                                register uint32_t data;

-                               ND_TCHECK2(*p, elem->asnlen);

                                elem->type = BE_UNS;
                                data = 0;
                                for (i = elem->asnlen; i-- > 0; p++)
                                elem->type = BE_UNS;
                                data = 0;
                                for (i = elem->asnlen; i-- > 0; p++)


@@ -588,7 +587,6 @@ asn1_parse(netdissect_options *ndo,
 
                        case COUNTER64: {
                                register uint64_t data64;
 
                        case COUNTER64: {
                                register uint64_t data64;

-                               ND_TCHECK2(*p, elem->asnlen);

                                elem->type = BE_UNS64;
                                data64 = 0;
                                for (i = elem->asnlen; i-- > 0; p++)
                                elem->type = BE_UNS64;
                                data64 = 0;
                                for (i = elem->asnlen; i-- > 0; p++)


@@ -627,7 +625,6 @@ asn1_parse(netdissect_options *ndo,
 
                default:
                        ND_PRINT((ndo, "[P/%s/%s]", Class[class].name, Class[class].Id[id]));
 
                default:
                        ND_PRINT((ndo, "[P/%s/%s]", Class[class].name, Class[class].Id[id]));

-                       ND_TCHECK2(*p, elem->asnlen);

                        elem->type = BE_OCTET;
                        elem->data.raw = (const uint8_t *)p;
                        break;
                        elem->type = BE_OCTET;
                        elem->data.raw = (const uint8_t *)p;
                        break;

diff --git a/tests/TESTLIST b/tests/TESTLIST
index 91c3b8a7b2b39205e90be68f1569d59d57032de0..e8856c01717313adab8a7dedeccef9874f735ab0 100644 (file)
--- a/tests/TESTLIST
+++ b/tests/TESTLIST
@@ -426,6 +426,10 @@ otv-heapoverflow-1 otv-heapoverflow-1.pcap         otv-heapoverflow-1.out          -t -c10
 otv-heapoverflow-2     otv-heapoverflow-2.pcap         otv-heapoverflow-2.out          -t -c10
 q933-heapoverflow-2    q933-heapoverflow-2.pcap        q933-heapoverflow-2.out         -t
 
 otv-heapoverflow-2     otv-heapoverflow-2.pcap         otv-heapoverflow-2.out          -t -c10
 q933-heapoverflow-2    q933-heapoverflow-2.pcap        q933-heapoverflow-2.out         -t
 

+# bad packets from Kamil Frankowicz
+snmp-heapoverflow-1    snmp-heapoverflow-1.pcap        snmp-heapoverflow-1.out         -t
+snmp-heapoverflow-2    snmp-heapoverflow-2.pcap        snmp-heapoverflow-2.out         -t
+

 # RTP tests
 # fuzzed pcap
 rtp-seg-fault-1  rtp-seg-fault-1.pcap  rtp-seg-fault-1.out  -t -v -T rtp
 # RTP tests
 # fuzzed pcap
 rtp-seg-fault-1  rtp-seg-fault-1.pcap  rtp-seg-fault-1.out  -t -v -T rtp

diff --git a/tests/snmp-heapoverflow-1.out b/tests/snmp-heapoverflow-1.out
new file mode 100644 (file)
index 0000000..b885607
--- /dev/null
+++ b/tests/snmp-heapoverflow-1.out
@@ -0,0 +1,21 @@
+30:30:30:30:30:30 > 30:30:30:30:30:30, ethertype Unknown (0x3030), length 808464432: 
+       0x0000:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0010:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0020:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0030:  3030                                     00
+30:30:30:30:30:30 > 30:30:30:30:30:30, ethertype Unknown (0x3030), length 808464432: 
+       0x0000:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0010:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0020:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0030:  3030                                     00
+30:30:30:30:30:30 > 30:30:30:30:30:30, ethertype Unknown (0x3030), length 808464432: 
+       0x0000:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0010:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0020:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0030:  3030                                     00
+30:30:30:30:30:30 > 30:30:30:30:30:30, ethertype Unknown (0x3030), length 808464432: 
+       0x0000:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0010:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0020:  3030 3030 3030 3030 3030 3030 3030 3030  0000000000000000
+       0x0030:  3030                                     00
+IP 48.48.48.48.12336 > 48.48.48.48.161:  [|snmp]

diff --git a/tests/snmp-heapoverflow-1.pcap b/tests/snmp-heapoverflow-1.pcap
new file mode 100644 (file)
index 0000000..83e5759
Binary files /dev/null and b/tests/snmp-heapoverflow-1.pcap differ

diff --git a/tests/snmp-heapoverflow-2.out b/tests/snmp-heapoverflow-2.out
new file mode 100644 (file)
index 0000000..9878915
--- /dev/null
+++ b/tests/snmp-heapoverflow-2.out
@@ -0,0 +1 @@
+IP 48.48.48.48.12336 > 48.48.48.48.162:  [|snmp]

diff --git a/tests/snmp-heapoverflow-2.pcap b/tests/snmp-heapoverflow-2.pcap
new file mode 100644 (file)
index 0000000..19fd248
Binary files /dev/null and b/tests/snmp-heapoverflow-2.pcap differ