Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ubsan fixes for various tcpdump printers #1012

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

ubsan fixes for various tcpdump printers #1012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ffcbd8
Bgp: Fix an undefined behavior when it tries to parse a too-short packet
fenner Oct 11, 2022
fb52e16
ISO: avoid undefined behavior and integer overflow in the fletcher ch…
fenner Oct 11, 2022
a0cf025
snmp: avoid two undefined behaviors
fenner Oct 11, 2022
2586d0a
lwres: avoid undefined behavior in pointer arithmetic
fenner Oct 11, 2022
a8dd632
OSPF6: Eliminate undefined behavior
fenner Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions checksum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ create_osi_cksum (const uint8_t *pptr, int checksum_offset, int length)

int x;
int y;
uint32_t mul;
int32_t mul;
uint32_t c0;
uint32_t c1;
uint64_t c1;
uint16_t checksum;
int idx;

Expand All @@ -138,18 +138,20 @@ create_osi_cksum (const uint8_t *pptr, int checksum_offset, int length)

mul = (length - checksum_offset)*(c0);

x = mul - c0 - c1;
y = c1 - mul - 1;

if ( y >= 0 ) y++;
if ( x < 0 ) x--;
/*
* Casting c0 and c1 here is guaranteed to be safe, because we know
* they have values between 0 and 254 inclusive. These casts are
* done to ensure that all of the arithmetic operations are
* well-defined (i.e., not mixing signed and unsigned integers).
*/
x = mul - (int)c0 - (int)c1;
y = (int)c1 - mul;

x %= 255;
y %= 255;


if (x == 0) x = 255;
if (y == 0) y = 255;
if (x <= 0) x += 255;
if (y <= 0) y += 255;

y &= 0x00FF;
checksum = ((x << 8) | y);
Expand Down
2 changes: 2 additions & 0 deletions print-bgp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,8 @@ decode_multicast_vpn(netdissect_options *ndo,
switch(route_type) {
case BGP_MULTICAST_VPN_ROUTE_TYPE_INTRA_AS_I_PMSI:
ND_TCHECK_LEN(pptr, BGP_VPN_RD_LEN);
if (route_length < BGP_VPN_RD_LEN)
goto trunc;
offset = (u_int)strlen(buf);
snprintf(buf + offset, buflen - offset, ", RD: %s, Originator %s",
bgp_vpn_rd_print(ndo, pptr),
Expand Down
4 changes: 3 additions & 1 deletion print-lwres.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,9 @@ lwres_print(netdissect_options *ndo,
if (ndo->ndo_vflag || v != LWRES_LWPACKETVERSION_0)
ND_PRINT(" v%u", v);
if (v != LWRES_LWPACKETVERSION_0) {
s = bp + GET_BE_U_4(np->length);
uint32_t pkt_len = GET_BE_U_4(np->length);
ND_TCHECK_LEN(bp, pkt_len);
s = bp + pkt_len;
goto tail;
}

Expand Down
13 changes: 11 additions & 2 deletions print-ospf6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,14 +388,23 @@ static int
ospf6_print_lshdr(netdissect_options *ndo,
const struct lsa6_hdr *lshp, const u_char *dataend)
{
u_int ls_length;

if ((const u_char *)(lshp + 1) > dataend)
goto trunc;

ls_length = GET_BE_U_2(lshp->ls_length);
if (ls_length < sizeof(struct lsa_hdr)) {
ND_PRINT("\n\t Bogus length %u < header (%zu)", ls_length,
sizeof(struct lsa_hdr));
goto trunc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perhaps more an "invalid" length case than a "truncated" case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perhaps more an "invalid" length case than a "truncated" case ?

My thinking is "this TLV doesn't have enough data to contain the header of the TLV, so, the TLV itself is truncated".

All 3 callers of ospf6_print_lshdr() use if (ospf6_print_lshdr(...)) return(1);. From a skim, I don't see a reason for any of them to distinguish between "truncated" and "invalid in this specific way".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notion of truncation may have evolved over the years, but currently a truncation occurs when we try to get len bytes in the packet buffer and we can't.
(checks via GET_*, ND_TTEST_*, ND_TCHECK_*, etc. macros). For ND_TCHECK_* macros, if ND_LONGJMP_FROM_TCHECK is defined for a printer, a longjmp occurs, no more goto trunc (Printers "Modernized" for the 5.0.0 release, remain 34 to do.)
The macros for testing invalid length cases are the ND_ICHECK*, with a goto invalid.
If we add in line 396 the following command, we get the value 470, with the test file ospf-signed-integer-ubsan.pcap:
ND_PRINT("[[%u]]", ND_BYTES_AVAILABLE_AFTER(lshp->ls_length));
Thus there is no truncation here.
Tshark says also the packet is malformed (like our invalid.), not [Packet size limited during capture: XXX truncated]:

[...]
            Link State ID: 145.245.255.254
            Advertising Router: 204.122.189.255
            Sequence Number: 0x02000000
            Checksum: 0x0000
            Length: 0
            [Expert Info (Warning/Protocol): Unknown LSA Type 224]
                [Unknown LSA Type 224]
                [Severity level: Warning]
                [Group: Protocol]
[Malformed Packet: OSPF]
    [Expert Info (Error/Malformed): Malformed Packet (Exception occurred)]
        [Malformed Packet (Exception occurred)]
        [Severity level: Error]
        [Group: Malformed]

Perhaps this change should be updated after a modernization of print-ospf6.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I'm not prepared to make a major rework as a prerequisite for my contribution, so if it's better to have undefined behavior than to print an incorrect truncation message, great, feel free to discard my changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the proposed change makes the code safer, perhaps we could address any cosmetic issues after merging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first idea was to hold the change waiting the modernization of print-ospf6.c, but as Denis says we can do the fix as a temporary change. I will commit this part. Thanks for your work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first idea was to hold the change waiting the modernization of print-ospf6.c, but as Denis says we can do the fix as a temporary change. I will commit this part. Thanks for your work.

Thank you. Undefined behavior does not necessarily lead to incorrect behavior, but, if you enable undefined behavior detection in your fuzzing environment, having undefined behavior can cause so much noise that it can prevent you from discovering real problems via fuzzing.

}

ND_PRINT("\n\t Advertising Router %s, seq 0x%08x, age %us, length %zu",
GET_IPADDR_STRING(lshp->ls_router),
GET_BE_U_4(lshp->ls_seq),
GET_BE_U_2(lshp->ls_age),
GET_BE_U_2(lshp->ls_length)-sizeof(struct lsa6_hdr));
ls_length-sizeof(struct lsa6_hdr));

ospf6_print_ls_type(ndo, GET_BE_U_2(lshp->ls_type),
&lshp->ls_stateid);
Expand Down Expand Up @@ -734,7 +743,7 @@ ospf6_decode_v3(netdissect_options *ndo,
const struct lsr6 *lsrp;
const struct lsa6_hdr *lshp;
const struct lsa6 *lsap;
int i;
uint32_t i;

switch (GET_U_1(op->ospf6_type)) {

Expand Down
8 changes: 5 additions & 3 deletions print-snmp.c
View file
Open in desktop
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
8 changes: 8 additions & 0 deletions tests/TESTLIST
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,3 +909,11 @@ quic_handshake quic_handshake.pcap quic_handshake.out -v
quic_handshake_truncated quic_handshake_truncated.pcap quic_handshake_truncated.out -v
quic_retry quic_retry.pcap quic_retry.out -v
gquic gquic.pcap gquic.out -v

# Undefined behavior tests
bgp-ub bgp-ub.pcap bgp-ub.out -v
fletcher-checksum-negative-shift fletcher-checksum-negative-shift.pcap fletcher-checksum-negative-shift.out -v
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
lwres-pointer-arithmetic-ub lwres-pointer-arithmetic-ub.pcap lwres-pointer-arithmetic-ub.out
ospf-signed-integer-ubsan ospf-signed-integer-ubsan.pcap ospf-signed-integer-ubsan.out -vv
227 changes: 227 additions & 0 deletions tests/bgp-ub.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
1 13:29:37.678220 IP (tos 0x0, ttl 64, id 1, offset 0, flags [none], proto TCP (6), length 4748)
127.0.0.1.540 > 127.0.0.1.179: Flags [S], cksum 0xf1a8 (correct), seq 0:4708, win 8192, length 4708: BGP [|bgp]
Update Message (2), length: 154
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 4456548
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:630 (= 0.0.2.118)
Cluster List (10), length: 4, Flags [O]: 172.17.0.0
Originator ID (9), length: 4, Flags [O]: 172.17.0.5
Multi-Protocol Reach NLRI (14), length: 81, Flags [OE]:
AFI: IPv4 (1), SAFI: labeled VPN Unicast (128)
nexthop: RD: 0:0 (= 0.0.0.0), 172.145.0.5, nh-length: 12, no SNPA
RD: 18826:630 (= 0.0.2.118), 172.17.30.208/28, label:1027 (bottom)
RD: 18826:630 (= 0.0.2.118), 172.17.30.224/28, label:1027 (bottom)
(illegal prefix length)
Update Message (2), length: 105
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:620 (= 0.0.2.108)
Cluster List (10), length: 37, Flags [O]: invalid len
Unknown Attribute (73), length: 138 [path attrs too short]
Update Message (2), length: 154
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:640 (= 0.0.2.128)
Cluster List (10), length: 4, Flags [O]: 172.17.0.0
Originator ID (9), length: 4, Flags [O]: 172.17.0.5
Multi-Protocol Reach NLRI (14), length: 81, Flags [OE]:
AFI: IPv4 (1), SAFI: labeled VPN Unicast (128)
nexthop: RD: 0:0 (= 0.0.0.0), 172.17.0.5, nh-length: 12, no SNPA
RD: 18826:640 (= 0.0.2.128), 172.17.33.64/28, label:1028 (bottom)
RD: 18826:640 (= 0.0.2.128), 172.17.33.80/28, label:1028 (bottom)
RD: 18826:640 (= 0.0.2.128), 172.84.34.0/28, label:132100 (bottom)
RD: 18826:549 (= 0.0.2.37), 0.17.34.16/28, label:1028 (bottom)
Update Message (2), length: 202
Withdrawn routes:
0.0.0.0/0
(illegal prefix length) [|bgp] [|bgp]
Update Message (2), length: 106
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Attribute Set (128), length: 3, Flags [O]: [|bgp] [|bgp]
Update Message (2), length: 172
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 4, Flags [T]: 64520
Multi Exit Discriminator (4), length: 4, Flags [O]: 131
Unknown Attribute (113), length: 16, Flags [T]:
no Attribute 113 decoder
0x0000: ffff ffff ffff ffff ffff ffff ffff ffff
Unknown Attribute (75), length: 2
no Attribute 75 decoder
0x0000: 0000
Unknown Attribute (47), length: 64
no Attribute 47 decoder
0x0000: 0101 0040 0206 0201 0001 0000 4003 04c0
0x0010: 0002 02c0 2018 0001 0000 0000 0001 0000
0x0020: 0001 0001 0000 0000 0001 0000 0002 20cb
0x0030: 0071 0cff ffff ffff ffff ffff ffff ffff
Reserved for development (255), length: 65280, Flags [OTPE+f]: [path attrs too short] [|bgp]
Update Message (2), length: 75
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 6, Flags [T]: 65536
Next Hop (3), length: 4, Flags [T]: 192.0.2.2
Large Community (32), length: 24, Flags [OT]:
65536:0:1, 65536:1:0
Updated routes:
203.0.113.15/32
Update Message (2), length: 87
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 6, Flags [T]: 65536
Next Hop (3), length: 4, Flags [T]: 5.12.0.0
Unknown Attribute (100), length: 192, Flags [+1]: [path attrs too short]
Updated routes:
0.0.0.0/0
(illegal prefix length) [|bgp]
Update Message (2), length: 105
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:620 (= 0.0.2.108)
Cluster List (10), length: 37, Flags [O]: invalid len
Unknown Attribute (73), length: 138 [path attrs too short]
Update Message (2), length: 154
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:640 (= 0.0.2.128)
Cluster List (10), length: 4, Flags [O]: 172.17.0.0
Originator ID (9), length: 4, Flags [O]: 172.17.0.5
Multi-Protocol Reach NLRI (14), length: 81, Flags [OE]:
AFI: IPv4 (1), SAFI: labeled VPN Unicast (128)
nexthop: RD: 0:0 (= 0.0.0.0), 172.17.0.5, nh-length: 12, no SNPA
RD: 18826:640 (= 0.0.2.128), 172.17.33.64/28, label:1028 (bottom)
RD: 18826:640 (= 0.0.2.128), 172.17.33.80/28, label:1028 (bottom)
RD: 18826:640 (= 0.0.2.128), 172.84.34.0/28, label:132100 (bottom)
RD: 18826:549 (= 0.0.2.37), 0.17.34.16/28, label:1028 (bottom)
Update Message (2), length: 202
Withdrawn routes:
0.0.0.0/0
(illegal prefix length) [|bgp] [|bgp]
Update Message (2), length: 106
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Attribute Set (128), length: 3, Flags [O]: [|bgp] [|bgp]
Update Message (2), length: 172
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 4, Flags [T]: 64520
Multi Exit Discriminator (4), length: 4, Flags [O]: 131
Unknown Attribute (113), length: 16, Flags [T]:
no Attribute 113 decoder
0x0000: ffff ffff ffff ffff ffff ffff ffff ffff
Unknown Attribute (75), length: 2
no Attribute 75 decoder
0x0000: 0000
Unknown Attribute (47), length: 64
no Attribute 47 decoder
0x0000: 0101 0040 0206 0201 0001 0000 4003 04c0
0x0010: 0002 02c0 2018 0001 0000 0000 0001 0000
0x0020: 0001 0001 0000 0000 0001 0000 0002 20cb
0x0030: 0071 0cff ffff ffff ffff ffff ffff ffff
Reserved for development (255), length: 65280, Flags [OTPE+f]: [path attrs too short] [|bgp]
Update Message (2), length: 75
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 6, Flags [T]: 65536
Next Hop (3), length: 4, Flags [T]: 192.0.2.2
Large Community (32), length: 24, Flags [OT]:
65536:0:1, 65536:1:0
Updated routes:
203.0.113.15/32
Update Message (2), length: 87
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 6, Flags [T]: 65536
Next Hop (3), length: 4, Flags [T]: 5.12.0.0
Unknown Attribute (100), length: 192, Flags [+1]: [path attrs too short]
Updated routes:
0.0.0.0/0
(illegal prefix length) [|bgp]
Update Message (2), length: 106
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:610 (= 0.0.2.98)
Cluster List (10), length: 40, Flags [O]: 24.120.4.0, 0.0.0.41, 24.120.4.1, 64.15.19.0, 1.1.0.0, 2.188.24.32, 45.0.0.0, 2.189.24.32, 45.1.64.14, 61.0.2.1
Large Community (32), length: 0, Flags [E]: invalid len
Unknown Attribute (21), length: 24 [path attrs too short] [|bgp]
Update Message (2), length: 100
Unknown Attribute (0), length: 0
no Attribute 0 decoder
Reserved for development (255), length: 255 [path attrs too short] [|bgp]
Update Message (2), length: 95
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 0, Flags [T]: empty
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 1:1 (= 0.0.0.1)
PMSI Tunnel (22), length: 17, Flags [OT]:
Tunnel-type RSVP-TE P2MP LSP (1), Flags [none], MPLS Label 0
Extended-Tunnel-ID 10.0.0.4, P2MP-ID 0x00008173
Multi-Protocol Reach NLRI (14), length: 23, Flags [OE]:
AFI: IPv4 (1), SAFI: Multicast VPN (5)
nexthop: 10.0.0.4, nh-length: 4
8 SNPA
1 bytes
0 bytes
0 bytes
0 bytes
1 bytes
0 bytes
1 bytes
0 bytes
Route-Type: Unknown (181), length: 0 [|bgp] [|bgp]
Update Message (2), length: 106
Origin (1), length: 1, Flags [T]: Incomplete
AS Path (2), length: 0, Flags [T]: empty
Multi Exit Discriminator (4), length: 4, Flags [O]: 0
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 18826:610 (= 0.0.2.98)
Cluster List (10), length: 40, Flags [O]: 24.120.4.0, 0.0.0.41, 24.120.4.1, 64.15.19.0, 1.1.0.0, 2.188.24.32, 45.0.0.0, 2.189.24.32, 45.1.64.14, 61.0.2.1
Large Community (32), length: 0, Flags [E]: invalid len
Unknown Attribute (21), length: 24 [path attrs too short] [|bgp]
Update Message (2), length: 100
Unknown Attribute (0), length: 0
no Attribute 0 decoder
Reserved for development (255), length: 255 [path attrs too short] [|bgp]
Update Message (2), length: 95
Origin (1), length: 1, Flags [T]: IGP
AS Path (2), length: 0, Flags [T]: empty
Local Preference (5), length: 4, Flags [T]: 100
Extended Community (16), length: 8, Flags [OT]:
target (0x0002), Flags [none]: 1:1 (= 0.0.0.1)
PMSI Tunnel (22), length: 17, Flags [OT]:
Tunnel-type RSVP-TE P2MP LSP (1), Flags [none], MPLS Label 0
Extended-Tunnel-ID 10.0.0.4, P2MP-ID 0x00008173
Multi-Protocol Reach NLRI (14), length: 23, Flags [OE]:
AFI: IPv4 (1), SAFI: Multicast VPN (5)
nexthop: 10.0.0.4, nh-length: 4
8 SNPA
1 bytes
0 bytes
0 bytes
0 bytes
1 bytes
0 bytes
0 bytes
1 bytes
Route-Type: Unknown (0), length: 0
Route-Type: Intra-AS Segment-Leaf (4), length: 255
Update Message (2), length: 30
Multi-Protocol Unreach NLRI (15), length: 3, Flags [OE]:
AFI: IPv4 (1), SAFI: labeled VPN Unicast (128)
End-of-Rib Marker (empty NLRI)
[|BGP Unknown Message Type]
Binary file added tests/bgp-ub.pcap
View file
Open in desktop
Binary file not shown.
Loading