-
Notifications
You must be signed in to change notification settings - Fork 140
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
tomoyamachi
merged 10 commits into
goodwithtech:master
from
qequ:add-sensitive-variable-checks
Jul 23, 2022
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
347469a
pkg/assessor/manifest: Add sensitive/suspicious vars checking to hist…
qequ 701f206
Add flag -sensitive-word to add keyword when searching in sensitive w…
qequ 46b10de
pkg/assessor/manifest: Add unit tests
qequ ecfe728
pkg/assessor/manifest: Apply corrections
qequ dff940d
pkg/assessor/manifest: Add unit test for mixed cases sensitive key
qequ 39d78c6
pkg/assessor/manifest: Add acceptance keys checks to sensitiveVars
qequ 8fd0f3e
remove: check img.Config.Env, precompile: regexp.Compiler
tomoyamachi 087a654
pkg/assessor/manifest: Removed API from suspiciousEnvKey slice
qequ 1fd632b
fix TestSensitiveVars
tomoyamachi 36464fd
Merge branch 'add-sensitive-variable-checks' of github.com:qequ/dockl…
tomoyamachi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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"} | ||
acceptanceEnvKey = map[string]struct{}{"GPG_KEY": {}, "GPG_KEYS": {}} | ||
suspiciousCompiler *regexp.Regexp | ||
) | ||
|
||
func (a ManifestAssessor) Assess(fileMap deckodertypes.FileMap) (assesses []*types.Assessment, err error) { | ||
|
@@ -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 compileSensitivePatterns() 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 | ||
return nil | ||
} | ||
|
||
func checkAssessments(img types.Image) (assesses []*types.Assessment, err error) { | ||
if err := compileSensitivePatterns(); err != nil { | ||
return nil, err | ||
} | ||
if img.Config.User == "" || img.Config.User == "root" { | ||
assesses = append(assesses, &types.Assessment{ | ||
Code: types.AvoidRootDefault, | ||
|
@@ -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, | ||
|
@@ -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, | ||
|
@@ -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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 timeresults, err := assessor.Assess(files)
is called? or what's your idea @tomoyamachi ?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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;
but testing it manually works fine
There was a problem hiding this comment.
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)