Skip to content

Conversation

@fabianbees
Copy link
Contributor

This PR fixes the parsing of the nmap-payloads for UDP scanning.

Currently the HEX-Bytes are not interpreted correctly with the rust build.rs parser.

This issue can be easily verified when comparing the original nmap-payloads (e.g. https://github.com/RustScan/RustScan/blob/master/nmap-payloads#L43) with the generated rust equivalent (e.g. https://github.com/RustScan/RustScan/blob/master/src/generated.rs#L13).

If we convert the rust vec! back to HEX values, we can see that they don't match. This PR fixes this issue.

As a result of this issue rustscan does send incorrect payloads to the services running on the specified port, which causes the service to not answer such an invalid request. Therefor rustscan does not find some open UDP ports.

Note

I also noticed, that currently only the last entry for a given port form the nmap-payloads is used, if there are multiple available payloads for a port.
The previous payload is overwritten (here: https://github.com/RustScan/RustScan/blob/master/build.rs#L228), so only the last payload for a port is included in generated.rs.
Maybe I will look into this in the future ...

For now I have just moved the SNMPv3GetRequest down (so that it is used first), as it had better results for findig open SNMP (161/UDP) ports.

@fabianbees
Copy link
Contributor Author

fabianbees commented Feb 10, 2025

One more thought: As the generated.rs File is automatically generated in the build process (via build.rs) and therefore not really source code which should be tracked in a Git repo, I would suggest excluding this file via .gitignore and removing it from the repo.

What are your opinions on that?

@PsypherPunk
Copy link
Collaborator

@fabianbees, this looks good; happy to merge but just a couple of suggestions:

  1. The issue you mentioned around only picking up the last payload is worth logging/addressing as a separate issue: I'd suggest reverting the changes to nmap-payloads and dealing with it directly (that file was taken directly from Nmap and if we start modifying it, we start having to maintain it and that rather defeats the point right now).
  2. your suggestion around removing/ignoring generated.rs is a good one: can you raise another issue/PR for that?
    • it'd be worth checking this behaves as expected: I've just tried this here and it seems to ignore subsequent changes to ignored files but I've definitely run into issues where .gitignore doesn't behave quite as you'd think with previously-modified files.

@fabianbees fabianbees force-pushed the fix/nmap-payloads-hex-conversion-error branch from c452e92 to a517eff Compare February 12, 2025 11:33
@fabianbees fabianbees force-pushed the fix/nmap-payloads-hex-conversion-error branch from a517eff to e797458 Compare February 12, 2025 11:36
@fabianbees
Copy link
Contributor Author

@fabianbees, this looks good; happy to merge but just a couple of suggestions:

Perfect!

  1. The issue you mentioned around only picking up the last payload is worth logging/addressing as a separate issue: I'd suggest reverting the changes to nmap-payloads and dealing with it directly (that file was taken directly from Nmap and if we start modifying it, we start having to maintain it and that rather defeats the point right now).

@PsypherPunk I reverted my change from nmap-payloads, so this can be merged now.

  1. your suggestion around removing/ignoring generated.rs is a good one: can you raise another issue/PR for that?

    • it'd be worth checking this behaves as expected: I've just tried this here and it seems to ignore subsequent changes to ignored files but I've definitely run into issues where .gitignore doesn't behave quite as you'd think with previously-modified files.

I will open a PR with this change in the next few minutes referencing your suggestion.

Copy link
Collaborator

@PsypherPunk PsypherPunk left a comment

Choose a reason for hiding this comment

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

LGTM!

@PsypherPunk PsypherPunk merged commit 71091bf into bee-san:master Feb 12, 2025
4 checks passed
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

Successfully merging this pull request may close these issues.

2 participants