Skip to content

Commit

Permalink
CVE-2017-13033/VTP: Add more bound and length checks.
Browse files Browse the repository at this point in the history 
This fixes a buffer over-read discovered by Bhargava Shastry.

Add a test using the capture file supplied by the reporter(s), modified
so the capture file won't be rejected as an invalid capture.

Update another VTP test's .out file for this change.

Don't treate a TLV type or length of 0 as invalid; a type of 0 should
just be reported as illegal if that type isn't used, and the length is
the length of the *value*, not the length of the entire TLV, so if it's
zero there won't be an infinite loop.  (It's still not *legal*, as the
values of all the TLVs we handle are 1 16-bit word long; we added a
check for that.)

Update some comments while we're at it, to give a new URL for one Cisco
page and a non-Cisco URL for another former Cisco page (Cisco's UniverCD
pages don't seem to be available any more, and Cisco's robots.txt file
didn't allow the Wayback Machine to archive it).
  • Loading branch information
@guyharris @infrastation
guyharris authored and infrastation committed Sep 13, 2017
1 parent e0d8ee5 commit ae83295
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 64 deletions.
142 changes: 80 additions & 62 deletions print-vtp.c
View file
Expand Up @@ -13,9 +13,8 @@
* FOR A PARTICULAR PURPOSE.
*
* Reference documentation:
* http://www.cisco.com/en/US/tech/tk389/tk689/technologies_tech_note09186a0080094c52.shtml
* http://www.cisco.com/warp/public/473/21.html
* http://www.cisco.com/univercd/cc/td/doc/product/lan/trsrb/frames.htm
* http://www.cisco.com/c/en/us/support/docs/lan-switching/vtp/10558-21.html
* http://docstore.mik.ua/univercd/cc/td/doc/product/lan/trsrb/frames.htm
*
* Original code ode by Carles Kishimoto <carles.kishimoto@gmail.com>
*/
Expand All @@ -36,7 +35,7 @@
#define VTP_DOMAIN_NAME_LEN 32
#define VTP_MD5_DIGEST_LEN 16
#define VTP_UPDATE_TIMESTAMP_LEN 12
#define VTP_VLAN_INFO_OFFSET 12
#define VTP_VLAN_INFO_FIXED_PART_LEN 12 /* length of VLAN info before VLAN name */

#define VTP_SUMMARY_ADV 0x01
#define VTP_SUBSET_ADV 0x02
Expand Down Expand Up @@ -252,89 +251,108 @@ vtp_print (netdissect_options *ndo,
ND_TCHECK2(*tptr, len);

vtp_vlan = (const struct vtp_vlan_*)tptr;
if (len < VTP_VLAN_INFO_FIXED_PART_LEN)
goto trunc;
ND_TCHECK(*vtp_vlan);
ND_PRINT((ndo, "\n\tVLAN info status %s, type %s, VLAN-id %u, MTU %u, SAID 0x%08x, Name ",
tok2str(vtp_vlan_status,"Unknown",vtp_vlan->status),
tok2str(vtp_vlan_type_values,"Unknown",vtp_vlan->type),
EXTRACT_16BITS(&vtp_vlan->vlanid),
EXTRACT_16BITS(&vtp_vlan->mtu),
EXTRACT_32BITS(&vtp_vlan->index)));
fn_printzp(ndo, tptr + VTP_VLAN_INFO_OFFSET, vtp_vlan->name_len, NULL);

/*
* Vlan names are aligned to 32-bit boundaries.
*/
len -= VTP_VLAN_INFO_OFFSET + 4*((vtp_vlan->name_len + 3)/4);
tptr += VTP_VLAN_INFO_OFFSET + 4*((vtp_vlan->name_len + 3)/4);
len -= VTP_VLAN_INFO_FIXED_PART_LEN;
tptr += VTP_VLAN_INFO_FIXED_PART_LEN;
if (len < 4*((vtp_vlan->name_len + 3)/4))
goto trunc;
ND_TCHECK2(*tptr, vtp_vlan->name_len);
fn_printzp(ndo, tptr, vtp_vlan->name_len, NULL);

/*
* Vlan names are aligned to 32-bit boundaries.
*/
len -= 4*((vtp_vlan->name_len + 3)/4);
tptr += 4*((vtp_vlan->name_len + 3)/4);

/* TLV information follows */

while (len > 0) {

/*
* Cisco specs says 2 bytes for type + 2 bytes for length, take only 1
* See: http://www.cisco.com/univercd/cc/td/doc/product/lan/trsrb/frames.htm
* Cisco specs say 2 bytes for type + 2 bytes for length;
* see http://docstore.mik.ua/univercd/cc/td/doc/product/lan/trsrb/frames.htm
* However, actual packets on the wire appear to use 1
* byte for the type and 1 byte for the length, so that's
* what we do.
*/
if (len < 2)
goto trunc;
ND_TCHECK2(*tptr, 2);
type = *tptr;
tlv_len = *(tptr+1);

ND_PRINT((ndo, "\n\t\t%s (0x%04x) TLV",
tok2str(vtp_vlan_tlv_values, "Unknown", type),
type));

/*
* infinite loop check
*/
if (type == 0 || tlv_len == 0) {
if (len < tlv_len * 2 + 2) {
ND_PRINT((ndo, " (TLV goes past the end of the packet)"));
return;
}

ND_TCHECK2(*tptr, tlv_len * 2 +2);

tlv_value = EXTRACT_16BITS(tptr+2);

switch (type) {
case VTP_VLAN_STE_HOP_COUNT:
ND_PRINT((ndo, ", %u", tlv_value));
break;

case VTP_VLAN_PRUNING:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "Enabled" : "Disabled",
tlv_value));
break;

case VTP_VLAN_STP_TYPE:
ND_PRINT((ndo, ", %s (%u)",
tok2str(vtp_stp_type_values, "Unknown", tlv_value),
tlv_value));
break;

case VTP_VLAN_BRIDGE_TYPE:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "SRB" : "SRT",
tlv_value));
break;

case VTP_VLAN_BACKUP_CRF_MODE:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "Backup" : "Not backup",
tlv_value));
break;

/*
* FIXME those are the defined TLVs that lack a decoder
* you are welcome to contribute code ;-)
*/

case VTP_VLAN_SOURCE_ROUTING_RING_NUMBER:
case VTP_VLAN_SOURCE_ROUTING_BRIDGE_NUMBER:
case VTP_VLAN_PARENT_VLAN:
case VTP_VLAN_TRANS_BRIDGED_VLAN:
case VTP_VLAN_ARP_HOP_COUNT:
default:
print_unknown_data(ndo, tptr, "\n\t\t ", 2 + tlv_len*2);
break;
/*
* We assume the value is a 2-byte integer; the length is
* in units of 16-bit words.
*/
if (tlv_len != 1) {
ND_PRINT((ndo, " (invalid TLV length %u != 1)", tlv_len));
return;
} else {
tlv_value = EXTRACT_16BITS(tptr+2);

switch (type) {
case VTP_VLAN_STE_HOP_COUNT:
ND_PRINT((ndo, ", %u", tlv_value));
break;

case VTP_VLAN_PRUNING:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "Enabled" : "Disabled",
tlv_value));
break;

case VTP_VLAN_STP_TYPE:
ND_PRINT((ndo, ", %s (%u)",
tok2str(vtp_stp_type_values, "Unknown", tlv_value),
tlv_value));
break;

case VTP_VLAN_BRIDGE_TYPE:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "SRB" : "SRT",
tlv_value));
break;

case VTP_VLAN_BACKUP_CRF_MODE:
ND_PRINT((ndo, ", %s (%u)",
tlv_value == 1 ? "Backup" : "Not backup",
tlv_value));
break;

/*
* FIXME those are the defined TLVs that lack a decoder
* you are welcome to contribute code ;-)
*/

case VTP_VLAN_SOURCE_ROUTING_RING_NUMBER:
case VTP_VLAN_SOURCE_ROUTING_BRIDGE_NUMBER:
case VTP_VLAN_PARENT_VLAN:
case VTP_VLAN_TRANS_BRIDGED_VLAN:
case VTP_VLAN_ARP_HOP_COUNT:
default:
print_unknown_data(ndo, tptr, "\n\t\t ", 2 + tlv_len*2);
break;
}
}
len -= 2 + tlv_len*2;
tptr += 2 + tlv_len*2;
Expand Down
1 change: 1 addition & 0 deletions tests/TESTLIST
View file
Expand Up @@ -524,6 +524,7 @@ pgm_opts_asan_2 pgm_opts_asan_2.pcap pgm_opts_asan_2.out -v
pgm_opts_asan_3 pgm_opts_asan_3.pcap pgm_opts_asan_3.out -v
vtp_asan vtp_asan.pcap vtp_asan.out -v
vtp_asan-2 vtp_asan-2.pcap vtp_asan-2.out -v
vtp_asan-3 vtp_asan-3.pcap vtp_asan-3.out -v
icmp6_mobileprefix_asan icmp6_mobileprefix_asan.pcap icmp6_mobileprefix_asan.out -v
ip_printroute_asan ip_printroute_asan.pcap ip_printroute_asan.out -v
mobility_opt_asan mobility_opt_asan.pcap mobility_opt_asan.out -v
Expand Down
3 changes: 1 addition & 2 deletions tests/vtp_asan-2.out
View file
@@ -1,3 +1,2 @@
FRF.16 Frag, seq 193, Flags [Begin, End], UI 08! VTPv69, Message Subset advertisement (0x02), length 2126400013
Domain name: , Seq number: 0, Config Rev fb499603
VLAN info status Unknown, type TrCRF, VLAN-id 256, MTU 771, SAID 0x03030303, Name ^C^I^C[|vtp]
Domain name: , Seq number: 0, Config Rev fb499603[|vtp]
2 changes: 2 additions & 0 deletions tests/vtp_asan-3.out
View file
@@ -0,0 +1,2 @@
FRF.16 Frag, seq 193, Flags [Begin, End], UI 08! VTPv69, Message Subset advertisement (0x02), length 2126400013
Domain name: , Seq number: 0, Config Rev 4040404[|vtp]
Binary file added tests/vtp_asan-3.pcap
View file
Binary file not shown.

0 comments on commit ae83295

Please sign in to comment.