rudimentary eap packet parsing and dot11_action #303
Conversation
|
I think that the 802.11 Action definition is missing the one-byte action code and dialog token fields but that should be a simple addition. |
|
those are not always present in all action packets iirc (for reference scapy only parses the one byte too) , maybe you can subclass it and add those in, but thats just a slippery slope of dozens of different types of actions to implement and my goal was mainly to just make sure action packets weren't dropped. |
|
After looking at 802.11-2016 (9.3.3.14/Table 9-38)and 802.11u-2011, it looks the one-byte action code field is defined for all action frames in order to specify the type of action frame it is. Other than that it looks good. EDIT: I stand corrected, it seems that it applies for all EXCEPT for vendor-specific (126/127). It looks like we will have to subclass it as you said. |
| void write_serialization(uint8_t* buffer, uint32_t total_sz); | ||
|
|
||
| eapol_header header_; | ||
| }; | ||
|
|
||
| class TINS_API Eap : public EAPOL { |
There was a problem hiding this comment.
Any reason not to call this EAP? That would make it consistent with the rest of the class names.
There was a problem hiding this comment.
i was going by the dot11 classes but i see the rest are all uppercase
| * \brief Enum for the different action categories. | ||
| * | ||
| */ | ||
| enum ActionCategories { |
There was a problem hiding this comment.
I would call this ActionCategory rather than the plural form of it.
| InputMemoryStream stream(buffer, total_sz); | ||
| stream.skip(management_frame_size()); | ||
| stream.read(body_); | ||
| if(stream) { |
| */ | ||
| enum EAPOLTYPE { | ||
| enum PacketTypes |
There was a problem hiding this comment.
I'd call this PacketType. Also, can you move the brace up to this line so it looks like enum name {?
| /** | ||
| * EAPOL key types enum. | ||
| */ | ||
| enum KeyTypes { |
| */ | ||
| TINS_BEGIN_PACK | ||
| struct extended_eap_header { | ||
| uint8_t code, id; |
There was a problem hiding this comment.
I think it's probably better to define extended_eap_header as something like (not necessarily with these names):
TINS_BEGIN_PACK
struct extended_eap_header {
eap_header base;
uint8_t type;
} TINS_END_PACK;This way you can make the code simpler below. e.g. when you read, you don't need a temporary eap_header, you can just read straight into eap_hader_.base
|
|
||
| extended_eap_header eap_header_; | ||
|
|
| /** | ||
| * EAP types enum. | ||
| */ | ||
| enum Codes { |
There was a problem hiding this comment.
Same comment about using singular names for enums, use Code here
| /** | ||
| * A number to signify 'no value' for type | ||
| */ | ||
| static const uint8_t invalid_type = 255; |
There was a problem hiding this comment.
It seems like this value is never exposed to a user, is it? If that's the case, it should be private and there should be no mention of it in the API docs, this is an implementation detail.
Skimming through the RFC, it seems like 255 is actually a valid type? It seems like it's an experimental type but it's still valid. This should be handled in a different way. If we could use boost here, I'd use a boost::optional but I'd rather not pull that dependency in here.
| */ | ||
| uint8_t type() const { | ||
| if(eap_header_.type == invalid_type) { | ||
| throw option_not_found(); |
There was a problem hiding this comment.
I'd probably throw another exception here. Maybe field_not_present which already exists could fit here? Also, the docs on this method should say something about this throwing if the PDU doesn't have a type.
There should probably be a bool has_type() const; to check if the frame has a type and not have to rely on a try-catch around calling this method.
Source: mfontanini#303
this is addressing the problem described in this issue of libtins not handling eapol packets that are not rsn\rc4 key exchanges.
for eapol added:
so now it can parse 100% of the pcap file posted in the original issue. (anything past the normal header is passed as RawPDU but parsing every single type of identifier is a MUCH bigger project)
also added new action pdu to dot11 packets so it won't just throw those out.
its very basic parsing (just one extra byte) but at least it won't throw them out