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

Pcapng Clarification question - is end of option need to be when there are no options#108

Open
rluvaton opened this issue Jul 8, 2021 · 4 comments

Comments

@rluvaton
Copy link

rluvaton commented Jul 8, 2021

If the option list is empty does code that writes pcapng files MUST put an opt_endofopt anyway?

The option list is terminated by a option which uses the special 'End of Option' code (opt_endofopt). Code that writes pcapng files MUST put an opt_endofopt option at the end of an option list. Code that reads pcapng files MUST NOT assume an option list will have an opt_endofopt option at the end; it MUST also check for the end of the block, and SHOULD treat blocks where the option list has no opt_endofopt option as if the option list had an opt_endofopt option at the end.

from Pcapng specification- options

@rluvaton rluvaton changed the title Pcapng Clarification question - is end of option need to be where there are no options Pcapng Clarification question - is end of option need to be when there are no options Jul 8, 2021
@mcr
Copy link
Collaborator

mcr commented Jul 8, 2021

If we have to write code that deals with the lack of an end of option, maybe we should just obsolete opt_endofopt?

@rluvaton
Copy link
Author

rluvaton commented Jul 8, 2021

I agree, I don't see additional value added to that option

@guyharris
Copy link
Collaborator

guyharris commented Jul 8, 2021

I agree, I don't see additional value added to that option

That's because there is none. At least for all existing blocks that support options, the options are at the end of the block, so the end of the options occurs when you run out of remaining data in the block, and there's no need for an end-of-options marker. Furthermore, the spec explicitly says that all blocks must be structured that way:

A block that may contain options must be structured so that the number of octets of data in the Block Body that precede the options can be determined from that data; that allows the beginning of the options to be found. That is true for all standard blocks that support options; for Custom Blocks that support options, the Custom Data must be structured in such a fashion. This means that the Block Length field (present in the General Block Structure, see Section 3.1) can be used to determine how many octets of optional fields, if any, are present in the block. That number can be used to determine whether the block has optional fields (if it is zero, there are no optional fields), to check, when processing optional fields, whether any optional fields remain, and to skip all the optional fields at once.

The spec says

The option list is terminated by a option which uses the special 'End of Option' code (opt_endofopt). Code that writes pcapng files MUST put an opt_endofopt option at the end of an option list. Code that reads pcapng files MUST NOT assume an option list will have an opt_endofopt option at the end; it MUST also check for the end of the block, and SHOULD treat blocks where the option list has no opt_endofopt option as if the option list had an opt_endofopt option at the end.

The intent behind

Code that reads pcapng files MUST NOT assume an option list will have an opt_endofopt option at the end; it MUST also check for the end of the block, and SHOULD treat blocks where the option list has no opt_endofopt option as if the option list had an opt_endofopt option at the end.

is to keep readers from doing something blatantly stupid such as running past the end of the block if the writer didn't put an end-of-option marker in the block.

The intent behind

Code that writes pcapng files MUST put an opt_endofopt option at the end of an option list.

is to avoid having readers that would do something blatantly stupid such as running past the end of the block if the writer didn't put an end-of-option marker in the block actually do something stupid. "Belt and suspenders", as the saying goes.

At least some software that writes blocks does not add an end-of-option marker if there are no options in the first place. For example, neither Wireshark nor the pcapng-writing code in the macOS libpcap does so.

As such, it may be that, when reading a block with no options, some blatantly stupid code might well process the non-existent options until it either 1) gets a SIGSEGV, 2) scribbles on memory (by byte-swapping in place) that causes sufficient damage to cause a crash, or 3) hits a random blob of memory with 4 bytes of zero so that it looks like an end-of-option marker.

However, there might be other code that's careful enough to find out how many bytes of option there are (which is enough information to prevent that form of blatant stupidity) but not careful enough to do anything with it other than

if (total_option_length != 0)
    process_options(...);

and then, in process_options(), assumes that there's an end-of-option marker.

So perhaps what we should do is change

Code that writes pcapng files MUST put an opt_endofopt option at the end of an option list.

to

Code that writes pcapng files MAY put an opt_endofopt option at the end of an option list, to defend against careless code assuming that there's always an opt_endofopt option at the end of an option list. If there are no options in a block, no opt_endofopt option need be written.

We'll continue to write it in Wireshark, at least for now, if any options are present. If any pcapng-writing code is added to libpcap, should it write it, or is it time to flush out blatantly stupid code, and not put that into libpcap and eventually remove it from Wireshark?

EDIT: Fixed 2023-01-22 to say "is to avoid having readers that would do something blatantly stupid such as running past the end of the block if the writer didn't put an end-of-option marker in the block actually do something stupid" rather than "is to avoid having writers that would do something blatantly stupid such as running past the end of the block if the writer didn't put an end-of-option marker in the block actually do something stupid".

@rluvaton
Copy link
Author

rluvaton commented Jul 8, 2021

Wow @guyharris, very comprehensive explanation.

After what you said, I don't think you should remove the end of options option if the option list isn't empty as it could potentially break parsing libraries that assume it exist.

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

No branches or pull requests

3 participants