From 4502dfb55787891d555682c1c5f6e3f83fa1d236 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Mon, 15 Nov 2021 19:03:54 -0800 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Reduce=20false=20positives=20in=20T?= =?UTF-8?q?oken-Permissions=20for=20contents=20permission=20(#1253)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix * tests --- checks/permissions.go | 11 ++++ checks/permissions_test.go | 58 +++++++++++++------ ...-workflow-permissions-packages-writes.yaml | 28 +++++++++ ...b-workflow-permissions-release-writes.yaml | 28 +++++++++ utests/utlib.go | 2 +- 5 files changed, 108 insertions(+), 19 deletions(-) create mode 100644 checks/testdata/github-workflow-permissions-packages-writes.yaml create mode 100644 checks/testdata/github-workflow-permissions-release-writes.yaml diff --git a/checks/permissions.go b/checks/permissions.go index deacb358..3a8d2a43 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -374,6 +374,9 @@ func createIgnoredPermissions(s, fp string, dl checker.DetailLogger) map[string] if requiresPackagesPermissions(s, fp, dl) { ignoredPermissions["packages"] = true } + if requiresContentsPermissions(s, fp, dl) { + ignoredPermissions["contents"] = true + } if isSARIFUploadWorkflow(s, fp, dl) { ignoredPermissions["security-events"] = true } @@ -463,3 +466,11 @@ func requiresPackagesPermissions(s, fp string, dl checker.DetailLogger) bool { // workflow is a packaging workflow. return isPackagingWorkflow(s, fp, dl) } + +// Note: this needs to be improved. +// Currently we don't differentiate between publishing on GitHub vs +// pubishing on registries. In terms of risk, both are similar, as +// an attacker would gain the ability to push a package. +func requiresContentsPermissions(s, fp string, dl checker.DetailLogger) bool { + return requiresPackagesPermissions(s, fp, dl) +} diff --git a/checks/permissions_test.go b/checks/permissions_test.go index de8f7390..74ad7de3 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -40,7 +40,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 3, + NumberOfDebug: 4, }, }, { @@ -51,7 +51,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore - 1, NumberOfWarn: 1, NumberOfInfo: 1, - NumberOfDebug: 3, + NumberOfDebug: 4, }, }, { @@ -62,7 +62,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 3, NumberOfInfo: 2, - NumberOfDebug: 3, + NumberOfDebug: 4, }, }, { @@ -72,7 +72,7 @@ func TestGithubTokenPermissions(t *testing.T) { Error: nil, Score: checker.MaxResultScore, NumberOfWarn: 0, - NumberOfInfo: 2, + NumberOfInfo: 3, NumberOfDebug: 3, }, }, @@ -84,7 +84,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 1, - NumberOfDebug: 3, + NumberOfDebug: 4, }, }, { @@ -95,7 +95,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -106,7 +106,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -117,7 +117,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -128,7 +128,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -139,7 +139,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 5, + NumberOfDebug: 6, }, }, { @@ -150,7 +150,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 10, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -161,7 +161,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 10, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -172,7 +172,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -183,7 +183,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore - 1, NumberOfWarn: 2, NumberOfInfo: 2, - NumberOfDebug: 5, + NumberOfDebug: 6, }, }, { @@ -194,7 +194,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MaxResultScore - 2, NumberOfWarn: 2, NumberOfInfo: 3, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -205,7 +205,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 2, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -216,7 +216,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 2, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -227,7 +227,7 @@ func TestGithubTokenPermissions(t *testing.T) { Score: checker.MinResultScore, NumberOfWarn: 1, NumberOfInfo: 1, - NumberOfDebug: 4, + NumberOfDebug: 5, }, }, { @@ -241,6 +241,28 @@ func TestGithubTokenPermissions(t *testing.T) { NumberOfDebug: 0, }, }, + { + name: "release workflow write", + filename: "./testdata/github-workflow-permissions-release-writes.yaml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 3, + NumberOfDebug: 3, + }, + }, + { + name: "package workflow write", + filename: "./testdata/github-workflow-permissions-packages-writes.yaml", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 3, + NumberOfDebug: 3, + }, + }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below diff --git a/checks/testdata/github-workflow-permissions-packages-writes.yaml b/checks/testdata/github-workflow-permissions-packages-writes.yaml new file mode 100644 index 00000000..7a797378 --- /dev/null +++ b/checks/testdata/github-workflow-permissions-packages-writes.yaml @@ -0,0 +1,28 @@ +# Copyright 2021 Security Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +name: release workflow +on: [push] +permissions: + +jobs: + Explore-GitHub-Actions: + runs-on: ubuntu-latest + permissions: + packages: write + steps: + - name: setup + uses: actions/setup-java@ + - name: publish + run: | + mvn deploy bla diff --git a/checks/testdata/github-workflow-permissions-release-writes.yaml b/checks/testdata/github-workflow-permissions-release-writes.yaml new file mode 100644 index 00000000..09304dcb --- /dev/null +++ b/checks/testdata/github-workflow-permissions-release-writes.yaml @@ -0,0 +1,28 @@ +# Copyright 2021 Security Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +name: release workflow +on: [push] +permissions: + +jobs: + Explore-GitHub-Actions: + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - name: setup + uses: actions/setup-java@ + - name: publish + run: | + mvn deploy bla diff --git a/utests/utlib.go b/utests/utlib.go index 24d96d43..3a803eb8 100644 --- a/utests/utlib.go +++ b/utests/utlib.go @@ -123,7 +123,7 @@ func ValidateTestReturn(t *testing.T, name string, expected *TestReturn, panic(err) } if !cmp.Equal(*expected, *actualTestReturn, cmp.Comparer(errCmp)) { - log.Println(cmp.Diff(*expected, *actualTestReturn)) + log.Println(name+":", cmp.Diff(*expected, *actualTestReturn)) return false } return true