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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snmp-brute.nse: Support non-standard ports, support snmp v1 and v2 #1473

Closed
wants to merge 3 commits into from

Conversation

@usd-markus
Copy link

@usd-markus usd-markus commented Feb 14, 2019

Changes Overview

These changes for the snmp-brute.nse script consist of essentially 3 changes:

  1. Changing the script to also run & work on non-standard ports
  2. Add support for snmp v2 (before, only snmp v1 requests were sent)
  3. Fixing a thrown error when running the script (even before any changes were made)

Each change is represented by one of the commits.

More info to change 3:

After cloning the git repo and trying the snmp-brute script without making any changes to the source code, I got the following error message:

/home/markus/repos/nmap/scripts/snmp-brute.nse:155: Invalid reuse of a socket from one thread to another.
stack traceback:
    [C]: in method 'send'
    /home/markus/repos/nmap/scripts/snmp-brute.nse:155: in function </home/markus/repos/nmap/scripts/snmp-brute.nse:140>

It seems like the socket created in the action function cannot be passed to a new thread anymore. That is why I replaced the thread creation with a direct function call.

Question about changelog

Is the changelog within snmp-brute.nse still used and should it be updated?

@Lem
Copy link

@Lem Lem commented Oct 2, 2019

What is the state of this merge request? I would need that,too, please.

@Reelix
Copy link

@Reelix Reelix commented Jul 26, 2020

After having the original script fail on me (Nmap 7.80) I found this version and can confirm that this version does work whilst the original does not.

The snmp-brute.communitiesdb parameter does not seem to support relative paths in Linux (~/wordlists/bla.txt), although that is probably out of the scope of the script.

@nnposter
Copy link

@nnposter nnposter commented Jul 27, 2020

In commit r37967, all SNMP scripts, not just snmp-brute, have been enabled to run on non-standard ports where SNMP gets detected.

The socket reuse error has been rectified in r37903 (5d281d0) back in February.

The SNMPv2 support has not been addressed.

@Reelix The tilde expansion (or the lack of) is a feature of the invoking shell and has nothing to do with Nmap per se.

@usd-markus
Copy link
Author

@usd-markus usd-markus commented Aug 17, 2020

In commit r37967, all SNMP scripts, not just snmp-brute, have been enabled to run on non-standard ports where SNMP gets detected.

Awesome, thanks 👍

The SNMPv2 support has not been addressed.

Any chance this will be addressed? I could also remove the other stuff that was already addressed from this pull request, so only my changes for snmp v2 support remain, if that's something of interest.

@nnposter
Copy link

@nnposter nnposter commented Aug 18, 2020

This would require some discussion:

  1. What exactly do you mean by "SNMPv2"? v2c? v2u? both? The SNMP library in NSE has no support for v2u and only partial support for v2c (which looks good enough for this purpose).
  2. What specific, real-world need is there for this? How common are targets that only support v2c, but not v1?
  3. Sending both PDUs for each community string is extremely wasteful. It would make more sense to use only v1 by default, but letting the user override it with a script argument.
  4. The PR is actually trying to replace, not augment, v1 with v2c and v2u, which is definitely not desirable (and not working correctly). Possibly you have meant for i = 0, 1 do ?
@usd-markus
Copy link
Author

@usd-markus usd-markus commented Aug 26, 2020

What exactly do you mean by "SNMPv2"? v2c? v2u? both? The SNMP library in NSE has no support for v2u and only partial support for v2c (which looks good enough for this purpose).

Sorry, should have been more clear. SNMPv2c is the desired version to be supported. I don't think there is any need for v2u suport.

What specific, real-world need is there for this? How common are targets that only support v2c, but not v1?

I've seen this on some occasions in the real-word. It would at least be nice to be sure when running the script that it supports the case that only v2c and v1 is supported.

Sending both PDUs for each community string is extremely wasteful. It would make more sense to use only v1 by default, but letting the user override it with a script argument.

Yes, I agree. A script argument would be a good solution here.

The PR is actually trying to replace, not augment, v1 with v2c and v2u, which is definitely not desirable (and not working correctly). Possibly you have meant for i = 0, 1 do ?

You're right, I mixed up the indices there.

@nnposter nnposter self-assigned this Sep 14, 2020
@nnposter
Copy link

@nnposter nnposter commented Sep 14, 2020

Please try the following patch and report back.

The patch implements script argument snmp.version. Use v1 or 0 for SNMPv1 and v2c or 1 for SNMPv2c. The default is SNMPv1. Note that this is a library-level argument so it applies to the majority of the SNMP scripts, not just snmp-brute.

@res0nat0r res0nat0r mentioned this pull request Oct 7, 2020
res0nat0r added a commit to res0nat0r/nmap that referenced this pull request Oct 7, 2020
stef (#1)
* Implement Ncat proxy creds via environment variable. Fixes nmap#2060, closes nmap#2073

* Fix --resume from IPv6 scans

* Use correct default buffer position. Closes nmap#2084

* Clarify upper boundary for variable-length numerical fields

* Make maximize_fdlimit return rlim_t on appropriate platforms. Closes nmap#2085. Fixes nmap#2079

* Credential object is creds.Account, not brute.Account. See nmap#2086

* Clarify location of the Error object

* Use correct default buffer position. Closes nmap#2086

* Minor optimization of url.parse_query()

* Output of matched fingerprints in http-default-accounts. Fixes nmap#2077

* Document that --open implies --defeat-rst-ratelimit since 7.40

* SNMP scripts are enabled on non-standard ports. See nmap#1473

* Increases SQL Server version resolution

* Eliminate reflection false positives in http-shellshock. Closes nmap#2089

* Unify AFP pathname serialization

* Correct AFP name extraction from responses. Closes nmap#2091
FPGetFileDirParms and FPEnumerateExt2 could crash due to unpacking from
out-of-bounds positions. This latent issue got exposed by converting from
bin.unpack to more stringent string.unpack

* Clarified parsing of the volume list in AFP FPGetSrvrParms

* Add cross references between the 2 whois scripts

* Streamline Boolean expressions

* Centralize AFP timestamp conversion to string

* Fix a word-wrapping issue

* Prevent SSH2 KEX confusion. Fixes nmap#2105

* Add ssh2.fetch_host_key() support for group 16

* Handle case of corrupted TCP options with length 0. Fixes nmap#2104

* Add iDRAC9 fingerprint to http-default-accounts. Closes nmap#2096

* fix license url: http -> https

* Implementation of TLS SNI override in Ncat
Closes nmap#2087, closes nmap#1928, fixes nmap#1927, fixes nmap#1974

* Fix off-by-one issue in last change. Fixes nmap#2107

* Be more strict with TCP options parsing, avoid reading off the end of TCP options. See nmap#2107

* Remove nmap-update

This feature was never publicly released, and has not been distributed
in our binary builds for a couple versions now. It needed to be removed
in order to reduce the number of places Nmap looks for data files. See nmap#2051

* If fetchfile didn't find the XSL, use a relative path on all platforms.

* Do not search NMAPDATADIR on Windows as it is not defined. See nmap#2051

* Remove an unused variable

* Require trailing '/' to match a directory name with --script. See nmap#2051

* Stop using Shellshock in header name. Fixes nmap#1983

* Fix line wrapping

* Speed improvement for script afp-ls. Closes nmap#2098

* New option --discovery-ignore-rst. Closes nmap#1616

* Nbase is needed for __attribute__ on Windows

* include string_pool in Windows build

* Use larger buffer size for socket errors (WSAETIMEDOUT was longer).

* Allow multiple UDP payloads per port. Closes nmap#1859 (payloads to be committed later)

* New UDP payloads. Closes nmap#1860

* Use ASCII chars for some payload data where it makes sense

* Pass error along instead of printing (link error)

* OpenSSL 1.1.X renamed libs: libeay32->libcrypto ssleay32->libssl

* More OpenSSL DLL name changes

* One last libeay32->libcrypto name change

* Fix loopback detection on Windows with new Npcap

* Add some popular favicon hashes

* Update nmap-mac-prefixes

* Update nmap-services from IANA

* Handle too-short response in s7-info. See nmap#2117

* Remove a todo item that is done (--resolve-all)

* Update dated 'class' network terms to CIDR. Closes nmap#2054

* Call superclass's init method from derived class

* Use signed value for tcp header offset and option lengths to detect underflow

* Correctly check for unsigned subtraction underflow.

* Add some missing changelog entries

* Tell LGTM to use the correct version of Python (2)

* Process new Linux and OpenBSD fingerprints

* Only get SSL options if we use them, currently for NO_SSLv2

* Process a few service fingerprint submissions

* Add a requested feature

* Try to make sure enough data is present before parsing. See nmap#2117

* Replace hyphens in the client SSH banner
Hyphen is not allowed in the software version string (RFC 4253, section 4.2)

* Update the SSH protocol flow. Closes nmap#1460
Allows the server to start the key exchange before the protocol version
exchange (banner exchange) is completed

* Silence static analysis warning

LGTM points out that since comparison with sizeof(buf) coerces n to
unsigned, all negative values become very large values, which are
necessarily larger than sizeof(buf), so the test is redundant. We still
want the test in our code to be explicit that we are checking for it, so
reordering the comparisons should silence the warning. A good optimizing
compiler should be able to combine the two conditions anyway.

See github/codeql#4249

* Be explicit about truncating division (timeout is in whole milliseconds)

* Improve docs on -Pn and host discovery

"Host discovery" is the preferred term over "ping scan" because of
confusion with ICMP Echo Request, a.k.a. "ping" as used by the "ping"
utility. Warn when users use -Pn because it has negative impact on scan
times since ultrascan timing parameters fall back to slow initial
defaults.

* Fix a config issue with LGTM (libverbs not linked in libpcap)

* Update IPv6 classifier based on new submissions through 2020-09-14

* Fix a meaningless error message when parsing IPv6 extension headers.

* Allow %F date format to mean YYYY-mm-dd like GNU date

* Remove duplicate test conditionals already tested in enclosing block

* Properly handle pcap reads in iocp engine. Fixes nmap#2126

Still has an odd code smell, but this fixes my test case with Nping.

* Add missing prototype

* Make IOCP the default Nsock engine on Windows. See nmap#2126

* Update macosx build to OpenSSL 1.1.1h, use jhbuild for all build steps

* Default rule base for script mysql-audit. See nmap#2125

* Avoid masked use of date before 1/1/1970 UTC. Fixes nmap#2136, closes nmap#2137

* Fix a CHANGELOG typo

* Reintegrate Nmap 7.90 release branch

* Bump version and regen docs for 7.90SVN post-release

* Only warn about protocol specs in port list with -p. Fixes nmap#2135

* Handle a weird IOCP error for UDP sockets. Fixes nmap#2140

Co-authored-by: nnposter <nnposter@e0a8ed71-7df4-0310-8962-fdc924857419>
Co-authored-by: dmiller <dmiller@e0a8ed71-7df4-0310-8962-fdc924857419>
@nnposter
Copy link

@nnposter nnposter commented Oct 9, 2020

The patch has been committed as r38093.

@nmap-bot nmap-bot closed this in e333add Oct 9, 2020
@usd-markus
Copy link
Author

@usd-markus usd-markus commented Oct 13, 2020

Sorry for the late reply. The patch works great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants