Add PE32/PE32+ risk detection (detect transmitted windows executables).

[utoni]

Please sign (check) the below before submitting the Pull Request:

• I have signed the ntop Contributor License Agreement at https://github.com/ntop/legal/blob/main/individual-contributor-licence-agreement.md
• I have read the contributing guide lines at https://github.com/ntop/nDPI/blob/dev/CONTRIBUTING.md
• I have updated the documentation (in doc/) to reflect the changes made (if applicable)

Link to the related issue:

Describe changes:

The risk detection is far from perfect e.g. the amount of packets it does look for is hard coded. I think it might be better to let the user decide how many packets should checked for a possible PE transmission. It also assumes that the first 0x3C + 4 bytes are transmitted as one packet e.g. w/o fragmentation/retransmission.

[0xA50C1A1]

I think it makes sense to also add HTTP content-type check for application/vnd.microsoft.portable-executable.

[utoni]

I think there is already such check in src/lib/protocols/http.c. But it may be useful to check the HTTP body for PE32/PE32+. What do you think?

[0xA50C1A1]

I think there is already such check in src/lib/protocols/http.c. But it may be useful to check the HTTP body for PE32/PE32+. What do you think?

Yeah, that's a great idea.

[0xA50C1A1]

I think there is already such check in src/lib/protocols/http.c.

Yes, it's there, but I meant something else: to classify flows with that content-type as Portable_Executable.

[utoni]

I think there is already such check in src/lib/protocols/http.c.

Yes, it's there, but I meant something else: to classify flows with that content-type as Portable_Executable.

Yea, makes also sense. I'll do both.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[IvanNardi]

I'll do a proper review later, but I have an initial question: is there any specific reasons why this PE stuff is treated as a new protocol instead of a new flow risk?
[It seems that the former allows a simpler code for the generic UDP/TCP case]
I might have a preference for the flow risk solution (PE seems a specific case of "binary transfer risk"] but I don't have strong opinions and I am fine with both solutions. I would like to know your reasoning about that...

[utoni]

I'll do a proper review later, but I have an initial question: is there any specific reasons why this PE stuff is treated as a new protocol instead of a new flow risk? [It seems that the former allows a simpler code for the generic UDP/TCP case] I might have a preference for the flow risk solution (PE seems a specific case of "binary transfer risk"] but I don't have strong opinions and I am fine with both solutions. I would like to know your reasoning about that...

Initially, I did not want to create a new protocol id for that.
But: The PCAP file shows a plaintext executable xfer, which was not classified before.
At first I've just wanted to write a protocol dissector that does not classify a protocol, but set's a risk.
It seems that there is a limitation in the core which prevents to do such things. So I've decided to create a new protocol id for it.
But I am open to revert that new protocol id creation if we can at least set the risk if a PE32/PE32+ executable is transmitted.

[IvanNardi]

I'll do a proper review later, but I have an initial question: is there any specific reasons why this PE stuff is treated as a new protocol instead of a new flow risk? [It seems that the former allows a simpler code for the generic UDP/TCP case] I might have a preference for the flow risk solution (PE seems a specific case of "binary transfer risk"] but I don't have strong opinions and I am fine with both solutions. I would like to know your reasoning about that...

Initially, I did not want to create a new protocol id for that. But: The PCAP file shows a plaintext executable xfer, which was not classified before. At first I've just wanted to write a protocol dissector that does not classify a protocol, but set's a risk. It seems that there is a limitation in the core which prevents to do such things. So I've decided to create a new protocol id for it. But I am open to revert that new protocol id creation if we can at least set the risk if a PE32/PE32+ executable is transmitted.

Just an idea. Your current code basically checks for PE transfer only for unknown traffic (because when/if the flow is classified, PE dissector is not called anymore). [let's ignore the HTTP case which seems easy to handle]
If we want that, we could remove the new dissector/protocol and add something like that

static ndpi_protocol ndpi_internal_detection_process_packet(...) {
[...]
  /* At the end of this function */
  if(ret.app_protocol == NDPI_PROTOCOL_UNKNOWN) {
    /* just the code that is right now in ndpi_search_portable_executable and if we found it we set a risk */
  }
}

This way, we don't need a new protocol and we can set a risk.
Just an idea

[utoni]

@IvanNardi
Again, I forgot a PR.
Need some kind of stale PR alert or so..

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[IvanNardi]

Thank you