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.

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

UBSan violations in print-pflog.c and print-snmp.c #1054

Open
Lukas-Dresel opened this issue Jun 1, 2023 · 4 comments
Open

UBSan violations in print-pflog.c and print-snmp.c #1054

Lukas-Dresel opened this issue Jun 1, 2023 · 4 comments

Comments

@Lukas-Dresel
Copy link

Tcpdump version:

tcpdump-original version 5.0.0-PRE-GIT
libpcap version 1.11.0-PRE-GIT (with TPACKET_V3)
OpenSSL 1.1.1f  31 Mar 2020
Compiled with AddressSanitizer/Clang.

Fuzzing tcpdump with our hybrid fuzzer has uncovered 3 Undefined Behavior Sanitizer violations in print-pflog.c and print-snmp.c. Reproducing pcaps are included and can be tested with tcpdump -e -r <filename>

left shift out of int bounds

$ /experiments/targets/tcpdump-original -e -r /tmp/crash_print-snmp-751 
reading from file /tmp/crash_print-snmp-751, link-type SUNATM (Sun raw ATM), snapshot length 65535
[Invalid header: len(2693020224) > 262144]
[Invalid header: caplen==0, len(167772160) > 262144]
print-snmp.c:751:11: runtime error: left shift of 1141379156 by 7 places cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:751:11 in

Reproducer:
crash_print-snmp-751.pcap.tar.gz

left shift of negative value -1

$ /experiments/targets/tcpdump-original -e -r /tmp/crash_print-snmp-545.pcap 
reading from file /tmp/crash_print-snmp-545.pcap, link-type SUNATM (Sun raw ATM), snapshot length 678101060
22:56:13.134349695 Rx: VPI:0 VCI:16 ilmi:  [!init SEQ]0.0.2.7
22:56:13.134349695 Rx: VPI:0 VCI:16 ilmi:  [!init SEQ]0.0.2.7
print-snmp.c:545:19: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:545:19 in

Reproducer: crash_print-snmp-545.pcap.tar.gz

member access within misaligned address 0x61d0000000a2 for type 'const struct pfloghdr', which requires 4 byte alignment

$ /experiments/targets/tcpdump-original -e -r /tmp/crash_print-pflog-147.pcap 
reading from file /tmp/crash_print-pflog-147.pcap, link-type PPI (Per-Packet Information), snapshot length 262144
[Invalid header: len(4278255616) > 262144]
[Invalid header: len(17891584) > 262144]
[Invalid header: len(18349825) > 262144]
print-pflog.c:147:6: runtime error: member access within misaligned address 0x61d0000000a2 for type 'const struct pfloghdr', which requires 4 byte alignment
0x61d0000000a2: note: pointer points here
 00 00  00 01 01 01 54 01 01 01  be be be be be be be be  be be be be be be be be  be be be be be be
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-pflog.c:147:6 in

Reproducer:
crash_print-pflog-147.pcap.tar.gz

@fenner
Copy link
Contributor

fenner commented Jun 2, 2023

Can you see if https://github.com/the-tcpdump-group/tcpdump/pull/1012/files addresses the Snmp ub you see?

@guyharris
Copy link
Member

Can you see if https://github.com/the-tcpdump-group/tcpdump/pull/1012/files addresses the Snmp ub you see?

It does, at least on my Ubuntu 22.04 VM. Tested by reverting to a version of print-snmp.c prior to your change, building with UBSAN, reading those two files, checking out the latest print-snmp.c, rebuilding, and reading those files again.

@fxlb
Copy link
Member

fxlb commented Aug 22, 2023

Currently on Debian bookworm, CC=clang-15:

reading from file PR-1054/crash_print-snmp-545.pcap, link-type SUNATM (Sun raw ATM), snapshot length 678101060
print-snmp.c:546:19: runtime error: left shift of 4294967295 by 8 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
    [...]
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:546:19 in

print-snmp.c:547:26: runtime error: implicit conversion from type 'uint32_t' (aka 'unsigned int') of value 4294958334 (32-bit, unsigned) to type 'int32_t' (aka 'int') changed the value to -8962 (32-bit, signed)
    [...]
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:547:26 in
reading from file PR-1054/crash_print-snmp-751.pcap, link-type SUNATM (Sun raw ATM), snapshot length 65535
print-snmp.c:753:11: runtime error: left shift of 1141379156 by 7 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
    [...]
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-snmp.c:753:11 in

@fxlb fxlb reopened this Aug 22, 2023
@fxlb
Copy link
Member

fxlb commented Aug 23, 2023

Can you see if https://github.com/the-tcpdump-group/tcpdump/pull/1012/files addresses the Snmp ub you see?

Remain some UBs, see above:.

fxlb pushed a commit to fxlb/tcpdump that referenced this issue Oct 13, 2023
This 1) makes sure that GET_ macros are used to extract data from the
structure (which they already were) and 2) avoids undefined behavior if
the structure isn't aligned on the appropriate memory boundary.

Fixes the-tcpdump-group#1054.  (The SNMP issues are fixed by changes for the-tcpdump-group#1012.)

(cherry picked from commit a0b7859)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants