ubsan fixes for various tcpdump printers

[fenner]

I ran some fuzzing with ubsan enabled. These are my fixes to the problems that this fuzzing found.

(This was kind of an accident; I didn't realize that ubsan was enabled and I got a bunch of surprise fuzz failures, but I figure these fixes are probably helpful anyway)

[infrastation]

Thank you for preparing these changes. Do you still have the UBSan messages? Does make check pass in your working copy?

[fenner]

Thank you for preparing these changes. Do you still have the UBSan messages? Does make check pass in your working copy?

I will recreate the UBSan messages, by removing the patches and rebuilding and using the test pcaps. I didn't keep records when I did this work, only pcaps. I'll put the corresponding messages in with each commit that fixes it?

The out files were generated with 4.99.1, so if there are changes in master that could explain the mismatch. I will regenerate them.

[fenner]

P.S. I will also squash the "Address integer overflows" commit into the "ISO:" commit once my discussion with fxlb is complete.

[infrastation]

Placing the messages into respective commits would be perfect, thank you!

[fenner]

The integer overflow commit is still separate, because I want to validate it against a separate implementation of this checksum implementation that I have access to.

I updated the .out files to pass with master in this commit, but I see illumos has already failed so I will investigate the check failure(s) and update.

[infrastation]

Rebased on the current master and added a fixup, the expectation is that this pull request passes all CI checks.

[fenner]

I didn't think it was worth pushing my fix for the illumos build, but I have done so now to avoid getting confused by

Your branch and 'origin/fenner-ubsan-fixes' have diverged,
and have 6 and 14 different commits each, respectively.

What's pending here is extracting the proprietary iso checksum algorithm into a form that I can test with. Once I have validated that our checksum algorithm and tcpdump's have the same result, I will squash the checksum fixes into a single commit, push that and at that point I think it will be ready to merge.

[infrastation]

Alright, thank you for the clarifications.

[fenner]

I apologize for the delay. In order to double-check that the new checksum algorithm gets the right value, I have processed all of the pcap files that @fxlb provided using this main.c, and both the (proprietary) gated and the updated tcpdump algorithm got the same value for each packet.

#include <pcap/pcap.h>

void callback(u_char *user, const struct pcap_pkthdr *h,
                                                const u_char *bytes) {
   int gated_cksum, tcpdump_cksum;
   gated_cksum = gated_iso_cksum( bytes, h->caplen, bytes );
   tcpdump_cksum = create_osi_cksum( bytes, 0, h->caplen );
   if ( gated_cksum != tcpdump_cksum ) {
      fprintf( stderr, "*** NOT MATCHING *** " );
   }
   fprintf( stderr, "gated %04x tcpdump %04x\n", gated_cksum, tcpdump_cksum );
}

int main(int argc, const char **argv) {
   char errbuf[PCAP_ERRBUF_SIZE];
   pcap_t *p;
   p = pcap_open_offline( argv[ 1 ], errbuf );
   pcap_loop( p, -1, callback, NULL );
}

[fxlb]

I have commited the fix for SNMP.
I tried to test the fix for lwres (with/without) without success.
Removing the patch, I don't get "addition of unsigned offset to xx be overflowed to yy" messages.
What are OS/compiler/configure options for this one?

[fenner]

What are OS/compiler/configure options for this one?

It's likely that some of these are specific to 32-bit builds (e.g., i386_el7).

[fxlb]

What are OS/compiler/configure options for this one?

It's likely that some of these are specific to 32-bit builds (e.g., i386_el7).

You are rigth. I need clang, sanitize undefined + address and 32-bit mode.
With it I reproduce the first error in print-lwres.c, line 294.
Was the error on line 549 obtained with the same pcap?

[fenner]

Was the error on line 549 obtained with the same pcap?

I'm sorry, I did this work over a year ago and did not expect to have to provide detailed notes. I committed all of my work to the Arista repository, and this is the only lwres pcap that I have in the Arista repository, so my best guess is that it was.

[fxlb]

I understand. Can you run the program again with the lwres pcap to check if it displays one or two error messages?

[fenner]

I understand. Can you run the program again with the lwres pcap to check if it displays one or two error messages?

I tried again (with a tcpdump-4.99.2 base) and only get the one:

print-lwres.c:294:10: runtime error: addition of unsigned offset to 0xf40032be overflowed to 0x96a2d560
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior print-lwres.c:294:10 in 

[fxlb]

Same for tcpdump-4.99.3, tcpdump-4.99.4, master. Thank you!
I have commited the fix for lwres with, in commit message, the error on line 294.

[fxlb]

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

[fenner]

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".

[fxlb]

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.

[fenner]

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.

[infrastation]

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

[fxlb]

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.

[fenner]

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.

[fxlb]

Rebase on main branch. 4/5 commits have already been merged.

[fxlb]

There is a warning with Visual Studio 17 2022:
checksum.c(153,26): warning C4244: '=': conversion from 'int' to 'uint16_t', possible loss of data.
https://ci.appveyor.com/project/guyharris/tcpdump/builds/51159591/job/or4dequ9f8xe2n4v
Cast needed?