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

allow "tcp[tcpflags]" access to all flag bits, including tcp-ae #1210

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

Conversation

rscheff
Copy link

@rscheff rscheff commented Jul 29, 2023

This is the adjacent patch to libpcap, to introduce the 9th TCP flag bit (tcp-ae).

Also handle tcp[tcpflags] specially to allow access to all TCP header flags without having to
revert to tcp[12:2] which is much less readable, so that existing scripts can easily be extended
to check for the AE flag in an easy to understand manner.

@guyharris
Copy link
Member

If you reinterpret tcpflags as 12:2 rather than 13:1, it also includes the Data Offset (Header Length) field. Should tcp[tcpflags] instead be interpreted as tcp[tcpflags] & 0x1FF, so that it only picks up bits that are used for flags, allowing filters to explicitly compare tcp[tcpflags] against a specific value rather than testing specific bits or doing masking explicitly in the filter?

@rscheff
Copy link
Author

rscheff commented Jul 29, 2023

Probably yes, masking out the length field would be good. I didn't dig deep enough to fully grasp the details of handling the return value in arth* though; I don't claim to have a good handle on yacc/bison for that.

This maybe something to generalize, to have access to other fields in a right-aligned way (e.g. tcp[offset], tcp[window], tcp[seq], tcp[ack], tcp[checksum], tcp[urgent]), but for now I just thought about the convinience of allowing direct access to all 12 bits of the TCP header flags.

@rscheff
Copy link
Author

rscheff commented Jul 29, 2023

If you reinterpret tcpflags as 12:2 rather than 13:1, it also includes the Data Offset (Header Length) field. Should tcp[tcpflags] instead be interpreted as tcp[tcpflags] & 0x1FF, so that it only picks up bits that are used for flags, allowing filters to explicitly compare tcp[tcpflags] against a specific value rather than testing specific bits or doing masking explicitly in the filter?

I think I understand now how to build such a dynamic structure in the parser; please let me know if this is the correct way to do it, and if the whitespace indentation is acceptable.

@tuexen
Copy link

tuexen commented Dec 30, 2023

If you reinterpret tcpflags as 12:2 rather than 13:1, it also includes the Data Offset (Header Length) field. Should tcp[tcpflags] instead be interpreted as tcp[tcpflags] & 0x1FF, so that it only picks up bits that are used for flags, allowing filters to explicitly compare tcp[tcpflags] against a specific value rather than testing specific bits or doing masking explicitly in the filter?

I think I understand now how to build such a dynamic structure in the parser; please let me know if this is the correct way to do it, and if the whitespace indentation is acceptable.

I would suggest to interpret tcp[tcpflags] as tcp[tcpflags] & 0xFFF and provide generic names for the three remaining flags like tcp-res-0, tcp-res-1, and tcp-res-2. These flags are currently not yet assigned, but are according to IANA header flags. So I might want to filter for them.

@rscheff
Copy link
Author

rscheff commented Dec 30, 2023

I would suggest to interpret tcp[tcpflags] as tcp[tcpflags] & 0xFFF and provide generic names for the three remaining flags like tcp-res-0, tcp-res-1, and tcp-res-2. These flags are currently not yet assigned, but are according to IANA header flags. So I might want to filter for them.

With this change, the tcp[tcpflags] expression creates that 12-bit value; I addeded "tcp-res, tcp-res1, tcp-res2 and tcp-res3" to check if any of the reserved flags are non-zero, or one specific flag is non-zero (e.g. "tcp[tcpflags] & tcp-res" would be non-zero if any of the three uppermost flag bits is set).

The documentation would need to go to tcpdump.4.in for these 4 additional tokens.

@guyharris
Copy link
Member

The documentation would need to go to tcpdump.4.in for these 4 additional tokens.

Presumably you meant "...would need to go to pcap-filter.manmisc.in ..."

@infrastation
Copy link
Member

Thank you for waiting. Let me comment on the following aspects separately:

  • Adding tcp-ae is the key part and the extension of tcp[tcpflags] from 8 to 12 bits is essential to support it properly. To that end, as soon as the registry has the bit allocated, it will be right to include tcp-ae into subsequent libpcap releases. These changes could sit merged into the master branch while the Internet-Draft remains an active Internet-Draft. Would this suit everyone?
  • tcp-res is not an essential element and one reason not to implement it is that the TCP flags registry, the various libpcap versions deployed with various software and the live Internet traffic will be out of sync. In particular, expressions using tcp-res would fail to compile until the libpcap change has propagated. From my experience, this would easily take years, the propagated version would use bits 4-6. Then the registry could allocate another flag, which would no longer be reserved in the registry, and subsequently in libpcap code after an update, then old libpcap versions would match the newly allocated flag bit as reserved, and new libpcap versions would not. This way, the same software with the same filter expression would match different live packets for reasons that should not be a problem of the library users or the end users. Then years later things would begin to converge until the registry allocated another flag, and so on. This is not necessarily the exact future development, but it seems probable enough to want to avoid it. Let me suggest to go only as far as implementing tcp[tcpflags] correctly in 12 bits, then the users can know that if libpcap accepted a filter without an error, the filter behaviour will be predictable.
  • tcp-res1, tcp-res2 and tcp-res3 correspond to neither the bit numbers nor the flag names in the TCP flags registry. These names would need to be better in order to justify the maintenance long-term (including the need for backward compatibility aliases after the other reserved TCP flags become allocated). Perhaps this can be decided later.

@rscheff
Copy link
Author

rscheff commented Feb 22, 2024

Agreed. Note that for logistical reasons, the adoption of AccECN as an RFC will not happen before the next IETF end of march, and probably a few weeks thereafter (change of the responsible Area Director will also happening).
The AccECN option values have already been reserved, the header flag not yet.
https://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml

@infrastation
Copy link
Member

As discussed, please include only the changes that implement 12-bit tcp[tcpflags] and tcp-ae and rebase on the current master branch. Or take a backup of this branch and let me know I can do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants