Skip to content

Commit

Permalink
SNMP: Fix two undefined behaviors
Browse files Browse the repository at this point in the history
When converting an integer from ASN.1, use an unsigned value
for the partial result and assign it to the integer part of
the union at the end, to avoid shifting a negative number left.

print-snmp.c:545:19: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:545:19

OID elements are unsigned; a large-enough oid value could result
in the undefined behavior of shifting a signed integer left through
the sign bit, so simply store them as unsigned.

print-snmp.c:751:11: runtime error: left shift of 268435455 by 7 places
  cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:751:11

[Part of the PR the-tcpdump-group#1012]
  • Loading branch information
fenner authored and fxlb committed Apr 19, 2023
1 parent 541c1ab commit df5cba8
Show file tree
Hide file tree
Showing 7 changed files with 12 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGES
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ DayOfTheWeek, Month DD, YYYY / The Tcpdump Group
BGP: Add support for BGP Role capability and OTC attribute
Arista: Use the test .pcap file from pull request #955 (HwInfo).
NFLOG: Use correct AF code points on all OSes.
SNMP: Fix two undefined behaviors.
User interface:
Add optional unit suffix on -C file size.
Add --print-sampling to print every Nth packet instead of all.
Expand Down
8 changes: 5 additions & 3 deletions print-snmp.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@

#include <stdio.h>
#include <string.h>
#include <limits.h>

#ifdef USE_LIBSMI
#include <smi.h>
Expand Down Expand Up @@ -531,7 +532,7 @@ asn1_parse(netdissect_options *ndo,
break;

case INTEGER: {
int32_t data;
uint32_t data;
elem->type = BE_INT;
data = 0;

Expand All @@ -540,7 +541,7 @@ asn1_parse(netdissect_options *ndo,
goto invalid;
}
if (GET_U_1(p) & ASN_BIT8) /* negative */
data = -1;
data = UINT_MAX;
for (i = elem->asnlen; i != 0; p++, i--)
data = (data << ASN_SHIFT8) | GET_U_1(p);
elem->data.integer = data;
Expand Down Expand Up @@ -726,7 +727,8 @@ asn1_print(netdissect_options *ndo,
break;

case BE_OID: {
int o = 0, first = -1;
int first = -1;
uint32_t o = 0;

p = (const u_char *)elem->data.raw;
i = asnlen;
Expand Down
4 changes: 4 additions & 0 deletions tests/TESTLIST
Original file line number Diff line number Diff line change
Expand Up @@ -918,3 +918,7 @@ NHRP_registration NHRP_registration.pcap NHRP_registration.out -v
NHRP-responder-address NHRP-responder-address.pcap NHRP-responder-address.out -v
nhrp-trace nhrp-trace.pcap nhrp-trace.out -v
nhrp nhrp.pcap nhrp.out -v

# Undefined behavior tests
ip-snmp-leftshift-unsigned ip-snmp-leftshift-unsigned.pcap ip-snmp-leftshift-unsigned.out
ip6-snmp-oid-unsigned ip6-snmp-oid-unsigned.pcap ip6-snmp-oid-unsigned.out
1 change: 1 addition & 0 deletions tests/ip-snmp-leftshift-unsigned.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1 14:35:45.695106 IP truncated-ip - 34734 bytes missing! 10.0.0.0.162 > 154.1.214.234.65535: [!init SEQ]-1
Binary file added tests/ip-snmp-leftshift-unsigned.pcap
Binary file not shown.
1 change: 1 addition & 0 deletions tests/ip6-snmp-oid-unsigned.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1 12:36:48.416500 IP6 fe80::20c:29ff:fe9b:a15d.161 > fe80::20c:0:0:0.546: [!init SEQ].1.11.1.99.0.0.0.0.0.0.4.4.71.8327.1936855.0.1.0.14.0.1.0.1.24.14347.63.0.12.41.57.14824.0.2.0.14.0.1.0.1.24.1821339.0.12.41.446675.0.56.0.61.0.11.0.16.42.1.0.0.4294967168.0.0.0.0.0.0.0.0.0.0.1.0.2.3.110.116.112.7.101.120.97
Binary file added tests/ip6-snmp-oid-unsigned.pcap
Binary file not shown.

0 comments on commit df5cba8

Please sign in to comment.