Skip to content

Conversation

BrendanGlancy
Copy link
Contributor

This is a work in progress; I don't know if all the packets are being generated correctly, as it is hard to test.

I am looking to get more eyes on this, as I have already made several decisions myself that other more experienced contributors could've made better calls on.

Basically, we are creating a BTreeMap of [Ports[], Payloads[]], which works better than anything else I have put together so far in terms of UDP scanning accuracy.

For example, port 67 and 68 are common DHCP ports so sending them the same packet to test for a response is easy with the format:

[[67,68],[payload]]

I am fairly certain I am not violating the Nmap license by using this payload database, as Rustscan is also free software, and this database is deprecated.

The current, more complete Nmap service probing database is incredibly difficult to parse, and incorporating something like that into Rustscan would cause the project to become rigid. This older version allows easy contribution in terms of adding UDP payloads and could be fairly easy to adapt to something like Yaml.

@BrendanGlancy
Copy link
Contributor Author

@PsypherPunk not sure how to request review besides tag

@PsypherPunk
Copy link
Collaborator

@BrendanGlancy, I'm away for a few days but will take a look ASAP. Looks like the build is failing at the moment though so probably worth determining where that's coming from.

On a less fun note, we might need to clarify the position around reusing Nmap's code. Although it's open-source, it uses its own "Nmap Public Source License" which has this explicit clause:

These additional terms mean that You may not distribute Covered Software or Derivative Works under plain GPL terms without special permission from Licensor.

…and as RustScan is GPL, that might rule this out.

@bee-san
Copy link
Owner

bee-san commented Aug 22, 2024

@BrendanGlancy, I'm away for a few days but will take a look ASAP. Looks like the build is failing at the moment though so probably worth determining where that's coming from.

On a less fun note, we might need to clarify the position around reusing Nmap's code. Although it's open-source, it uses its own "Nmap Public Source License" which has this explicit clause:

These additional terms mean that You may not distribute Covered Software or Derivative Works under plain GPL terms without special permission from Licensor.

…and as RustScan is GPL, that might rule this out.

That license specifically defines "GPL" as GPL version 2

  • "GPL" means the GNU General Public License Version 2, as published
    by the Free Software Foundation and provided in Exhibit A.

Under "Definitions"

RustScan uses GPL 3.0 https://github.com/RustScan/RustScan?tab=GPL-3.0-1-ov-file#readme

Also, their document here mentions
https://nmap.org/npsl/

That it's "open sourcd" and links to this definition:
https://opensource.org/osd

Which says:

The license must allow modifications and derived works, and must allow them to be distributed under the same terms as the license of the original software.

I do not see any issues here, so long as we do not commercialise RustScan with private code it will be ok :)

Reading their website too their personal opinion seems to be "if you're an open source project you can use our stuff, just don't make it closed source and sell it or try to sell licenses of nmap" etc. So even if there was a questionable license conflict, I think they would be ok with it so long as our intention is to not do something evil 😂

Copy link
Owner

@bee-san bee-san left a comment

Choose a reason for hiding this comment

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

just 2 initial comments :D

@@ -0,0 +1,159 @@
use std::collections::BTreeMap;
Copy link
Owner

Choose a reason for hiding this comment

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

  1. We should have comments on what this file does :)
  2. I am not a super big fan of using Rust to build files :( We should use something like Bash or Python I think, as it's easier to read & contribute to?

map.insert(vec![31337], vec![206,99,209,210,22,231,19,207,56,165,165,134,178,117,75,153,170,50,88]);
map.insert(vec![34555], vec![]);
map.insert(vec![64738], vec![0,0,0,0,171,205,239]);
map
Copy link
Owner

Choose a reason for hiding this comment

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

this is a big file, do we have to make this at run time? can we not build this before hand and consume it with yaml or something? or even have a perfect hash map https://skerritt.blog/perfect-hash-tables/ ?

Copy link
Contributor Author

@BrendanGlancy BrendanGlancy Aug 22, 2024

Choose a reason for hiding this comment

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

I am not a super big fan of using Rust to build files :( We should use something like Bash or Python I think, as it's easier to read & contribute to?

Do you mean rewriting the entire file parser in Python?

I not entirely sure how this could be done, maybe you know @HDTHREE

this is a big file, do we have to make this at run time? can we not build this before hand and consume it with yaml or something? or even have a perfect hash map https://skerritt.blog/perfect-hash-tables/ ?

A couple of things could happen with this file; we could .gitignore it. I cannot get any version of the Rust LSP to format it for me, so I think it breaks part of the CI. I was super weirded out by the whole code generation at compile time thing, but yes the way we are actually consuming and creating this map at runtime is something I can work on.

@BrendanGlancy, I'm away for a few days but will take a look ASAP. Looks like the build is failing at the moment though so probably worth determining where that's coming from.

@PsypherPunk && Bee no rush on any of this, just wanted to get your thoughts on this before I moved any further forward as this PR is already the size of the Epstein list.

Copy link
Owner

Choose a reason for hiding this comment

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

Do you mean rewriting the entire file parser in Python?

I am a bit sick so I didn't take a proper look, but I think your build.rs parses the nmap file and turns it into another Rust file to use, right?

typically for scripts like that I see them as Python / Bash and they go into a seperate /scripts folder or something or maybe fixtures 🤔

A couple of things could happen with this file; we could .gitignore it. I cannot get any version of the Rust LSP to format it for me, so I think it breaks part of the CI. I was super weirded out by the whole code generation at compile time thing, but yes the way we are actually consuming and creating this map at runtime is something I can work on.

yeah that weirds me out too, i think generally the PR looks fine though :)

@PsypherPunk && Bee no rush on any of this, just wanted to get your thoughts on this before I moved any further forward as this PR is already the size of the Epstein list.

PR is failing partially because cargo format cant format build,rs, another win for not putting it into rust ;)

i think trying one of these:
https://www.reddit.com/r/rust/comments/1c6ceuu/how_to_force_cargo_fmt_to_ignore_some_files/

and then running cargo format locally to see if it works or not.

other issue is casued by #527

this has a fix:

The temporary solution is
change
let arguments = shell_words::split(&to_run).expect("Failed to parse script arguments");
to
let arguments:Vec =to_run.split_whitespace().map(|s| s.to_string()).collect();

does that work? 🤔

Copy link
Contributor Author

@BrendanGlancy BrendanGlancy Aug 22, 2024

Choose a reason for hiding this comment

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

I am a bit sick so I didn't take a proper look, but I think your build.rs parses the nmap file and turns it into another Rust file to use, right?

typically for scripts like that I see them as Python / Bash and they go into a seperate /scripts folder or something or maybe fixtures 🤔

The problem with using python for this is, and I am not 100% sure about this, we will not be able to put the values for the BTreeMap (or whatever datastructure) in memory during compile time.

Copy link
Owner

Choose a reason for hiding this comment

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

ah you're right, I am silly :)

@bee-san
Copy link
Owner

bee-san commented Aug 26, 2024

Concerning it's breaking on Windows... I was hoping it would be temporary.......

image

Does this mean it fails when trying to compile futures-lite?

once_cell does appear to compile for windows :(

Edit: Unless it's trying to compile (run) build.rs at compile time and that's failing? What happens if we don't run that and just include the file in the repo 😂

this is not a fun bug

@BrendanGlancy
Copy link
Contributor Author

BrendanGlancy commented Aug 26, 2024

Oh, sorry, I got distracted. I made the Python version of the parser over the weekend. (https://github.com/BrendanGlancy/RustScan/blob/dev/services.py) It kind of makes everything even more complicated

I don't have a Windows box; I'll have to spin up a VM and look into this. But...

Ideally, this was just supposed to work, and we could move on to TCP service probing; I should've turned on the CI stuff on my branch before I submitted this.

I think the solution is to hand roll our own service probing 'database.' Should we use YAML?

Don't think we need to use once_cell, just was allowing me to be lazy

@BrendanGlancy BrendanGlancy marked this pull request as draft August 27, 2024 18:26
@BrendanGlancy
Copy link
Contributor Author

@bee-san what is up with the Linux CI test? I'm not even sure what could be causing that

@bee-san
Copy link
Owner

bee-san commented Aug 30, 2024

@bee-san what is up with the Linux CI test? I'm not even sure what could be causing that

semver violation, let me re-run it. in semantic versioning something like "1.0.0dev" is not allowed. I can't find proof of this anywhere, so I am rerunning in case it just broke :)

also weird it only happened on linux, i think the CI just broke and not your code

@bee-san
Copy link
Owner

bee-san commented Aug 30, 2024

ok this is weird... i am not sure...... :(

@PsypherPunk
Copy link
Collaborator

In the env for the failing step you can see TAG: dev. In the previous step that if clause must not be firing so the name is staying at its default of dev.

Looks like it's passing on macos as that step would appear to miss macos entirely (!?); passing on windows as it doesn't make use of TAG.

Oh, hang on: should this even be running? Do we actually want to make a release based on pull-request? Build/test definitely, but actually release…? The above is likely to be because it was a pull-request that triggered it, not a tag (therefore there's no Git tag from which to derive TAG).

@bee-san
Copy link
Owner

bee-san commented Aug 30, 2024

In the env for the failing step you can see TAG: dev. In the previous step that if clause must not be firing so the name is staying at its default of dev.

Looks like it's passing on macos as that step would appear to miss macos entirely (!?); passing on windows as it doesn't make use of TAG.

Oh, hang on: should this even be running? Do we actually want to make a release based on pull-request? Build/test definitely, but actually release…? The above is likely to be because it was a pull-request that triggered it, not a tag (therefore there's no Git tag from which to derive TAG).

I think @CMNatic wanted to do PR-based releases for the docker image? We can disable that CI though I don't mind so long as no one else does!!

Thank you for your investigation

@BrendanGlancy BrendanGlancy marked this pull request as ready for review September 3, 2024 18:37
@BrendanGlancy
Copy link
Contributor Author

@bee-san So I think at this point everything is fixed except the dev tag in the CI thing

@bee-san bee-san merged commit 45fa80a into bee-san:master Sep 13, 2024
11 of 12 checks passed
@bee-san
Copy link
Owner

bee-san commented Sep 13, 2024

@BrendanGlancy talked to some people, we think its cause its in a PR it doesn't like it but hopefully on release it will like it haha

@bee-san
Copy link
Owner

bee-san commented Sep 13, 2024

merged!!

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.

3 participants