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

Expose detector-specific false positive logic #2743

Merged
merged 17 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions pkg/custom_detectors/custom_detectors.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type CustomRegexWebhook struct {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*CustomRegexWebhook)(nil)
var _ detectors.CustomFalsePositiveChecker = (*CustomRegexWebhook)(nil)

// NewWebhookCustomRegex initializes and validates a CustomRegexWebhook. An
// unexported type is intentionally returned here to ensure the values have
Expand Down Expand Up @@ -109,6 +110,10 @@ func (c *CustomRegexWebhook) FromData(ctx context.Context, verify bool, data []b
return results, nil
}

func (c *CustomRegexWebhook) IsFalsePositive(_ detectors.Result) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: If all parameters are ignored, you can omit _. You can even omit c in the pointer receiver.

func (*CustomRegexWebhook) IsFalsePositive(detectors.Result) bool

return false
}

func (c *CustomRegexWebhook) createResults(ctx context.Context, match map[string][]string, verify bool, results chan<- detectors.Result) error {
if common.IsDone(ctx) {
// TODO: Log we're possibly leaving out results.
Expand Down
5 changes: 5 additions & 0 deletions pkg/detectors/azurebatch/azurebatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type Scanner struct {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
defaultClient = common.SaneHttpClient()
Expand Down Expand Up @@ -105,6 +106,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
return results, nil
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}

func (s Scanner) Type() detectorspb.DetectorType {
return detectorspb.DetectorType_AzureBatch
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Scanner struct {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
defaultClient = common.SaneHttpClient()
Expand Down Expand Up @@ -94,6 +95,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
return results, nil
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}
Comment on lines +98 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you consider creating a struct that implements this so it can be embedded into the Scanner struct?

// in detectors package
type SkipFalsePositiveCheck struct{}

func (SkipFalsePositiveCheck) IsFalsePositive(Result) bool {
    return false
}

// elsewhere

type Scanner struct {
     detectors.SkipFalsePositiveCheck
}

Copy link
Collaborator Author

@rosecodym rosecodym May 1, 2024

Choose a reason for hiding this comment

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

no, i didn't! does that save any implementation effort?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Marginally? imo it's a "nice-to-have" for a common scenario, especially for areas of the code that get a lot of outside contribution.


func (s Scanner) Type() detectorspb.DetectorType {
return detectorspb.DetectorType_AzureContainerRegistry
}
50 changes: 23 additions & 27 deletions pkg/detectors/falsepositives.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,17 @@ import (
"unicode/utf8"

ahocorasick "github.com/BobuSumisu/aho-corasick"
"github.com/trufflesecurity/trufflehog/v3/pkg/pb/detectorspb"

"github.com/trufflesecurity/trufflehog/v3/pkg/context"
)

var DefaultFalsePositives = []FalsePositive{"example", "xxxxxx", "aaaaaa", "abcde", "00000", "sample", "www"}

type FalsePositive string

type CustomFalsePositiveChecker interface {
IsFalsePositive(result Result) bool
}

//go:embed "badlist.txt"
var badList []byte

Expand All @@ -43,6 +45,17 @@ func init() {
filter = builder.Build()
}

func GetFalsePositiveCheck(detector Detector) func(Result) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat!

checker, ok := detector.(CustomFalsePositiveChecker)
if ok {
return checker.IsFalsePositive
}

return func(res Result) bool {
return IsKnownFalsePositive(string(res.Raw), DefaultFalsePositives, true)
}
}

// IsKnownFalsePositive will not return a valid secret finding if any of the disqualifying conditions are met
// Currently that includes: No number, english word in key, or matches common example pattens.
// Only the secret key material should be passed into this function
Expand Down Expand Up @@ -132,34 +145,17 @@ func FilterResultsWithEntropy(ctx context.Context, results []Result, entropy flo
}

// FilterKnownFalsePositives filters out known false positives from the results.
func FilterKnownFalsePositives(ctx context.Context, results []Result, falsePositives []FalsePositive, wordCheck bool, shouldLog bool) []Result {
func FilterKnownFalsePositives(ctx context.Context, detector Detector, results []Result, shouldLog bool) []Result {
var filteredResults []Result

isFalsePositive := GetFalsePositiveCheck(detector)

for _, result := range results {
if !result.Verified {
switch result.DetectorType {
case detectorspb.DetectorType_CustomRegex:
filteredResults = append(filteredResults, result)
case detectorspb.DetectorType_GCP,
detectorspb.DetectorType_URI,
detectorspb.DetectorType_AzureBatch,
detectorspb.DetectorType_AzureContainerRegistry,
detectorspb.DetectorType_Shopify,
detectorspb.DetectorType_Postgres,
detectorspb.DetectorType_MongoDB,
detectorspb.DetectorType_JDBC:
if !result.Verified && result.Raw != nil {
if !isFalsePositive(result) {
filteredResults = append(filteredResults, result)
default:
if result.Raw != nil {
if !IsKnownFalsePositive(string(result.Raw), falsePositives, wordCheck) {
filteredResults = append(filteredResults, result)
} else {
if shouldLog {
ctx.Logger().Info("Filtered out known false positive", "result", result)
}
}
} else {
filteredResults = append(filteredResults, result)
}
} else if shouldLog {
ctx.Logger().Info("Filtered out known false positive", "result", result)
}
} else {
filteredResults = append(filteredResults, result)
Expand Down
53 changes: 53 additions & 0 deletions pkg/detectors/falsepositives_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,63 @@
package detectors

import (
"context"
_ "embed"
"testing"

"github.com/stretchr/testify/assert"
logContext "github.com/trufflesecurity/trufflehog/v3/pkg/context"
"github.com/trufflesecurity/trufflehog/v3/pkg/pb/detectorspb"
)

type fakeDetector struct{}
type customFalsePositiveChecker struct{ fakeDetector }

func (d fakeDetector) FromData(ctx context.Context, verify bool, data []byte) ([]Result, error) {
return nil, nil
}

func (d fakeDetector) Keywords() []string {
return nil
}

func (d fakeDetector) Type() detectorspb.DetectorType {
return detectorspb.DetectorType(0)
}

func (d customFalsePositiveChecker) IsFalsePositive(result Result) bool {
return IsKnownFalsePositive(string(result.Raw), []FalsePositive{"a specific magic string"}, false)
}

func TestFilterKnownFalsePositives_DefaultLogic(t *testing.T) {
results := []Result{
{Raw: []byte("00000")}, // "default" false positive list
{Raw: []byte("number")}, // from wordlist
{Raw: []byte("hga8adshla3434g")}, // real secret
}
expected := []Result{
{Raw: []byte("hga8adshla3434g")},
}
filtered := FilterKnownFalsePositives(logContext.Background(), fakeDetector{}, results, false)
assert.ElementsMatch(t, expected, filtered)
}

func TestFilterKnownFalsePositives_CustomLogic(t *testing.T) {
results := []Result{
{Raw: []byte("a specific magic string")}, // specific target
{Raw: []byte("00000")}, // "default" false positive list
{Raw: []byte("number")}, // from wordlist
{Raw: []byte("hga8adshla3434g")}, // real secret
}
expected := []Result{
{Raw: []byte("00000")},
{Raw: []byte("number")},
{Raw: []byte("hga8adshla3434g")},
}
filtered := FilterKnownFalsePositives(logContext.Background(), customFalsePositiveChecker{}, results, false)
assert.ElementsMatch(t, expected, filtered)
}

func TestIsFalsePositive(t *testing.T) {
type args struct {
match string
Expand Down
9 changes: 5 additions & 4 deletions pkg/detectors/ftp/ftp.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type Scanner struct {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
keyPat = regexp.MustCompile(`\bftp://[\S]{3,50}:([\S]{3,50})@[-.%\w\/:]+\b`)
Expand Down Expand Up @@ -96,16 +97,16 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
}
}

if detectors.IsKnownFalsePositive(string(s1.Raw), []detectors.FalsePositive{"@ftp.freebsd.org"}, false) {
continue
}

results = append(results, s1)
}

return results, nil
}

func (s Scanner) IsFalsePositive(result detectors.Result) bool {
return detectors.IsKnownFalsePositive(string(result.Raw), []detectors.FalsePositive{"@ftp.freebsd.org"}, false)
}

func isErrDeterminate(e error) bool {
ftpErr := &textproto.Error{}
return errors.As(e, &ftpErr) && ftpErr.Code == ftpNotLoggedIn
Expand Down
5 changes: 5 additions & 0 deletions pkg/detectors/gcp/gcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type Scanner struct{}

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
keyPat = regexp.MustCompile(`\{[^{]+auth_provider_x509_cert_url[^}]+\}`)
Expand Down Expand Up @@ -120,6 +121,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
return
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}

func (s Scanner) Type() detectorspb.DetectorType {
return detectorspb.DetectorType_GCP
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
s1.SetVerificationError(verificationErr, match)
}

// This function will check false positives for common test words, but also it will make sure the key appears 'random' enough to be a real key.
if !s1.Verified && detectors.IsKnownFalsePositive(string(s1.Raw), detectors.DefaultFalsePositives, true) {
continue
}

results = append(results, s1)
}

Expand Down
4 changes: 0 additions & 4 deletions pkg/detectors/generic/generic.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
// Least expensive-> most expensive filters.
// Substrings, then patterns.

if detectors.IsKnownFalsePositive(token, detectors.DefaultFalsePositives, true) {
continue
}

// toss any that match regexes
if hasReMatch(s.excludeMatchers, token) {
continue
Expand Down
2 changes: 2 additions & 0 deletions pkg/detectors/github/v1/github_old.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result

token := match[1]

// Note that this false positive check happens **before** verification! I don't know why it's written this way
// but that's why this logic wasn't moved into a CustomFalsePositiveChecker implementation.
specificFPs := []detectors.FalsePositive{"github commit"}
if detectors.IsKnownFalsePositive(token, specificFPs, false) {
continue
Expand Down
3 changes: 0 additions & 3 deletions pkg/detectors/github/v2/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
}
}

if !s1.Verified && detectors.IsKnownFalsePositive(string(s1.Raw), detectors.DefaultFalsePositives, true) {
continue
}
results = append(results, s1)
}

Expand Down
7 changes: 5 additions & 2 deletions pkg/detectors/jdbc/jdbc.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func WithIgnorePattern(ignoreStrings []string) func(*Scanner) {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
keyPat = regexp.MustCompile(`(?i)jdbc:[\w]{3,10}:[^\s"']{0,512}`)
Expand Down Expand Up @@ -98,14 +99,16 @@ matchLoop:
// TODO: specialized redaction
}



results = append(results, s)
}

return
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}

func tryRedactAnonymousJDBC(conn string) string {
if s, ok := tryRedactURLParams(conn); ok {
return s
Expand Down
5 changes: 0 additions & 5 deletions pkg/detectors/jupiterone/jupiterone.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
}
}

// This function will check false positives for common test words, but also it will make sure the key appears 'random' enough to be a real key.
if !s1.Verified && detectors.IsKnownFalsePositive(resMatch, detectors.DefaultFalsePositives, true) {
continue
}

results = append(results, s1)
}

Expand Down
5 changes: 5 additions & 0 deletions pkg/detectors/mongodb/mongodb.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type Scanner struct {

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

var (
defaultTimeout = 2 * time.Second
Expand Down Expand Up @@ -72,6 +73,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
return results, nil
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}

func isErrDeterminate(err error) bool {
switch e := err.(type) {
case topology.ConnectionError:
Expand Down
5 changes: 0 additions & 5 deletions pkg/detectors/onfleet/onfleet.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
s1.SetVerificationError(verificationErr, match)
}

// This function will check false positives for common test words, but also it will make sure the key appears 'random' enough to be a real key.
if !s1.Verified && detectors.IsKnownFalsePositive(match, detectors.DefaultFalsePositives, true) {
continue
}

results = append(results, s1)
}

Expand Down
5 changes: 0 additions & 5 deletions pkg/detectors/pagarme/pagarme.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
s1.SetVerificationError(verificationErr, match)
}

// This function will check false positives for common test words, but also it will make sure the key appears 'random' enough to be a real key.
if !s1.Verified && detectors.IsKnownFalsePositive(match, detectors.DefaultFalsePositives, true) {
continue
}

results = append(results, s1)
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/detectors/postgres/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ type Scanner struct {
detectLoopback bool // Automated tests run against localhost, but we want to ignore those results in the wild
}

var _ detectors.Detector = (*Scanner)(nil)
var _ detectors.CustomFalsePositiveChecker = (*Scanner)(nil)

func (s Scanner) Keywords() []string {
return []string{"postgres"}
}
Expand Down Expand Up @@ -144,6 +147,10 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) ([]dete
return results, nil
}

func (s Scanner) IsFalsePositive(_ detectors.Result) bool {
return false
}

func findUriMatches(data []byte) []map[string]string {
var matches []map[string]string
for _, uri := range uriPattern.FindAll(data, -1) {
Expand Down
Loading
Loading