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

pkg/assessor/manifest: Add sensitive variable names checks #189

Merged
merged 10 commits into from
Jul 23, 2022
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ require (
github.com/docker/go-connections v0.4.0
github.com/goodwithtech/deckoder v0.0.1
github.com/google/go-cmp v0.5.6
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
github.com/owenrumney/go-sarif/v2 v2.0.17
github.com/urfave/cli v1.22.4
go.uber.org/zap v1.17.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ github.com/google/pprof v0.0.0-20210601050228-01bbb1931b22/go.mod h1:kpwsk12EmLe
github.com/google/pprof v0.0.0-20210609004039-a478d1d731e9/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE=
github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4=
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ=
github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
Expand Down
8 changes: 7 additions & 1 deletion pkg/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package pkg

import (
"fmt"
"github.com/urfave/cli"
"os"
"time"

"github.com/urfave/cli"
)

var (
Expand Down Expand Up @@ -62,6 +63,11 @@ OPTIONS:
EnvVar: "DOCKLE_ACCEPT_KEYS",
Usage: "For CIS-DI-0010. You can add acceptable keywords. e.g) -ak GPG_KEY -ak KEYCLOAK",
},
cli.StringSliceFlag{
Name: "sensitive-word, sw",
EnvVar: "DOCKLE_REJECT_KEYS",
Usage: "For CIS-DI-0010. You can add sensitive keywords to look for. e.g) -ak api_password -sw keys",
},
cli.StringSliceFlag{
Name: "accept-file, af",
EnvVar: "DOCKLE_ACCEPT_FILES",
Expand Down
80 changes: 60 additions & 20 deletions pkg/assessor/manifest/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"os"
"regexp"
"strings"
"time"

deckodertypes "github.com/goodwithtech/deckoder/types"

"github.com/google/shlex"

"github.com/goodwithtech/dockle/pkg/log"

"github.com/goodwithtech/dockle/pkg/types"
Expand All @@ -19,9 +23,10 @@ type ManifestAssessor struct{}

var ConfigFileName = "metadata"
var (
sensitiveDirs = map[string]struct{}{"/sys": {}, "/dev": {}, "/proc": {}}
suspiciousEnvKey = []string{"PASSWD", "PASSWORD", "SECRET", "KEY", "ACCESS"}
acceptanceEnvKey = map[string]struct{}{"GPG_KEY": {}, "GPG_KEYS": {}}
sensitiveDirs = map[string]struct{}{"/sys": {}, "/dev": {}, "/proc": {}}
suspiciousEnvKey = []string{"PASS", "PASSWD", "PASSWORD", "SECRET", "KEY", "ACCESS", "TOKEN", "API"}
tomoyamachi marked this conversation as resolved.
Show resolved Hide resolved
acceptanceEnvKey = map[string]struct{}{"GPG_KEY": {}, "GPG_KEYS": {}}
suspiciousCompiler *regexp.Regexp
)

func (a ManifestAssessor) Assess(fileMap deckodertypes.FileMap) (assesses []*types.Assessment, err error) {
Expand All @@ -41,13 +46,30 @@ func (a ManifestAssessor) Assess(fileMap deckodertypes.FileMap) (assesses []*typ
return checkAssessments(d)
}

func AddSensitiveWords(words []string) {
suspiciousEnvKey = append(suspiciousEnvKey, words...)
}

func AddAcceptanceKeys(keys []string) {
for _, key := range keys {
acceptanceEnvKey[key] = struct{}{}
}
}

func setSensitivePatterns() error {
pat := fmt.Sprintf(`.*(?i)%s.*`, strings.Join(suspiciousEnvKey, "|"))
r, err := regexp.Compile(pat)
if err != nil {
return fmt.Errorf("compile suspicious key: %w", err)
}
suspiciousCompiler = r
Copy link
Collaborator

@tomoyamachi tomoyamachi Jul 16, 2022

Choose a reason for hiding this comment

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

regexp.Compile is very slow, so I changed to compiling and set it globally.
golang/go#26623 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed this function sets the regex pattern globally so this solves the problem of compiling every time the regex

Copy link
Contributor Author

@qequ qequ Jul 19, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for your review. I fixed it. (1fd632b)

return nil
}

func checkAssessments(img types.Image) (assesses []*types.Assessment, err error) {
if err := setSensitivePatterns(); err != nil {
return nil, err
}
if img.Config.User == "" || img.Config.User == "root" {
assesses = append(assesses, &types.Assessment{
Code: types.AvoidRootDefault,
Expand All @@ -56,23 +78,6 @@ func checkAssessments(img types.Image) (assesses []*types.Assessment, err error)
})
}

for _, envVar := range img.Config.Env {
e := strings.Split(envVar, "=")
envKey := e[0]
for _, suspiciousKey := range suspiciousEnvKey {
if strings.Contains(envKey, suspiciousKey) {
if _, ok := acceptanceEnvKey[envKey]; ok {
continue
}
assesses = append(assesses, &types.Assessment{
Code: types.AvoidCredential,
Filename: ConfigFileName,
Desc: fmt.Sprintf("Suspicious ENV key found : %s (You can suppress it with --accept-key)", envKey),
})
}
}
}

if img.Config.Healthcheck == nil {
assesses = append(assesses, &types.Assessment{
Code: types.AddHealthcheck,
Expand Down Expand Up @@ -132,6 +137,15 @@ func splitByCommands(line string) map[int][]string {
func assessHistory(index int, cmd types.History) []*types.Assessment {
var assesses []*types.Assessment
cmdSlices := splitByCommands(cmd.CreatedBy)

found, varName := sensitiveVars(cmd.CreatedBy)
if found {
assesses = append(assesses, &types.Assessment{
Code: types.AvoidCredential,
Filename: ConfigFileName,
Desc: fmt.Sprintf("Suspicious ENV key found : %s on %s (You can suppress it with --accept-key)", varName, cmd.CreatedBy),
})
}
if reducableApkAdd(cmdSlices) {
assesses = append(assesses, &types.Assessment{
Code: types.UseApkAddNoCache,
Expand Down Expand Up @@ -210,6 +224,32 @@ func useADDstatement(cmdSlices map[int][]string) bool {
return false
}

func sensitiveVars(cmd string) (bool, string) {
if !strings.Contains(cmd, "=") {
return false, ""
}
toklexer := shlex.NewLexer(strings.NewReader(strings.ReplaceAll(cmd, "#", "")))
for {
word, err := toklexer.Next()
if err == io.EOF {
break
}
if !strings.Contains(word, "=") {
continue
}
varName := strings.Split(word, "=")[0]

Choose a reason for hiding this comment

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

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

if _, ok := acceptanceEnvKey[varName]; ok {
continue
}

if suspiciousCompiler.MatchString(varName) {
return true, varName
}
}

return false, ""
}

func checkAptCommand(target []string, command string) bool {
if containsThreshold(target, []string{"apt-get", "apt", command}, 2) {
return true
Expand Down
21 changes: 21 additions & 0 deletions pkg/assessor/manifest/manifest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,27 @@ func TestAddStatement(t *testing.T) {
}
}

func TestSensitiveVars(t *testing.T) {
var tests = map[string]struct {
cmd string
expected bool
}{
"basic": {cmd: "/bin/sh -c #(nop) ENV PASS=ADMIN", expected: true},
qequ marked this conversation as resolved.
Show resolved Hide resolved
"mixed cases": {cmd: "/bin/sh -c #(nop) ENV PasS=ADMIN", expected: true},
"two vars": {cmd: "/bin/sh -c #(nop) ENV abc=hello password=sensibledata", expected: true},
"run command": {cmd: `/bin/sh -c SECRET_API_KEY=63AF7AA15067C05616FDDD88A3A2E8F226F0BC06 echo "data"`, expected: true},
"run false positive": {cmd: `/bin/sh -c HELLO="PASS=\"notThis\"" echo "false positive"`, expected: false},
"run command 2": {cmd: `/bin/sh -c SECRET=myLittleSecret VAR2=VALUE2 VAR3=VALUE3 echo "Do something"`, expected: true},
}
for testname, v := range tests {
actual, _ := sensitiveVars(v.cmd)
if actual != v.expected {
t.Errorf("%s want: %t, got %t", testname, v.expected, actual)
}
}

}

func TestUseDistUpgrade(t *testing.T) {
var tests = map[string]struct {
cmdSlices map[int][]string
Expand Down
4 changes: 3 additions & 1 deletion pkg/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import (
"context"
"errors"
"fmt"
"github.com/goodwithtech/dockle/pkg/assessor/manifest"
l "log"
"os"
"strings"

"github.com/goodwithtech/dockle/pkg/assessor/manifest"

"github.com/containers/image/v5/transports/alltransports"
deckodertypes "github.com/goodwithtech/deckoder/types"

Expand Down Expand Up @@ -75,6 +76,7 @@ func Run(c *cli.Context) (err error) {
return fmt.Errorf("invalid image: %w", err)
}
}
manifest.AddSensitiveWords(c.StringSlice("sensitive-word"))
manifest.AddAcceptanceKeys(c.StringSlice("accept-key"))
scanner.AddAcceptanceFiles(c.StringSlice("accept-file"))
scanner.AddAcceptanceExtensions(c.StringSlice("accept-file-extension"))
Expand Down