tcpdump mailing list archives
[tcpdump] After setjmp/longjmp update
From: Francois-Xavier Le Bail via tcpdump-workers <tcpdump-workers () lists tcpdump org>
Date: Sat, 5 Sep 2020 18:20:42 +0200
--- Begin Message --- From: Francois-Xavier Le Bail <devel.fx.lebail () orange fr>
Date: Sat, 5 Sep 2020 18:20:42 +0200
Hello, Some people ask for a new version of tcpdump. To understand why we cannot release now, here is some information: We are in the process to harden the tcpdump code with use of new (GET_) macros (with setjmp/longjmp logic) to fetch the packet data without have to do ND_TCHECK_/ND_TTEST_ macro calls before EXTRACT_ macros calls. They were some preliminary stages (for example: add and use nd_ types, more EXTRACT_XXX() calls, rename EXTRACT_XXX(), ND_TTEST_XXX() and ND_TCHECK_XXX macros, use EXTRACT_[SU]_1() to replace direct dereferences, etc.). The first step was to replace almost all the EXTRACT_ calls by GET_ calls. See: -176e182416d822df0f9d4695410479e8b17a07b3 (Apply the first step of the new way to fetch data with bounds checking) -ee68aa36460d7efeca48747f33b7f2adc0900bfb (Use the new GET_ macros instead of the EXTRACT_ ones) After we had to manage this warning: After we have had this warning: ./print.c: In function 'pretty_print_packet': ./print.c:342:8: warning: variable 'hdrlen' might be clobbered by 'longjmp' or 'vfork' [-Wclobbered] 342 | u_int hdrlen = 0; | ^~~~~~ therefore, the second step was: -757e793ca521cbfdc4a6104c9a2a920f5538d195 (Apply the first step of the new way to update the link-layer header length) - ~30 commits -b30f3843b93c11e897e6d8888a91abf709a716ae (Apply the last step of the new way to update the link-layer header length) There are now redundant bounds checks because most of the GET_.*_n(e) macros are preceded by a ND_TCHECK_n(e) call (same n, same e). There are also two ways to process the truncated cases: GET_ only (via longjmp) or ND_TCHECK_/ND_TTEST_ before GET_ going back all the functions called. As a consequence of these two ways to process the truncated cases, the -x/-X output may be not the same. Example of a Kerberos truncated packet: A) Currently we have: ./tcpdump -#envvvvvx -r krb-truncated.pcap reading from file krb-truncated.pcap, link-type EN10MB (Ethernet), snapshot length 69 1 01:00:00.000000 01:01:fc:83:00:00 > 02:8e:00:50:6a:e1, ethertype IPv4 (0x0800), length 262144: (tos 0x8, ttl 64, id 55616, offset 0, flags [DF], proto UDP (17), length 32820, bad cksum 6a70 (->3baf)!) 10.0.0.1.88 > 0.234.154.214.24074: v4 be KDC_REQUEST: ^O^O^O^O^O^U.@^O^D^O^O^O^O^O^O^O^O^O^O^O^O [|krb] 0x0000: 4508 8034 d940 4000 4011 6a70 0a00 0001 0x0010: 00ea 9ad6 0058 5e0a 0280 00b1 0402 0f0f 0x0020: 0f0f 0f15 0000 0f04 0f0f 0f0f 0f0f 0f0f 0x0030: 0f0f 0f0f 0016 88 The -x printing begins by: 4508: IPv4 with tos 0x08: OK B) Testing a patch to remove now useless ND_TCHECK_1() checks (because of GET_[SU]_1() uses) $ git diff --------------------------------------------------------------------- diff --git a/print-krb.c b/print-krb.c index 41a6126a..c6c2fe7d 100644 --- a/print-krb.c +++ b/print-krb.c @@ -168,8 +168,6 @@ krb4_print(netdissect_options *ndo, kp = (const struct krb *)cp; - ND_TCHECK_1(kp->type); - type = GET_U_1(kp->type) & (0xFF << 1); ND_PRINT(" %s %s: ", @@ -181,7 +179,6 @@ krb4_print(netdissect_options *ndo, if ((cp = krb4_print_hdr(ndo, cp)) == NULL) return; cp += 4; /* ctime */ - ND_TCHECK_1(cp); ND_PRINT(" %umin ", GET_U_1(cp) * 5); cp++; PRINT; @@ -191,14 +188,11 @@ krb4_print(netdissect_options *ndo, case AUTH_MSG_APPL_REQUEST: cp += 2; - ND_TCHECK_1(cp); ND_PRINT("v%u ", GET_U_1(cp)); cp++; PRINT; - ND_TCHECK_1(cp); ND_PRINT(" (%u)", GET_U_1(cp)); cp++; - ND_TCHECK_1(cp); ND_PRINT(" (%u)", GET_U_1(cp)); break; --------------------------------------------------------------------- C) The -x printing begins now by: 028e: Begin of destination MAC address, IPv4 is 14 octets after. ./tcpdump -#envvvvvx -r krb-truncated.pcap reading from file krb-truncated.pcap, link-type EN10MB (Ethernet), snapshot length 69 1 01:00:00.000000 01:01:fc:83:00:00 > 02:8e:00:50:6a:e1, ethertype IPv4 (0x0800), length 262144: (tos 0x8, ttl 64, id 55616, offset 0, flags [DF], proto UDP (17), length 32820, bad cksum 6a70 (->3baf)!) 10.0.0.1.88 > 0.234.154.214.24074: v4 be KDC_REQUEST: ^O^O^O^O^O^U.@^O^D^O^O^O^O^O^O^O^O^O^O^O^O [|krb] 0x0000: 028e 0050 6ae1 0101 fc83 0now 000 0800 4508 0x0010: 8034 d940 4000 4011 6a70 0a00 0001 00ea 0x0020: 9ad6 0058 5e0a 0280 00b1 0402 0f0f 0f0f 0x0030: 0f15 0000 0f04 0f0f 0f0f 0f0f 0f0f 0f0f 0x0040: 0f0f 0016 88 We have: 1) The "old" way, before the patch, returning from all functions and at the end returning the header length to the link-layer dissector (xxx_if_print), updating the 'ndo->ndo_ll_hdr_len' field (output A). b) The "longjmp" way with GET_ macros but no ND_TCHECK_() macros (and thus no 'trunc' label code use): The 'ndo->ndo_ll_hdr_len' is not (or sometimes partially) updated (output C). To have a single process of the truncated cases and avoid redundant bounds checks, we could: 1) Remove all the redundant ND_TCHECK_/ND_TTEST_ bounds checks. Consequently remove most of 'trunc' labels and the associated codes. 1bis) Examine if we have to remove some ND_TCHECK_SIZE/ND_TCHECK_LEN redundant macros calls. 1ter) Examine if we have to remove some ND_TTEST_ macros calls. [number of ND_TCHECK_ calls: ND_TCHECK_1: 358 ND_TCHECK_2: 337 ND_TCHECK_3: 21 ND_TCHECK_4: 367 ND_TCHECK_5: 5 ND_TCHECK_6: 16 ND_TCHECK_7: 4 ND_TCHECK_8: 65 ND_TCHECK_16: 25 ND_TCHECK_LEN: 445 ND_TCHECK_SIZE: 238 Total: 1881] [number of ND_TTEST_ calls: ND_TTEST_1: 39 ND_TTEST_2: 17 ND_TTEST_3: 0 ND_TTEST_4: 6 ND_TTEST_5: 0 ND_TTEST_6: 1 ND_TTEST_7: 0 ND_TTEST_8: 0 ND_TTEST_16: 0 ND_TTEST_LEN: 45 ND_TTEST_SIZE: 15 Total: 123] (A patch is almost ready to remove ~700 ND_TCHECK_n) 2) Process all the truncated cases with: ndo->ndo_ll_hdr_len = 0; longjmp(ndo->ndo_truncated, 1); (With a new macro, like 'ND_TRUNCATED' or 'ND_IS_TRUNCATED') 3) Consequently replace 'longjmp(ndo->ndo_truncated, 1);' by this new macro in all the get_XXX inline functions in addrtoname.h and extract.h. 4) If some 'goto trunc' are still necessary, replace them by the new macro. 5) This implies that the functions which use a return value to indicate a truncated case, like: --------- static int handle_deauth(netdissect_options *ndo, const uint8_t *src, const u_char *p, u_int length) { [...] return 1; trunc: return 0; } --------- should be be updated to void functions. Also, there is an impact on the code *calling* these 'void functions' like for this example: mgmt_body_print(), etc. (Many changes to do) 6) State that -x When parsing and printing, in addition to printing the headers of each packet, print the data of each packet (minus its link level header) in hex. If the packet was truncated the link layer header is printed. -X When parsing and printing, in addition to printing the headers of each packet, print the data of each packet (minus its link level header) in hex and ASCII. If the packet was truncated the link layer header is printed. -- Francois-Xavier
--- End Message ---
_______________________________________________ tcpdump-workers mailing list tcpdump-workers () lists tcpdump org https://lists.sandelman.ca/mailman/listinfo/tcpdump-workers
Current thread:
- [tcpdump] After setjmp/longjmp update Francois-Xavier Le Bail via tcpdump-workers (Sep 05)
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 07)
- Re: [tcpdump] After setjmp/longjmp update Francois-Xavier Le Bail via tcpdump-workers (Sep 07)
- Message not available
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 07)
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 07)
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 12)
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 17)
- Re: [tcpdump] After setjmp/longjmp update Francois-Xavier Le Bail via tcpdump-workers (Sep 17)
- Re: [tcpdump] After setjmp/longjmp update Francois-Xavier Le Bail via tcpdump-workers (Sep 18)
- Message not available
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 18)
- Message not available
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 24)
- Re: [tcpdump] After setjmp/longjmp update Francois-Xavier Le Bail via tcpdump-workers (Sep 17)
- Re: [tcpdump] After setjmp/longjmp update Denis Ovsienko via tcpdump-workers (Sep 17)