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

✨ Dangerous-Workflow: Check for workflow_run trigger #3892

Closed
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
2 changes: 2 additions & 0 deletions checker/raw_result.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ const (
DangerousWorkflowScriptInjection DangerousWorkflowType = "scriptInjection"
// DangerousWorkflowUntrustedCheckout represents an untrusted checkout.
DangerousWorkflowUntrustedCheckout DangerousWorkflowType = "untrustedCheckout"
// DangerousWorkflowSecretUsage represents a usage of GITHUB_TOKEN combined with workflow_run.
DangerousWorkflowSecretUsage DangerousWorkflowType = "secretOnWorfklowRun"
)

// DangerousWorkflowData contains raw results
Expand Down
17 changes: 16 additions & 1 deletion checks/evaluation/dangerous_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
sce "github.com/ossf/scorecard/v4/errors"
"github.com/ossf/scorecard/v4/finding"
"github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowScriptInjection"
"github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowTrigger"
"github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowUntrustedCheckout"
)

Expand All @@ -28,6 +29,7 @@
) checker.CheckResult {
expectedProbes := []string{
hasDangerousWorkflowScriptInjection.Probe,
hasDangerousWorkflowTrigger.Probe,
hasDangerousWorkflowUntrustedCheckout.Probe,
}

Expand Down Expand Up @@ -58,7 +60,8 @@
}
}

if hasDWWithUntrustedCheckout(findings) || hasDWWithScriptInjection(findings) {
if hasDWWithUntrustedCheckout(findings) || hasDWWithScriptInjection(findings) ||
hasDWWithWorkflowRunTrigger(findings) {
return checker.CreateMinScoreResult(name,
"dangerous workflow patterns detected")
}
Expand Down Expand Up @@ -101,3 +104,15 @@
}
return false
}

func hasDWWithWorkflowRunTrigger(findings []finding.Finding) bool {
for i := range findings {
f := &findings[i]
if f.Probe == hasDangerousWorkflowTrigger.Probe {
if f.Outcome == finding.OutcomeNegative {
return true
}

Check warning on line 114 in checks/evaluation/dangerous_workflow.go

View check run for this annotation

Codecov / codecov/patch

checks/evaluation/dangerous_workflow.go#L113-L114

Added lines #L113 - L114 were not covered by tests
}
}
return false
}
21 changes: 21 additions & 0 deletions checks/evaluation/dangerous_workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func TestDangerousWorkflow(t *testing.T) {
LineStart: &testLineStart,
Snippet: &testSnippet,
},
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand All @@ -64,6 +67,9 @@ func TestDangerousWorkflow(t *testing.T) {
}, {
Probe: "hasDangerousWorkflowUntrustedCheckout",
Outcome: finding.OutcomeNotApplicable,
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomeNotApplicable,
},
},
result: scut.TestReturn{
Expand All @@ -79,6 +85,9 @@ func TestDangerousWorkflow(t *testing.T) {
}, {
Probe: "hasDangerousWorkflowUntrustedCheckout",
Outcome: finding.OutcomePositive,
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand All @@ -100,6 +109,9 @@ func TestDangerousWorkflow(t *testing.T) {
LineStart: &testLineStart,
Snippet: &testSnippet,
},
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand All @@ -122,6 +134,9 @@ func TestDangerousWorkflow(t *testing.T) {
}, {
Probe: "hasDangerousWorkflowUntrustedCheckout",
Outcome: finding.OutcomePositive,
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand Down Expand Up @@ -153,6 +168,9 @@ func TestDangerousWorkflow(t *testing.T) {
}, {
Probe: "hasDangerousWorkflowUntrustedCheckout",
Outcome: finding.OutcomePositive,
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand Down Expand Up @@ -238,6 +256,9 @@ func TestDangerousWorkflow(t *testing.T) {
}, {
Probe: "hasDangerousWorkflowUntrustedCheckout",
Outcome: finding.OutcomePositive,
}, {
Probe: "hasDangerousWorkflowTrigger",
Outcome: finding.OutcomePositive,
},
},
result: scut.TestReturn{
Expand Down
22 changes: 19 additions & 3 deletions checks/fileparser/github_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@
return execAction.Uses
}

// getWith returns the 'with' statement in this step or nil if this step does not have one.
func getWith(step *actionlint.Step) map[string]*actionlint.Input {
// GetWith returns the 'with' statement in this step or nil if this step does not have one.
func GetWith(step *actionlint.Step) map[string]*actionlint.Input {
if step == nil {
return nil
}
Expand All @@ -100,6 +100,22 @@
return execAction.Inputs
}

// GetWith returns the 'args' statement in this step or nil if this step does not have one.
// 'args' are what's passed to the Docker container's ENTRYPOINT when a step is a Docker image.
func GetArgs(step *actionlint.Step) *actionlint.String {
if step == nil {
return nil
}

Check warning on line 108 in checks/fileparser/github_workflow.go

View check run for this annotation

Codecov / codecov/patch

checks/fileparser/github_workflow.go#L107-L108

Added lines #L107 - L108 were not covered by tests
if !IsStepExecKind(step, actionlint.ExecKindAction) {
return nil
}
execAction, ok := step.Exec.(*actionlint.ExecAction)
if !ok || execAction == nil {
return nil
}

Check warning on line 115 in checks/fileparser/github_workflow.go

View check run for this annotation

Codecov / codecov/patch

checks/fileparser/github_workflow.go#L114-L115

Added lines #L114 - L115 were not covered by tests
return execAction.Args
}

// getRun returns the 'run' statement in this step or nil if this step does not have one.
func getRun(step *actionlint.Step) *actionlint.String {
if step == nil {
Expand Down Expand Up @@ -421,7 +437,7 @@

// Make sure 'with' matches if present.
if len(stepToMatch.With) > 0 {
with := getWith(step)
with := GetWith(step)
if with == nil {
return false
}
Expand Down
2 changes: 1 addition & 1 deletion checks/fileparser/github_workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@ func Test_getWith(t *testing.T) {
tt := tt // Re-initializing variable so it is not changed while executing the closure below
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if got := getWith(tt.args.step); !reflect.DeepEqual(got, tt.want) {
if got := GetWith(tt.args.step); !reflect.DeepEqual(got, tt.want) {
t.Errorf("getWith() = %v, want %v", got, tt.want)
}
})
Expand Down
146 changes: 146 additions & 0 deletions checks/raw/dangerous_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
triggerWorkflowRun = triggerName("workflow_run")
checkoutUntrustedPullRequestRef = "github.event.pull_request"
checkoutUntrustedWorkflowRunRef = "github.event.workflow_run"
//nolint:gosec
secretRefGithubToken = "secrets.GITHUB_TOKEN"
)

// DangerousWorkflow retrieves the raw data for the DangerousWorkflow check.
Expand Down Expand Up @@ -118,6 +120,11 @@
return false, err
}

// 3. Check for usages of GitHub Workflow token combined with workflow_run trigger
if err := validateUsesGithubTokenOnWorkflowRun(workflow, path, pdata); err != nil {
return false, err
}

Check warning on line 126 in checks/raw/dangerous_workflow.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/dangerous_workflow.go#L125-L126

Added lines #L125 - L126 were not covered by tests

// TODO: Check other dangerous patterns.
return true, nil
}
Expand Down Expand Up @@ -270,3 +277,142 @@
}
return nil
}

func validateUsesGithubTokenOnWorkflowRun(workflow *actionlint.Workflow, path string,
pdata *checker.DangerousWorkflowData,
) error {
if !usesEventTrigger(workflow, triggerWorkflowRun) {
return nil
}

for _, job := range workflow.Jobs {
if err := checkUsesGithubToken(job, path, pdata); err != nil {
return err
}

Check warning on line 291 in checks/raw/dangerous_workflow.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/dangerous_workflow.go#L290-L291

Added lines #L290 - L291 were not covered by tests
}

return nil
}

// List every GitHub workflow that's passed the GITHUB_TOKEN,
// checking all methods for passing a variable to a workflow
//
//nolint:gocognit
func checkUsesGithubToken(
job *actionlint.Job, path string,
pdata *checker.DangerousWorkflowData,
) error {
// jobs.<job_id>.env
if job.Env != nil {
for _, v := range job.Env.Vars {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}
}

// jobs.<job_id>.steps[*].env
for _, s := range job.Steps {
if s.Env != nil {
for _, v := range s.Env.Vars {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}
}

// jobs.<job_id>.steps[*].with
with := fileparser.GetWith(s)
for _, v := range with {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}

// jobs.<job_id>.steps[*].with.args
args := fileparser.GetArgs(s)
if args != nil {
if strings.Contains(args.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
args, path, job,
))
}
}
}

// jobs.<job_id>.container.env
if job.Container != nil && job.Container.Env != nil {
for _, v := range job.Container.Env.Vars {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}
}

// jobs.<job_id>.services.<service_id>.env
for _, s := range job.Services {
if s.Container.Env != nil {
for _, v := range s.Container.Env.Vars {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}
}
}

if job.WorkflowCall != nil {
// jobs.<job_id>.with
for _, v := range job.WorkflowCall.Inputs {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}

// jobs.<job_id>.secrets
for _, v := range job.WorkflowCall.Secrets {
if strings.Contains(v.Value.Value, secretRefGithubToken) {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
v.Value, path, job,
))
}
}

// jobs.<job_id>.secrets.inherit
if job.WorkflowCall.InheritSecrets {
pdata.Workflows = append(pdata.Workflows, createTokenUsageResult(
job.WorkflowCall.Uses, path, job,
))
}
}

return nil
}

func createTokenUsageResult(
snippet *actionlint.String, path string, job *actionlint.Job,
) checker.DangerousWorkflow {
// Is this the right pos? Or should it be the entire workflow? Or maybe just the step?
line := fileparser.GetLineNumber(snippet.Pos)
return checker.DangerousWorkflow{
File: checker.File{
Path: path,
Type: finding.FileTypeSource,
Offset: line,
Snippet: snippet.Value,
},
Job: createJob(job),
Type: checker.DangerousWorkflowSecretUsage,
}
}
Loading
Loading