Allow access to all 12 TCP header flags, Decode 9th flag TH_AE (AccECN) as "a" stateless.

[rscheff]

Provide the means to decode all 12 flag bits of the TCP header.

While the Nonce Sum ("NS" in wireshark) was made historic (https://datatracker.ietf.org/doc/html/rfc3540 , https://datatracker.ietf.org/doc/status-change-ecn-signaling-with-nonces-to-historic/03/ ), that bit will become Accurate ECN ("AE") shortly (https://datatracker.ietf.org/doc/draft-ietf-tcpm-accurate-ecn/), which is an integral signaling protocol for the L4S framework.

Other utilities (packetdrill) have already been extended to allow the scripting with the "A" bit (or the full ACE 3-bit field as a shorthand; as state would be required to decide if to show the bits or the field, keeping this simple and only show individual fields - which makes a tcpdump a valid input for packetdrill still).

Also, there are two TCP option numbers for experimental use (253 and 254).

[rscheff]

Note that the tests fail, as all the test pcap file contain non-zero bits as reserved bits in the TCP headers.

I'll try to fix these binaries to follow the RFCs (initialize reserved bits with zero).

[infrastation]

Since a packet analyzer's purpose is to display what is on the wire (even and especially if what's on the wire is not correct), it might be better to leave the captures as they are and update the output instead.

[rscheff]

Since a packet analyzer's purpose is to display what is on the wire (even and especially if what's on the wire is not correct), it might be better to leave the captures as they are and update the output instead.

Yes - I decided to do it this way, as editing the binaries was becoming painful. Reverted the one pcap, and adjusted the correct output instead now.

[rscheff]

Note that the output of tcpdump with this patch is equivalent with the fork of tcpdump in MacOS, being used for dev work on AccECN there, as well as the fork used by Google on Linux on their dev branches.

[rscheff]

Since a packet analyzer's purpose is to display what is on the wire (even and especially if what's on the wire is not correct), it might be better to leave the captures as they are and update the output instead.

Question: The RFCs are relatively clear, that reserved fields should be initialized with zero until they get an assigned purpose. In the past there have been implementations, which would react too restrictive, when encountering non-zero reserved bits.

Should this be shown in the text output of tcpdump (rather than only when you fully decode and check all the packet header bits one-by-one?

[infrastation]

Quite often the most useful approach is to name the reserved bits using the MBZ prefix (as is done in print-aos.c, print-radius.c and print-udld.c) and/or to check the bit field using a "known allocated" bitmask and to print "bogus" if any unallocated bits are set (see OFPPS_U in print-openflow-1.0.c).

[rscheff]

Thank you. Is that a request to further improve this pull request? Or for a subsequent one?

Strictly speaking, these reserved bits are NOT MBZ (must be zero), but rather SBZ (RFC2119 SHOULD be zero) ;)

As all the other tcp header Flags are assigned a single Char for text decoding purposes, and the remaining 3 bits are undefined/reserved (a receiver or middlebox should not behave differently regardless of their state), I think this should not be part of this pull request.

As for the procedural stuff - is there any additional formal discussing needed for such a patch to go in? The Accurate ECN draft will be ready for WGLC very soon, but functional implementations of this protocol already exist (eg.
https://developer.apple.com/videos/play/wwdc2022/10078/ ).

[infrastation]

The next step is that someone needs to study the changes with attention and to tell if everything looks ready [enough]. Eventually the WIP commits will need to be squashed together and merged. Please have some patience.

[infrastation]

For reference, respective IANA registry has moved here.

[infrastation]

Rebased on the current master branch, squashed, removed modifications to pre-existing packet captures and updated the tests to pass.

[infrastation]

Thank you for waiting. Please find below some more comments regarding the proposed code changes.

• The only change to TH_ECNECHO and TH_CWR is replacing a tab with a space, which is not consitent with the lines above and should not be done in this commit. Likewise, tests/TESTLIST should not lose the trailing newline.
• This change makes the TCP decoder recognize 9 bits (8 existing ones plus the new AE bit from the I-D above), not 12 flags; this way, currently the commit message does not match the changes and does not tell the rationale for the change, please improve that.
• The new TH_RES1, TH_RES2 and TH_RES3 constants are not used in any way; if these are an intended part of this change, please add respective code as discussed above, otherwise it will be right not to add the dead code.
• Most importantly, as far as the registry goes, the AE bit is neither allocated nor reserved. Surely there is the traditional chicken and egg problem here, but at the very least the comment for the new constant should tell the I-D name, revision and expiry date. As a matter of fact, a significant part of this source code maintenance comes from early implementations of I-Ds that either changed the protocol in later revisions or lapsed, but the code was left in place. Please address that in the best way you can.

[rscheff]

Thank you for waiting. Please find below some more comments regarding the proposed code changes.

Sorry - apparently this response never reached me.

• The only change to TH_ECNECHO and TH_CWR is replacing a tab with a space, which is not consitent with the lines above and should not be done in this commit. Likewise, tests/TESTLIST should not lose the trailing newline.

The other way around, making this consistent with the other lines. Since this is a style change and unrelated, i reverted this and leave the differently styled lines as they are. A trailing newline should now be part of tests/TESTLIST.

• This change makes the TCP decoder recognize 9 bits (8 existing ones plus the new AE bit from the I-D above), not 12 flags; this way, currently the commit message does not match the changes and does not tell the rationale for the change, please improve that.

Fixed the naming of this pull request

• The new TH_RES1, TH_RES2 and TH_RES3 constants are not used in any way; if these are an intended part of this change, please add respective code as discussed above, otherwise it will be right not to add the dead code.

Removed.

• Most importantly, as far as the registry goes, the AE bit is neither allocated nor reserved. Surely there is the traditional chicken and egg problem here, but at the very least the comment for the new constant should tell the I-D name, revision and expiry date. As a matter of fact, a significant part of this source code maintenance comes from early implementations of I-Ds that either changed the protocol in later revisions or lapsed, but the code was left in place. Please address that in the best way you can.

I added relevant referecences to the architeture RFCs under which AccECN is being standardized (currently WGLC; RFC by the end of the year likely) into the comment in tcp.h.

[rscheff]

When looking at the CI failures, it seems they fail due to some connectivity issues, but not the changes in this pull request...

https://ci.appveyor.com/project/guyharris/tcpdump/builds/47615803/job/jswck707rky68qg3

[infrastation]

Looks fine to me with the added commit. I am going to merge this pull request before the end of this Sunday, if anybody has any other remarks, please make these soon. If anybody merges this pull request before me, please do not forget to squash and to make sure the commit message looks good.

[infrastation]

@rscheff, please see issue #1069 about the man page. Would it be better to update the man page in the same commit?

[fxlb]

The man page should be updated to add the new flag.

[fxlb]

It may be useful to point out that the new 'A' flag is not ACK (ACK is '.').

[rscheff]

I checked in a minor update to the man page. However, reading through this, I would like to understand where the parser code lives that interprets "tcp[tcpflags] & tcp-cwr != 0". The tcpflags field is really 12 bit wide (octets 12&13), and to align with what is currently available, tcp-ae should also be a named flag (0x100) to match against this 12 bit field...

[rscheff]

I checked in a minor update to the man page. However, reading through this, I would like to understand where the parser code lives that interprets "tcp[tcpflags] & tcp-cwr != 0". The tcpflags field is really 12 bit wide (octets 12&13), and to align with what is currently available, tcp-ae should also be a named flag (0x100) to match against this 12 bit field...

Found that the parser actually lives in libpcap; allowing a token like "tcp[tcpflags]" to extract the full tcp flags field would have to go there (and be followed by corresponding man page changes). This however should not block this pull request. Once the new parser/grammer to allow that (special casing) is ready, I'll make a pull request for libpcap.

[infrastation]

The man page requires more work before merging. Your branch now has the fixup again. Let's not touch libpcap until the registry has settled, it is a pain to change the filter syntax after it has been published.

[rscheff]

Any update on taking this on? Where are we here?

[infrastation]

Please raise this on the mailing list, as suggested. Even if there is no feedback, it will be useful to know.

[rscheff]

I’ve tried to subscribe to the mailing list given the instructions https://www.tcpdump.org/#lists, but my post doesn’t show up here https://seclists.org/tcpdump/2023/q3/

Nor was there any reply from majordomo (and derivatives) which you’d normally get when subscribing…

The traffic volume also appears to be so low, that I question how active this is (or if the majordomo instance may be broken for a few months ☹

Richard Scheffenegger Consulting Solution Architect NAS & Networking

NetApp +43 1 3676 811 3157 Direct Phone +43 664 8866 1857 Mobile Phone @.@.>

From: Denis Ovsienko @.> Sent: Donnerstag, 17. August 2023 23:50 To: the-tcpdump-group/tcpdump @.> Cc: Scheffenegger, Richard @.>; Mention @.> Subject: Re: [the-tcpdump-group/tcpdump] Decode 9 tcp flag bits, add exp1 tcp option (PR #999)

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.

Please raise this on the mailing list, as suggested. Even if there is no feedback, it will be useful to know.

— Reply to this email directly, view it on GitHubhttps://github.com/the-tcpdump-group/tcpdump/pull/999#issuecomment-1683025331, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABL422KLKSZPPQNFAJYYNTDXV2G2LANCNFSM542V5O5A. You are receiving this because you were mentioned.Message ID: @.@.>>

[infrastation]

The recent migration to Mailman 3 had issues, some of which still stand. I have received your message, which means other subscribers have received it too. Thank you for posting, I propose to merge the changes if nobody objects by the end of August.

[fxlb]

I propose to merge the changes if nobody objects by the end of August.

I think this is premature, as the mailing list isn't working properly. I haven't received the reply I sent. Nor does it appear on https://seclists.org/tcpdump/2023/q3/. If interested parties can't reply, we can't conclude on the choice of the letter representing the new flag.

[rscheff]

This is still in limbo, without me understanding if there is any constructive path forward here.

In the meantime, other areas where the TCP header is decoded suggested to use "a" for AE while there "A" is for ACK...

Updated this patch accordingly...

[tuexen]

This is still in limbo, without me understanding if there is any constructive path forward here.

In the meantime, other areas where the TCP header is decoded suggested to use "a" for AE while there "A" is for ACK...

Updated this patch accordingly...

Isn't an ACK displaying as . in tcpdump? How can I see it printed as an 'A'? I would really like to see all twelve flags being displayed if they are non zero. In addition to the AE flags, these would be three flags which are currently being reserved. But that does not mean that they don't show up non-zero on the network.

[infrastation]

@rscheff, thank you for waiting. This matter is taking quite a while longer than it should.

The points you made earlier have been in the back of my mind for several months, but just as I was ready to respond to your latest post to tcpdump-workers, the mailing list went down. As it seems to require more time to get fixed, let's try to use this pull request comments again for now.

First of all, I agree that the use of lowercase letters is a good idea. But let me suggest to avoid even "a" for AccECN to eliminate as much confusion with ACK as possible. One way to achieve this could be as follows:

F -- FIN
S -- SYN
R -- RST
P -- PSH
. -- ACK
U -- URG
E -- ECE (ECN-Echo)
W -- CWR

p -- future development based on PSH, if any
u -- future development based on URG, if any
e -- AE (AccECN)
w -- future development based on CWR, if any

Do you agree this would work a bit better long-term? I can update and rebase the changes accordingly if you want.

[rscheff]

Yes, I also reconsidered - while commonality between different tools was in the back of my mind, I don't think this should stop making AccECN decoding available in as many tools as possible. In the meantime, another tool got AccECN support, also with its own nomenclature (https://github.com/Netflix/read_bbrlog/pull/4/commits/b9f66a4bb7773cbe070753177b478f6705d3b992 if anyone is interested) - as long as we decode it, I'm fine.

The tests (including old test files with randomized header data) would need adjustement too. If you can take care of this, it would be great. In the meantime, we found that certain NIC drivers on the send path need patches too, for efficient TSO support of AccECN.

[fxlb]

We could choose:

1. The Denis proposal "e" Or to keep uppercases:
2. "More Accurate Explicit Congestion Notification (ECN)", thus "C".
3. "More Accurate Explicit Congestion Notification (ECN)", thus "N".