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

Adding Vulnerability Scans to Scan #453

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions scan/scan_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,12 @@ type FamilySet map[string]*Family

// Default contains each scan Family that is defined
var Default = FamilySet{
"Connectivity": Connectivity,
"TLSHandshake": TLSHandshake,
"TLSSession": TLSSession,
"PKI": PKI,
"Broad": Broad,
"Vulnerability": Vulnerability,
"Connectivity": Connectivity,
"TLSHandshake": TLSHandshake,
"TLSSession": TLSSession,
"PKI": PKI,
"Broad": Broad,
}

// ScannerResult contains the result for a single scan.
Expand Down
119 changes: 119 additions & 0 deletions scan/vulnerability.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package scan

import (
"crypto/rsa"
"crypto/x509"
"errors"
"net/url"

"github.com/FiloSottile/Heartbleed/heartbleed"
"github.com/cloudflare/cfssl/certinfo"
"github.com/cloudflare/cfssl/helpers"
)

// Vulnerability contains scanners to detect vulnerabilities such as Heartbleed
var Vulnerability = &Family{
Description: "Scans a host to detect TLS vulnerabilities.",
Scanners: map[string]*Scanner{
"Heartbleed": {
"Scan for Heartbleed vulnerability",
heartbleedScan,
},
"BERserk": {
"Scan for BERserk vulnerability",
BERserkScan,
},
"LogJam": {
"Scan for LogJam vulnerability",
LogJamScan,
},
},
}

func heartbleedScan(addr, hostname string) (grade Grade, output Output, err error) {
tgt := &heartbleed.Target{
Service: "https",
HostIp: hostname,
}

u, err := url.Parse(tgt.HostIp)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's assumed that hostname will never have a scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I'll be sure to take that out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need some err handling after this?

if err == nil && u.Host != "" {
tgt.HostIp = u.Host
}

out, err := heartbleed.Heartbleed(tgt,
[]byte("github.com/FiloSottile/Heartbleed"), true)
if err == heartbleed.Safe {
Copy link
Contributor

Choose a reason for hiding this comment

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

A switch statement might be a bit more clear here

grade = Good
err = nil
} else if err != nil {
grade = Warning
} else {
grade, output = Bad, out
}
return
}

func BERserkScan(addr, hostname string) (grade Grade, output Output, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like Travis failed here, since this function name's first letter is capitalized, add a comment above it or uncapitalized it. Reference: http://www.goinggo.net/2014/03/exportedunexported-identifiers-in-go.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. I'll get to that, thanks.

// Vulnerable if certificate has a 3 as the public exponent
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not a valid indicator of BERserk vulnerability. BERserk is a client side vuln, and the existence of ANY e=3 trusted roots make ALL websites vulnerable. (Modulo pinning, if you will, but I don't think that's ground for saying that e=3 is a server vulnerability.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for pointing that out.

// get certificate from host via certinfo

certData, err := certinfo.ParseCertificateDomain(hostname)
if err != nil {
grade = Bad
Copy link
Contributor

Choose a reason for hiding this comment

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

Bad is (very intentionally) the zero value of the Grade type, so almost all of these grade = Bad statements are noops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see.

return
}

ca, err := helpers.ParseCertificatePEM([]byte(certData.RawPEM))
if err != nil {
grade = Bad
return
}

if ca.PublicKeyAlgorithm != x509.RSA {
grade = Good
}

pub, ok := ca.PublicKey.(*rsa.PublicKey)
if !ok {
grade = Good
}

if pub.E == 3 {
grade = Bad
err = errors.New("certificate has RSA public exponent 3 and is vulnerable")
}

if pub.N.BitLen() == 1024 || pub.N.BitLen() == 2048 {
grade = Bad
err = errors.New("certificate has bad public key length")
}
return
}

func LogJamScan(addr, hostname string) (grade Grade, output Output, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above

// do a DHE handshake and look at the public exponent used by the server.
// If it’s 1024 bits, then it’s vulnerable.

//DHE handshake?
Copy link
Contributor

Choose a reason for hiding this comment

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

Go's crypto/tls doesn't support DHE TLS handshakes, you'll need to use https://github.com/cloudflare/cf-tls/blob/master/tls/cfsslscan_handshake.go to construct a custom client hello and then read the server hello ServerDHParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacobhaven Hmm, what does ServerDHParams refer to? I assume it's the cert returned by SayHello(), but I am having some trouble reading the public key length from that cert.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the Diffie Hellman Parameters are in a separate Server Key Exchange TLS message. You'll need to read and parse that message and detect if dh_p is under 1024 bits.

certData, err := certinfo.ParseCertificateDomain(hostname)
if err != nil {
grade = Bad
return
}

ca, err := helpers.ParseCertificatePEM([]byte(certData.RawPEM))
if err != nil {
grade = Bad
return
}

pub, _ := ca.PublicKey.(*rsa.PublicKey)
if pub.N.BitLen() == 1024 {
grade = Bad
err = errors.New("unsupported public key length")
return
}
grade = Good
return
}