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

🐛 Discard GitHub token in dangerous workflow check #1772

Merged
merged 4 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
49 changes: 34 additions & 15 deletions checks/dangerous_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult {
// Check file content.
var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = func(path string,
content []byte,
args ...interface{}) (bool, error) {
args ...interface{},
) (bool, error) {
if !fileparser.IsWorkflowFile(path) {
return true, nil
}
Expand Down Expand Up @@ -160,7 +161,8 @@ var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = f
}

func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
triggers := make(map[triggerName]bool)

// We need pull request trigger.
Expand Down Expand Up @@ -194,7 +196,8 @@ func validateSecretsInPullRequests(workflow *actionlint.Workflow, path string,
}

func validateUntrustedCodeCheckout(workflow *actionlint.Workflow, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
if !usesEventTrigger(workflow, triggerPullRequestTarget) {
return nil
}
Expand Down Expand Up @@ -229,7 +232,8 @@ func jobUsesEnvironment(job *actionlint.Job) bool {
}

func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool,
path string, dl checker.DetailLogger, pdata *patternCbData) error {
path string, dl checker.DetailLogger, pdata *patternCbData,
) error {
if job == nil {
return nil
}
Expand Down Expand Up @@ -271,7 +275,8 @@ func checkJobForUsedSecrets(job *actionlint.Job, triggers map[triggerName]bool,
}

func workflowUsesCodeCheckoutAndNoEnvironment(workflow *actionlint.Workflow,
triggers map[triggerName]bool) bool {
triggers map[triggerName]bool,
) bool {
if workflow == nil {
return false
}
Expand Down Expand Up @@ -318,7 +323,8 @@ func jobUsesCodeCheckout(job *actionlint.Job) (bool, string) {
}

func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
if job == nil {
return nil
}
Expand Down Expand Up @@ -359,7 +365,8 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string,
}

func validateScriptInjection(workflow *actionlint.Workflow, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
for _, job := range workflow.Jobs {
if job == nil {
continue
Expand All @@ -382,7 +389,8 @@ func validateScriptInjection(workflow *actionlint.Workflow, path string,
}

func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[triggerName]bool,
path string, dl checker.DetailLogger, pdata *patternCbData) error {
path string, dl checker.DetailLogger, pdata *patternCbData,
) error {
// We need code checkout and not environment rule protection.
if !workflowUsesCodeCheckoutAndNoEnvironment(workflow, triggers) {
return nil
Expand All @@ -392,7 +400,8 @@ func checkWorkflowSecretInEnv(workflow *actionlint.Workflow, triggers map[trigge
}

func checkSecretInEnv(env *actionlint.Env, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
if env == nil {
return nil
}
Expand All @@ -406,7 +415,8 @@ func checkSecretInEnv(env *actionlint.Env, path string,
}

func checkSecretInRun(step *actionlint.Step, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
if step == nil || step.Exec == nil {
return nil
}
Expand All @@ -421,7 +431,8 @@ func checkSecretInRun(step *actionlint.Step, path string,
}

func checkSecretInActionArgs(step *actionlint.Step, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
if step == nil || step.Exec == nil {
return nil
}
Expand All @@ -442,7 +453,8 @@ func checkSecretInActionArgs(step *actionlint.Step, path string,
}

func checkSecretInScript(script string, pos *actionlint.Pos, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
for {
s := strings.Index(script, "${{")
if s == -1 {
Expand All @@ -454,8 +466,13 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string,
return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error())
}

// Note: The default GitHub token is allowed, as it has
// only read permission for `pull_request`.
// For `pull_request_event`, we use other signals such as
// whether checkout action is used.
variable := strings.Trim(script[s:s+e+2], " ")
if strings.Contains(variable, "secrets.") {
if !strings.Contains(variable, "secrets.GITHUB_TOKEN") &&
strings.Contains(variable, "secrets.") {
line := fileparser.GetLineNumber(pos)
dl.Warn(&checker.LogMessage{
Path: path,
Expand All @@ -472,7 +489,8 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string,
}

func checkVariablesInScript(script string, pos *actionlint.Pos, path string,
dl checker.DetailLogger, pdata *patternCbData) error {
dl checker.DetailLogger, pdata *patternCbData,
) error {
for {
s := strings.Index(script, "${{")
if s == -1 {
Expand Down Expand Up @@ -548,7 +566,8 @@ func createResultForDangerousWorkflowPatterns(result patternCbData, err error) c
}

func testValidateGitHubActionDangerousWorkflow(pathfn string,
content []byte, dl checker.DetailLogger) checker.CheckResult {
content []byte, dl checker.DetailLogger,
) checker.CheckResult {
data := patternCbData{
workflowPattern: make(map[dangerousResults]bool),
}
Expand Down
22 changes: 22 additions & 0 deletions checks/dangerous_workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,28 @@ func TestGithubDangerousWorkflow(t *testing.T) {
NumberOfDebug: 0,
},
},
{
name: "default secret in pull request",
filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-pr.yml",
expected: scut.TestReturn{
Error: nil,
Score: checker.MaxResultConfidence,
NumberOfWarn: 0,
NumberOfInfo: 0,
NumberOfDebug: 0,
},
},
{
name: "default secret in pull request target",
filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-default-secret-prt.yml",
expected: scut.TestReturn{
Error: nil,
Score: checker.MinResultConfidence,
NumberOfWarn: 1,
NumberOfInfo: 0,
NumberOfDebug: 0,
},
},
{
name: "secret in top env no checkout pull request target",
filename: "./testdata/.github/workflows/github-workflow-dangerous-pattern-secret-env-no-checkout-prt.yml",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: Close issue on Jira

on:
pull_request

env:
BLA: ${{ secrets.GITHUB_TOKEN }}

jobs:
test1:
runs-on: ubuntu-latest
steps:
- uses: actions/[email protected]
with:
ref: ${{ github.event.pull_request.head.sha }}
name: Use in env toJson

- uses: some/[email protected]
with:
option: ${{ secrets.GITHUB_TOKEN }}
name: Use secret in args

- name: Use in with toJson
env:
GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }}
run: |
echo "$GITHUB_CONTEXT"
echo "${{ secrets.GITHUB_TOKEN }}"

- name: Use in with toJson
uses: some/[email protected]
env:
GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }}
run: |
echo "$GITHUB_CONTEXT"
echo "${{ secrets.GITHUB_TOKEN }}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: Close issue on Jira

on:
pull_request_target

env:
BLA: ${{ secrets.GITHUB_TOKEN }}

jobs:
test1:
runs-on: ubuntu-latest
steps:
- uses: actions/[email protected]
with:
ref: ${{ github.event.pull_request.head.sha }}
name: Use in env toJson

- uses: some/[email protected]
with:
option: ${{ secrets.GITHUB_TOKEN }}
name: Use secret in args

- name: Use in with toJson
env:
GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }}
run: |
echo "$GITHUB_CONTEXT"
echo "${{ secrets.GITHUB_TOKEN }}"

- name: Use in with toJson
uses: some/[email protected]
env:
GITHUB_CONTEXT: ${{ secrets.GITHUB_TOKEN }}
run: |
echo "$GITHUB_CONTEXT"
echo "${{ secrets.GITHUB_TOKEN }}"