pkg/assessor/manifest: Add sensitive variable names checks #189
Conversation
…ory cmds Signed-off-by: Alvaro Frias Garay <[email protected]>
…ords assessment Signed-off-by: Alvaro Frias Garay <[email protected]>
Signed-off-by: Alvaro Frias Garay <[email protected]>
pkg/assessor/manifest/manifest.go
Outdated
| sensitiveWords = append(sensitiveWords, strings.ToUpper(s)) | ||
| } | ||
| pat := strings.ReplaceAll(`.*(REP).*`, "REP", strings.Join(sensitiveWords, "|")) | ||
| r, _ := regexp.Compile(pat) |
There was a problem hiding this comment.
Don't ignore regexp.Compiler error, it could failed from a malformed regex originated from user input.
| if !strings.Contains(word, "=") { | ||
| continue | ||
| } | ||
| varName := strings.Split(word, "=")[0] |
There was a problem hiding this comment.
You may catch a few false-positives with this approach, I can think of cases with command line arguments such as:
/bin/sh -c cmd --pass=$SECURE_PASSWORD
/bin/sh -c cmd --api_config=api.yaml
Use suspiciousEnvKey and remove sensitiveWords slice. Change senstiveVars function signature. Returns boolean and sensitive word string if found. Update regex to ignore case sensitivity & handle regex error. Remove useless for. Use Sprintf instead of ReplaceAll. Update assessment message; now prints the suspicious env key found. Signed-off-by: Alvaro Frias Garay <[email protected]>
Signed-off-by: Alvaro Frias Garay <[email protected]>
Signed-off-by: Alvaro Frias Garay <[email protected]>
|
@tomoyamachi Would you care to take a look at this one ? I'm probably missing a few stuff. |
| if err != nil { | ||
| return fmt.Errorf("compile suspicious key: %w", err) | ||
| } | ||
| suspiciousCompiler = r |
There was a problem hiding this comment.
regexp.Compile is very slow, so I changed to compiling and set it globally.
golang/go#26623 (comment)
There was a problem hiding this comment.
Should it be compiled before hand in GetAssessments and passed every time results, err := assessor.Assess(files) is called? or what's your idea @tomoyamachi ?
There was a problem hiding this comment.
Just noticed this function sets the regex pattern globally so this solves the problem of compiling every time the regex
There was a problem hiding this comment.
tests for manifest checking sensitive vars fail with this change;
=== RUN TestSensitiveVars
--- FAIL: TestSensitiveVars (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x90 pc=0x5093d0]
goroutine 6 [running]:
testing.tRunner.func1.2({0x5e2fc0, 0x789c30})
/usr/local/go/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
/usr/local/go/src/testing/testing.go:1392 +0x39f
panic({0x5e2fc0, 0x789c30})
/usr/local/go/src/runtime/panic.go:838 +0x207
regexp.(*Regexp).doExecute(0x0?, {0x0?, 0x0?}, {0x0?, 0xffffffffffffffff?, 0x0?}, {0xc000166db0?, 0xc0000506b0?}, 0x5e20c0?, 0x0, ...)
/usr/local/go/src/regexp/exec.go:527 +0x90
regexp.(*Regexp).doMatch(...)
/usr/local/go/src/regexp/exec.go:514
regexp.(*Regexp).MatchString(...)
/usr/local/go/src/regexp/regexp.go:531
github.com/goodwithtech/dockle/pkg/assessor/manifest.sensitiveVars({0x61f612, 0x20})
/home/alvaro/myDockle/pkg/assessor/manifest/manifest.go:245 +0x205
github.com/goodwithtech/dockle/pkg/assessor/manifest.TestSensitiveVars(0xc000121040)
/home/alvaro/myDockle/pkg/assessor/manifest/manifest_test.go:417 +0x2fa
testing.tRunner(0xc000121040, 0x62b890)
/usr/local/go/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
/usr/local/go/src/testing/testing.go:1486 +0x35f
FAIL github.com/goodwithtech/dockle/pkg/assessor/manifest 0.005s
but testing it manually works fine
There was a problem hiding this comment.
Thank you for your review. I fixed it. (1fd632b)
|
@arieltorti Thanks! |
Signed-off-by: Alvaro Frias Garay <[email protected]>
…e into add-sensitive-variable-checks
|
@qequ Thank you for your contribution! |
Add check for sensitive variable names in commands history.
Add unit tests.
Add cli flag for adding sensitive keys to look for.