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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TCP protocol to SNMP login scanner

Closed
wants to merge 1 commit into from

Conversation

CaptainFreak
Copy link

@CaptainFreak CaptainFreak commented Oct 21, 2018

This aims to add TCP protocol to snmp login scanner as requested in #9649 .
I tried to test the initial code without this changes on host running SNMP over UDP but couldn't get any result. But SNMP is properly configured and can be verified from following snap :

snmp

and MSF snmp scanner errors out with

msferror

Added the initial code with reference to already existing code for UDP.
will need help in following thing to polish this more :

  • Submitting Snmp GETREQUEST over TCP in ruby.
  • Checking port type (UDP/TCP )even before firing packets.

May require lot of work to be able to merge. This is my first PR of many to come to this awesome Project 馃槃

Cheers 馃嵒

@CaptainFreak CaptainFreak changed the title Initial code for adding TCP to snmp login scanner Add TCP protocol to SNMP login scanner Oct 21, 2018
@CaptainFreak
Copy link
Author

CaptainFreak commented Oct 26, 2018

Any guidance @sinn3r @busterb ? 馃槂

@jrobles-r7 jrobles-r7 self-assigned this Nov 16, 2018
@jmartin-r7
Copy link
Member

jmartin-r7 commented Nov 21, 2018

Jenkins test this please.

@busterb busterb self-assigned this Nov 26, 2018
@busterb
Copy link
Contributor

busterb commented Nov 26, 2018

Looks reasonable, but there does not seem to be a way to select or disable connecting with both protocols in parallel. Do you think there should be a way to select them?

@busterb
Copy link
Contributor

busterb commented Nov 28, 2018

Answering my own question, I see you have it as a checkbox

Checking port type (UDP/TCP )even before firing packets.

@busterb
Copy link
Contributor

busterb commented Nov 29, 2018

It looks like the bigger issue is all of the UDP packets sent by this scanner appear to have an incorrect checksum, which explains why you originally saw the scanner error out as well.

EDIT - hah, fooled again by Checksum offload, that looks fine.

@CaptainFreak
Copy link
Author

CaptainFreak commented Dec 1, 2018

@busterb can you give a check-off list to make this merge-able ? 馃槂

@CaptainFreak
Copy link
Author

CaptainFreak commented Dec 16, 2018

Any suggestion ?

@CaptainFreak
Copy link
Author

CaptainFreak commented Dec 27, 2018

Please have a look at this.

@busterb
Copy link
Contributor

busterb commented Jan 9, 2019

  • Ideally, this should be a configurable option, not something that is enabled by unconditionally. Make it an enum for 'UDP', 'TCP', 'UDP or TCP'
  • If 'UDP or TCP' is enabled, then TCP should only fire if UDP does not receive a reply. There's not much reason to do a TCP connect after a successful UDP connect.

@busterb
Copy link
Contributor

busterb commented Jan 9, 2019

The real issue that also side-tracked me when reviewing this was that this scanner only supports SNMPv2, which is pretty obsolete, even among the already-obsolete gear I tested it against which only supported SNMPv3. It made me question whether a higher-priority fix for this module would be in adding SNMPv3 support rather than TCP support.

Tl;DR - I'd find SMNPv3 support more useful in this module than TCP today. At least it'd be easier to test in real-world scenarios.

@busterb
Copy link
Contributor

busterb commented Feb 20, 2019

I took a good stab at fixing this up this morning, but there are some additional problems I found during testing. Namely, this uses an unconnected tcp socket via sendto, which doesn't appear to send any data, then uses recvfrom with a fixed buffer size, which when you do fix the socket connection problem, causes the whole scanner to hang indefinitely.

I'm going to close this one for now. I think my best overall suggestion is that the scanner code here needs a lot more work than just swapping in the socket type. I might even suggest evaluating existing SNMP implementations for Ruby that actually implement the protocol properly, and changing the login scanner to use one of those instead of the minimal implementation present inside of this login scanner.

https://rubygems.org/search?utf8=%E2%9C%93&query=snmp

Thanks.

@busterb busterb closed this Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants