IEEE 802.11: include the "Mesh ID" field while printing management frames #924

gokulkumar792 · 2021-07-04T13:03:09Z

In an 802.11s mesh network, on detecting that the Beacon and Probe Response frames transmitted has a non-zero length "Mesh ID", print it.
Also add a test to check the 802.11s Mesh ID in management frames. The newly added pcap file contains a Mesh Beacon frame, a Wildcard Probe Request and a Mesh Probe Response. The test case checks if the "Mesh ID" field is properly parsed by the IEEE 802.11 printer.

mcr · 2021-07-05T00:07:24Z

print-802_11.c

+struct meshid_t {
+uint8_telement_id;
+uint8_tlength;
+u_charmeshid[33]; /* 32 + 1 for null */
+};
+


Is this going to work properly with all compilers on all architectures? Don't we need a PACKED attribute here to make sure that the bytes are not padded out?

On 99 44/100% of compilers, struct meshid_t won't be padded, as nothing in it requires more than alignment on byte boundaries.

I think some compilers would pad the structure either to a 2-byte or 4-byte boundary, but even in that case, this structure isn't being overlaid on top of packet data, it's just being filled in from packet data, so that doesn't matter.

@mcr, i have tested this code changes in x86 and arm. For checking on other platforms, i have added a new test "802.11_meshid". With that, i was relying on tcpdump buildbot to run the new test and confirm that the 802.11 printer was able to extract the 802.11s Mesh ID from the Mesh Beacon & Mesh Probe Respose IE on other platforms.

mcr · 2021-07-05T00:13:30Z

print-802_11.c

 }
 break;
+case E_MESHID:
+memcpy(&meshid, p + offset, 2);


I was wondering if the p+offset could be an overread.
line 1167 checks if the elementlen number of bytes are present, but that number could be less than 2, I think?
I don't think we check if elementlen >= 2, such that the entire messid.length is present.
I think that the memcpy() at line 1357 needs to use GET_U...().
@fxlb do you concur? It might be that other branches need adjustment too. I haven't looked yet.

No, 1167 does an initial check of the length, and makes sure it is all present, so I think that there is no issue here.

Should print-802_11.c just use GET_CPY_BYTES() instead of memcpy()?

I think that it is not strictly necessary, but it would make it easier.
As for your previous comment about the structure being overlaid, the memcpy() is doing exactly that.

As for your previous comment about the structure being overlaid, the memcpy() is doing exactly that.

Not exactly.

The first memcpy - which is done for every IE, not just for the one - is copying the first two bytes of the IE, which are the type and length. If padding is inserted between 1-byte fields, We Have A Problem with just about every data structure we have, but I know of no compilers that would do that.

The second memcpy - which is also done from most if not all IEs - is copying into the string buffer in the structure, not into the structure as a whole.

This is the way other IEs are handled as well.

It might be cleaner if the structures in question didn't have an element_id member, as that's not used (the type of the structure implies which IE it is), at which point there would be nothing that even looks like overlaying being done.

It might be cleaner if the structures in question didn't have an element_id member, as that's not used (the type of the structure implies which IE it is), at which point there would be nothing that even looks like overlaying being done.

I've checked in that change as 33b6502, with further improvements in 36fded6.

struct meshid_t should be changed to match, with the element_id element removed, and with the length element being a u_int.

The first line of the case E_MESHID: code should be changed from a memcpy() to

meshid.length = elementlen;

@mcr & @guyharris , thanks for the review. As suggested, i have made the changes and updated the PR.

And as you both might have seen already, the newly added MESHID field is printed to stdout enclosed by a parenthesis like below. Reason for introducing this parenthesis is, earlier i was printing the MESHID immediately after the empty parenthesis of the SSID field, so I also enclosed the MESHID field with it for better readability. But then i have moved it to the end of the line, because if someone have written a tcpdump stdout parser the newly added field at the middle of the line would break their logic.

12:20:37.867811 9526800862us tsft 6.0 Mb/s 5745 MHz 11a -34dBm signal [bit 22] Beacon () [6.0* 9.0 12.0* 18.0 24.0* 36.0 48.0 54.0 Mbit] IBSS CH: 149, PRIVACY (MESHID: 11s-mesh-network)

Now i have some second thoughts. Would you suggest printing the MESHID field as it is with a parenthesis, or introduce a comma before printing the MESHID without any parenthesis like below ?

12:20:37.867811 9526800862us tsft 6.0 Mb/s 5745 MHz 11a -34dBm signal [bit 22] Beacon () [6.0* 9.0 12.0* 18.0 24.0* 36.0 48.0 54.0 Mbit] IBSS CH: 149, PRIVACY, MESHID: 11s-mesh-network

@mcr, @guyharris, @fxlb, I request you to suggest one of the two ways mentioned in my previous comment to print the "meshid" to stdout, so that I can update the PR if needed. Thanks.

Using parenthesis - the commit in this Pull Request uses this format to print "Mesh ID"
12:20:37.867811 9526800862us tsft 6.0 Mb/s 5745 MHz 11a -34dBm signal [bit 22] Beacon () [6.0* 9.0 12.0* 18.0 24.0* 36.0 48.0 54.0 Mbit] IBSS CH: 149, PRIVACY (MESHID: 11s-mesh-network)

Using comma
12:20:37.867811 9526800862us tsft 6.0 Mb/s 5745 MHz 11a -34dBm signal [bit 22] Beacon () [6.0* 9.0 12.0* 18.0 24.0* 36.0 48.0 54.0 Mbit] IBSS CH: 149, PRIVACY, MESHID: 11s-mesh-network

I prefer comma. Can meshid have a comma in it? Then "11s-mesh-network" might be best.

@mcr, thanks a lot for your suggestion. Yes, the comma and even double quotes are allowed in Mesh ID and I didn't think about this. The Mesh ID field is exactly similar to the SSID field with respect to the allowed characters and length. So rather than enclosing the Mesh ID field with double quotes, we can stick to printing the Mesh ID field with parenthesis, so that its consistent with the way the SSID field is printed. we don't have to update the current patch in this PR, sorry for the confusion.

gokulkumar792 · 2021-08-21T19:13:12Z

Hi, please let me know if there is anything that I could help for integrating this changes into tcpdump. Thanks.

mcr · 2021-08-21T22:21:39Z

Hi, please let me know if there is anything that I could help for integrating this changes into tcpdump. Thanks.

If @guyharris has no further objections, I will merge.

guyharris

Looks good to me.

infrastation · 2021-08-21T22:52:20Z

59 commits behind the master branch, needs to be rebased.

…ames In an 802.11s mesh network, on detecting that the Beacon and Probe Response frames transmitted has a non-zero length "Mesh ID", print it.

…ames The newly added pcap file contains a Mesh Beacon frame, a Wildcard Probe Request and a Mesh Probe Response. The test case checks if the "Mesh ID" field is properly parsed by the IEEE 802.11 printer.

gokulkumar792 · 2021-08-22T05:54:22Z

Rebased the branch on top of the-tcpdump-group:master, tested it and updated the PR, Thanks.

infrastation · 2021-08-22T10:05:22Z

I looked at the initially proposed changes briefly in July and it was not immediately clear that data access was safe in terms of lenghts and shapend guards. After a closer look at the latest revision it looks fine in this regard.

The only potential issue that caught my eye this time is that meshid.meshid is not initialized and only its last byte is set to NUL, which leaves space for printing random bytes. For example, when meshid.length is zero (hence no call to memcpy()), or when it is not (if mesh ID on the wire is not NUL-terminated). If that's the case, it would be better to zero whole meshid.meshid before the if-block instead of NUL-terminating it after.

gokulkumar792 · 2021-08-24T19:49:37Z

I looked at the initially proposed changes briefly in July and it was not immediately clear that data access was safe in terms of lenghts and shapend guards. After a closer look at the latest revision it looks fine in this regard.

The only potential issue that caught my eye this time is that meshid.meshid is not initialized and only its last byte is set to NUL, which leaves space for printing random bytes. For example, when meshid.length is zero (hence no call to memcpy()), or when it is not (if mesh ID on the wire is not NUL-terminated). If that's the case, it would be better to zero whole meshid.meshid before the if-block instead of NUL-terminating it after.

Thanks for the review. For the "MESHID" field, if the meshid.length struct member is 0, then yes memcpy() to meshid.meshid is not done, but we are setting index 0 (meshid.length) of meshid.meshid to NULL.
So in this case, while printing the meshid.meshid, because of the 0th index being NULL, we would not attempt to print indexes that are > 0 irrespective of them having junk chars or not.

For example: The "SSID" & "MESHID" field parsing/printing follows the same logic.
So in the newly added tests/ieee802.11_meshid.out file, we can see that for the Mesh Beacon (1st line) containing the Wildcard SSID (ssid.length being 0), the ssid field is printed with empty parenthesis without any junk characters.

1 12:20:37.867811 9526800862us tsft 6.0 Mb/s 5745 MHz 11a -34dBm signal [bit 22] Beacon () [6.0* 9.0 12.0* 18.0 24.0* 36.0 48.0 54.0 Mbit] IBSS CH: 149, PRIVACY (MESHID: 11s-mesh-network)

Also in the case of received meshid field not having a NULL termination, we are explicitly adding it after the last non-junk char ("after the last non-junk char" index is meshid.length) in the meshid.meshid array. So I believe we would not need to memset the meshid.meshid to 0 before the if-block. Please clarify if I'm mistaken.

And let me know, if you still prefer to add it. But then I would suggest that we add the zero memset also for E_SSID, E_CHALLENGE along with E_MESHID for consistency.

infrastation · 2021-08-24T20:16:32Z

You are right. It is good as it is now.

infrastation · 2021-08-25T09:26:14Z

So let's merge it then, there is plenty of other work waiting to be done.

infrastation added the improvement label Jul 5, 2021

mcr approved these changes Jul 6, 2021

View reviewed changes

gokulkumar792 force-pushed the print_meshid branch 2 times, most recently from 6abb8f9 to 653a430 Compare July 7, 2021 15:58

fxlb force-pushed the print_meshid branch from 653a430 to 0c1258c Compare July 12, 2021 07:34

mcr requested a review from guyharris August 21, 2021 22:20

guyharris approved these changes Aug 21, 2021

View reviewed changes

gokulkumar792 added 2 commits August 22, 2021 11:08

IEEE 802.11: include the "Mesh ID" field while printing management fr…

2782351

…ames In an 802.11s mesh network, on detecting that the Beacon and Probe Response frames transmitted has a non-zero length "Mesh ID", print it.

IEEE 802.11: Add a test to check the 802.11s Mesh ID in management fr…

34f0181

…ames The newly added pcap file contains a Mesh Beacon frame, a Wildcard Probe Request and a Mesh Probe Response. The test case checks if the "Mesh ID" field is properly parsed by the IEEE 802.11 printer.

gokulkumar792 force-pushed the print_meshid branch from 0c1258c to 34f0181 Compare August 22, 2021 05:48

mcr merged commit e5db29f into the-tcpdump-group:master Aug 25, 2021

gokulkumar792 deleted the print_meshid branch August 25, 2021 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

IEEE 802.11: include the "Mesh ID" field while printing management frames #924

IEEE 802.11: include the "Mesh ID" field while printing management frames #924

Uh oh!

gokulkumar792 commented Jul 4, 2021

mcr Jul 5, 2021

guyharris Jul 6, 2021

gokulkumar792 Jul 7, 2021

mcr Jul 5, 2021

mcr Jul 5, 2021

guyharris Jul 6, 2021

mcr Jul 6, 2021

guyharris Jul 6, 2021 •

edited

Loading

guyharris Jul 6, 2021

gokulkumar792 Jul 7, 2021 •

edited

Loading

gokulkumar792 Jul 18, 2021

mcr Jul 19, 2021

gokulkumar792 Jul 20, 2021

gokulkumar792 commented Aug 21, 2021

mcr commented Aug 21, 2021

guyharris left a comment

infrastation commented Aug 21, 2021

gokulkumar792 commented Aug 22, 2021

infrastation commented Aug 22, 2021

gokulkumar792 commented Aug 24, 2021

infrastation commented Aug 24, 2021

infrastation commented Aug 25, 2021

Labels

4 participants

IEEE 802.11: include the "Mesh ID" field while printing management frames #924

IEEE 802.11: include the "Mesh ID" field while printing management frames #924

Uh oh!

Conversation

gokulkumar792 commented Jul 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guyharris Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulkumar792 Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulkumar792 commented Aug 21, 2021

mcr commented Aug 21, 2021

guyharris left a comment

Choose a reason for hiding this comment

infrastation commented Aug 21, 2021

gokulkumar792 commented Aug 22, 2021

infrastation commented Aug 22, 2021

gokulkumar792 commented Aug 24, 2021

infrastation commented Aug 24, 2021

infrastation commented Aug 25, 2021

Labels

4 participants

guyharris Jul 6, 2021 •

edited

Loading

gokulkumar792 Jul 7, 2021 •

edited

Loading