Ecubit/fix false positives #262
Conversation
…t-censys/zgrab2 into ecubit/fix-false-positives
|
/cc @LizIzhikevich for 👀 since she caught many of these originally |
There was a problem hiding this comment.
Looking pretty good Elliot! My comments revolve around the different definition of "success" for zgrab2 versus an actual protocol implementation; our "success" is really whether we can positively identify a given protocol. To that end, errors that are well-defined by the protocol should never cause us to not send back results.
modules/imap/imap.go
Outdated
| return s, nil | ||
| } | ||
| if strings.HasPrefix(s, "* NO"){ | ||
| return s, zgrab2.NewScanError(zgrab2.SCAN_APPLICATION_ERROR, errors.New("IMAP reported error")) |
There was a problem hiding this comment.
This isn't actually an error, at least for us. Since the goal of zgrab2 is to identify services, receiving a well-formed error response is a good thing, as it is a positive indication that (in this case) we are talking to an imap server. (This also applies to the * BAD case, as well).
There is also * PREAUTH and * BYE
modules/pop3/pop3.go
Outdated
| return s, nil | ||
| } | ||
| if strings.HasPrefix(s, "+ERR "){ | ||
| return s, zgrab2.NewScanError(zgrab2.SCAN_APPLICATION_ERROR, errors.New("POP3 Reported Error")) |
There was a problem hiding this comment.
Similar comment as for imap; this is actually a success case
There was a problem hiding this comment.
Based on the contents of the modbus scanner, it seems that SCAN_APPLICATION_ERROR is a success case - indicating that a valid response was received but the server reported back an error in the final ScanResult
There was a problem hiding this comment.
Though I just realized that string should be "-ERR" regardless
There was a problem hiding this comment.
SCAN_APPLICATION_ERROR can be treated as a "success" error case; the real indication is if the scanner returns nil for the results or not. So it would be OK in this and the IMAP module to return SCAN_APPLICATION_ERROR here, except that in both of them scanner.go will omit results.
For example, from pop3/scanner.go:
banner, err := conn.ReadResponse()
if err != nil {
return zgrab2.TryGetScanStatus(err), nil, err
}
modules/smtp/smtp.go
Outdated
| return s, err | ||
| } | ||
| if code < 200 || code >= 300 { | ||
| return s, zgrab2.NewScanError(zgrab2.SCAN_APPLICATION_ERROR, fmt.Errorf("SMTP returned error code %d", code)) |
There was a problem hiding this comment.
Similar comment; if we wanted to vet that it is a valid code, anything returned as a code from the SMTP RFC would qualify as success. However, I wonder if we even need to be that strict (in case there are non-standard extensions out there)?
ghost
requested a review
from
modules/imap/scanner.go
Outdated
| // Check the contents of the IMAP banner and return a relevant ScanStatus | ||
| func VerifyIMAPContents(banner string) zgrab2.ScanStatus { | ||
| switch { | ||
| case strings.HasPrefix(banner, "* OK"), |
There was a problem hiding this comment.
Did some searching on censys.io, and some imap servers actually return OKAY as well: https://censys.io/ipv4/198.133.159.137/table#143
We should also match on a strings.Contains(banner, "IMAP)?
https://censys.io/ipv4/155.230.11.8/table#143
* telnet module should not return success when it finds something other than telnet * telnet module should not return success when it finds something other than telnet * Adds verification step for POP3 banners * Add validation for IMAP banners & fix some formatting problems * Verify SMTP banners exist and are successful * Add check for is_dnp3 flag which seems to be working as expected * Fix dropping SCAN_APPLICATION_ERROR in IMAP * Fix dropping SCAN_APPLICATION_ERROR in POP3 * Fix dropping SCAN_APPLICATION_ERROR in SMTP * Add protocol and blacklist indicators to email protocols Co-authored-by: Elliot Cubit <elliotcubit@elliots-mbp.lan>
Contents
Check contents of banners to make sure they match their protocol before returning success.
Fixes telnet, ssh, smtp, pop3, imap, and dnp3
How to Test
Test each protocol against a few protocol servers and ssh running on localhost to verify results:
Docker images used:
Telnet: jaredharringtongibbs/telnet-server on :23
pop3, imap: instrumentisto/dovecot on :110 and :143
smtp: bytemark/smtp on :25
Testing for pop3:
$ nc -l 9999 &
$ ./zgrab2 pop3 --port 9999 --timeout 2 <<< "127.0.0.1"
$ ./zgrab2 pop3 --port {23|143|25|22} --timeout 2 <<< "127.0.0.1"
$ > success = false
$ ./zgrab2 pop3 --port 110 --timeout 2 <<< "127.0.0.1"
$ > success = true
Repeat for each protocol, only port running correct protocol should have success=true.
Didn't have a dnp3 server locally to test with; found an open system at 216.164.167.23 to test against for the success case.