Skip to content

Conversation

corubba
Copy link

@corubba corubba commented Sep 23, 2025

Add basic printing of "Organization Specific Slow Protocol" (OSSP), which is standardized in IEEE 802.3 Annex 57B. Since this is used for non-standardized protocols and thus carries mostly unknown-structured data, most of the packet is still printed verbatim.

Also add the OUI of ITU-T that is used as part of its "Ethernet synchronization messaging channel" (ESMC) protocol, which is based on OSSP.

ND_PRINT("\n\tOUI: %s (0x%06x)",
tok2str(oui_values, "Unknown", oui),
oui);
tlen -= 3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, this line can cause an out-of-bounds read: neither slow_ossp_print() nor its caller ensure that here tlen >= 3. Please add a length check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET_BE_U_3 does a length check, and will not return if there is not enough data. You can find the same pattern in other protocols, for example here, here and here. Worked fine when I tested it reducing the packet step by step one byte at a time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context the GET_ macros guard the boundaries of the actually present (captured) data, and tlen is the declared data length. Please note:

  • the ND_ICHECK_U(len, <, 8); just before the data fetch in the first example,
  • the if (subtlv_len >= 6) around the data fetch in the second example, and
  • the if (len < length * NSH_HDR_WORD_SIZE) before the data fetch in the third example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand how tlen could ever be less than the captured data (the other way around sure), and thus tlen < 3 not always first trigger the GET_BE_U_3 longjump. But I will assume you know better and submit to your judgement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the reason is to have the correct message in the output, truncated packet (due to snaplen) vs invalid packet (simply too short).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness of reporting is one reason, and consistency of code style is another.

Yet another reason is safety: any "length" recorded in the captured data generally cannot be trusted. In this case tlen is not obviously off: the EtherType/length field of the Ethernet frame stands for EtherType so in ethertype_print() it seems likely that length >= caplen, especially when the frame is read from a file and the length values have been sanitized.

However, if you consider how many protocols encapsulate Ethernet frames now and how many more will do that in future, it is almost certain that an encoding exists that has its own idea of an encapsulated Ethernet frame length and will present, let's say, 1KB of "captured" data and a value of length that will cause tlen here to be, let's say, 1. Then after the subtraction tlen would underflow to a very large value and print_unknown_data() will be printing a memory dump until it trips a boundary of the process' virtual memory and causes a segmentation fault. This is why the other three cases in this file and most if not all other decoders check both lengths.

Add basic printing of "Organization Specific Slow Protocol" (OSSP),
which is standardized in IEEE 802.3 Annex 57B. Since this is used for
non-standardized protocols and thus carries mostly unknown-structured
data, most of the packet is still printed verbatim.
@infrastation
Copy link
Member

Alright, this revision looks safe to me, maybe somebody else would like to have a look.

Let me note that print-slow.c makes it easy to introduce a bug without realising it: for SLOW_PROTO_LACP and SLOW_PROTO_MARKER slow_print() unnecessarily reaches beyond the subtype field, but leaves all SLOW_PROTO_OAM verification to the downstream function. It also switches on the subtype twice. Perhaps the only two things slow_print() needs to do is decoding the subtype and calling the next function, which will take it from there and print any other subtype-specific fields as it sees fit. This can be attempted later, of course.

Copy link
Member

@infrastation infrastation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This revision looks safer.

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.

2 participants