From 6e8fc89034f9214461714c6401c277be95565483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 09:29:24 +0000 Subject: [PATCH 1/8] Add default branch name check --- ql/lib/codeql/actions/dataflow/ExternalFlow.qll | 10 ++++++++-- .../dataflow/internal/ExternalFlowExtensions.qll | 6 +++++- .../actions/security/CachePoisoningQuery.qll | 15 ++++++++------- ql/lib/ext/workflow-models/workflow-models.yml | 6 ++++++ ql/test/library-tests/workflowenum.ql | 6 +++--- 5 files changed, 30 insertions(+), 13 deletions(-) diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll index 5db10e7823e..f10a90ee6ee 100644 --- a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll +++ b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll @@ -3,10 +3,16 @@ private import codeql.actions.DataFlow private import actions predicate workflowDataModel( - string path, string visibility, string job, string secrets_source, string permissions, + string path, string trigger, string job, string secrets_source, string permissions, string runner ) { - Extensions::workflowDataModel(path, visibility, job, secrets_source, permissions, runner) + Extensions::workflowDataModel(path, trigger, job, secrets_source, permissions, runner) +} + +predicate repositoryDataModel( + string visibility, string default_branch_name +) { + Extensions::repositoryDataModel(visibility, default_branch_name) } /** diff --git a/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll b/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll index 529f7721e71..34f0297d799 100644 --- a/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll +++ b/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll @@ -24,6 +24,10 @@ extensible predicate sinkModel( ); extensible predicate workflowDataModel( - string path, string visibility, string job, string secrets_source, string permissions, + string path, string trigger, string job, string secrets_source, string permissions, string runner ); + +extensible predicate repositoryDataModel( + string visibility, string default_branch_name +); diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index ab0f2d0809a..df2e1db3bdd 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -10,17 +10,18 @@ string defaultBranchTriggerEvent() { ] } -string defaultBranchNames() { result = ["main", "master", "default"] } - predicate runsOnDefaultBranch(Job j) { exists(Event e | j.getATriggerEvent() = e and + exists(string default_branch_name | + repositoryDataModel(_, default_branch_name) + ) and ( e.getName() = defaultBranchTriggerEvent() and not e.getName() = "pull_request_target" or e.getName() = "push" and - e.getAPropertyValue("branches") = defaultBranchNames() + e.getAPropertyValue("branches") = default_branch_name or e.getName() = "pull_request_target" and ( @@ -30,18 +31,18 @@ predicate runsOnDefaultBranch(Job j) { // only branches-ignore filter e.hasProperty("branches-ignore") and not e.hasProperty("branches") and - not e.getAPropertyValue("branches-ignore") = defaultBranchNames() + not e.getAPropertyValue("branches-ignore") = default_branch_name or // only branches filter e.hasProperty("branches") and not e.hasProperty("branches-ignore") and - e.getAPropertyValue("branches") = defaultBranchNames() + e.getAPropertyValue("branches") = default_branch_name or // branches and branches-ignore filters e.hasProperty("branches") and e.hasProperty("branches-ignore") and - e.getAPropertyValue("branches") = defaultBranchNames() and - not e.getAPropertyValue("branches-ignore") = defaultBranchNames() + e.getAPropertyValue("branches") = default_branch_name and + not e.getAPropertyValue("branches-ignore") = default_branch_name ) ) ) diff --git a/ql/lib/ext/workflow-models/workflow-models.yml b/ql/lib/ext/workflow-models/workflow-models.yml index f9f983be693..ca4a46b25d0 100644 --- a/ql/lib/ext/workflow-models/workflow-models.yml +++ b/ql/lib/ext/workflow-models/workflow-models.yml @@ -1,4 +1,10 @@ extensions: + - addsTo: + pack: githubsecuritylab/actions-all + extensible: repositoryDataModel + data: [ + - ["public", "main"] + ] - addsTo: pack: githubsecuritylab/actions-all extensible: workflowDataModel diff --git a/ql/test/library-tests/workflowenum.ql b/ql/test/library-tests/workflowenum.ql index 692d1eb706b..b3dc9185ec4 100644 --- a/ql/test/library-tests/workflowenum.ql +++ b/ql/test/library-tests/workflowenum.ql @@ -2,7 +2,7 @@ import actions import codeql.actions.dataflow.internal.ExternalFlowExtensions as Extensions from - string path, string visibility, string job, string secrets_source, string permissions, + string path, string trigger, string job, string secrets_source, string permissions, string runner -where Extensions::workflowDataModel(path, visibility, job, secrets_source, permissions, runner) -select visibility, path, job, secrets_source, permissions, runner +where Extensions::workflowDataModel(path, trigger, job, secrets_source, permissions, runner) +select trigger, path, job, secrets_source, permissions, runner From f38af29f80f24ece46242b1c29d26ccd3d3fce55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 09:36:18 +0000 Subject: [PATCH 2/8] Fix array --- ql/lib/ext/workflow-models/workflow-models.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ql/lib/ext/workflow-models/workflow-models.yml b/ql/lib/ext/workflow-models/workflow-models.yml index ca4a46b25d0..2293080d93e 100644 --- a/ql/lib/ext/workflow-models/workflow-models.yml +++ b/ql/lib/ext/workflow-models/workflow-models.yml @@ -2,9 +2,8 @@ extensions: - addsTo: pack: githubsecuritylab/actions-all extensible: repositoryDataModel - data: [ + data: - ["public", "main"] - ] - addsTo: pack: githubsecuritylab/actions-all extensible: workflowDataModel From cae29e0abe34af10186565c751a4a3c5affb4dd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 10:03:17 +0000 Subject: [PATCH 3/8] temporary fix --- ql/lib/codeql/actions/security/CachePoisoningQuery.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index df2e1db3bdd..b60eb7da761 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -1,4 +1,5 @@ import actions +import codeql.actions.dataflow.ExternalFlow string defaultBranchTriggerEvent() { result = From a2503dd14b8bf028a6f28fb3c4bc6d9355441f7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 10:22:40 +0000 Subject: [PATCH 4/8] fix default_branch_name visibility --- .../actions/security/CachePoisoningQuery.qll | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index b60eb7da761..69590a4a0de 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -15,35 +15,35 @@ predicate runsOnDefaultBranch(Job j) { exists(Event e | j.getATriggerEvent() = e and exists(string default_branch_name | - repositoryDataModel(_, default_branch_name) - ) and - ( - e.getName() = defaultBranchTriggerEvent() and - not e.getName() = "pull_request_target" - or - e.getName() = "push" and - e.getAPropertyValue("branches") = default_branch_name - or - e.getName() = "pull_request_target" and + repositoryDataModel(_, default_branch_name) and ( - // no filtering - not e.hasProperty("branches") and not e.hasProperty("branches-ignore") + e.getName() = defaultBranchTriggerEvent() and + not e.getName() = "pull_request_target" or - // only branches-ignore filter - e.hasProperty("branches-ignore") and - not e.hasProperty("branches") and - not e.getAPropertyValue("branches-ignore") = default_branch_name - or - // only branches filter - e.hasProperty("branches") and - not e.hasProperty("branches-ignore") and + e.getName() = "push" and e.getAPropertyValue("branches") = default_branch_name or - // branches and branches-ignore filters - e.hasProperty("branches") and - e.hasProperty("branches-ignore") and - e.getAPropertyValue("branches") = default_branch_name and - not e.getAPropertyValue("branches-ignore") = default_branch_name + e.getName() = "pull_request_target" and + ( + // no filtering + not e.hasProperty("branches") and not e.hasProperty("branches-ignore") + or + // only branches-ignore filter + e.hasProperty("branches-ignore") and + not e.hasProperty("branches") and + not e.getAPropertyValue("branches-ignore") = default_branch_name + or + // only branches filter + e.hasProperty("branches") and + not e.hasProperty("branches-ignore") and + e.getAPropertyValue("branches") = default_branch_name + or + // branches and branches-ignore filters + e.hasProperty("branches") and + e.hasProperty("branches-ignore") and + e.getAPropertyValue("branches") = default_branch_name and + not e.getAPropertyValue("branches-ignore") = default_branch_name + ) ) ) ) From 1a4939a13ba37ccd1d6dac0a8bc6af63c8c28888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 16:19:58 +0200 Subject: [PATCH 5/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alvaro Muñoz --- ql/lib/ext/workflow-models/workflow-models.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ql/lib/ext/workflow-models/workflow-models.yml b/ql/lib/ext/workflow-models/workflow-models.yml index 2293080d93e..f71f2081c8f 100644 --- a/ql/lib/ext/workflow-models/workflow-models.yml +++ b/ql/lib/ext/workflow-models/workflow-models.yml @@ -2,8 +2,7 @@ extensions: - addsTo: pack: githubsecuritylab/actions-all extensible: repositoryDataModel - data: - - ["public", "main"] + data: [] - addsTo: pack: githubsecuritylab/actions-all extensible: workflowDataModel From 11edff936b2a66da31ae278b9f9efe6104c0d62f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 15:27:59 +0000 Subject: [PATCH 6/8] Fix tests --- .../actions/security/CachePoisoningQuery.qll | 63 +++++++++++-------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index 69590a4a0de..d2a5909206e 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -11,39 +11,48 @@ string defaultBranchTriggerEvent() { ] } +string defaultBranchNames() { + exists(string default_branch_name | + repositoryDataModel(_, default_branch_name) and + result = default_branch_name + ) + or + not exist(string default_branch_name | + repositoryDataModel(_, default_branch_name) and + result = ["main", "master"] + ) +} + predicate runsOnDefaultBranch(Job j) { exists(Event e | j.getATriggerEvent() = e and - exists(string default_branch_name | - repositoryDataModel(_, default_branch_name) and + ( + e.getName() = defaultBranchTriggerEvent() and + not e.getName() = "pull_request_target" + or + e.getName() = "push" and + e.getAPropertyValue("branches") = defaultBranchNames() + or + e.getName() = "pull_request_target" and ( - e.getName() = defaultBranchTriggerEvent() and - not e.getName() = "pull_request_target" + // no filtering + not e.hasProperty("branches") and not e.hasProperty("branches-ignore") or - e.getName() = "push" and - e.getAPropertyValue("branches") = default_branch_name + // only branches-ignore filter + e.hasProperty("branches-ignore") and + not e.hasProperty("branches") and + not e.getAPropertyValue("branches-ignore") = defaultBranchNames() or - e.getName() = "pull_request_target" and - ( - // no filtering - not e.hasProperty("branches") and not e.hasProperty("branches-ignore") - or - // only branches-ignore filter - e.hasProperty("branches-ignore") and - not e.hasProperty("branches") and - not e.getAPropertyValue("branches-ignore") = default_branch_name - or - // only branches filter - e.hasProperty("branches") and - not e.hasProperty("branches-ignore") and - e.getAPropertyValue("branches") = default_branch_name - or - // branches and branches-ignore filters - e.hasProperty("branches") and - e.hasProperty("branches-ignore") and - e.getAPropertyValue("branches") = default_branch_name and - not e.getAPropertyValue("branches-ignore") = default_branch_name - ) + // only branches filter + e.hasProperty("branches") and + not e.hasProperty("branches-ignore") and + e.getAPropertyValue("branches") = defaultBranchNames() + or + // branches and branches-ignore filters + e.hasProperty("branches") and + e.hasProperty("branches-ignore") and + e.getAPropertyValue("branches") = defaultBranchNames() and + not e.getAPropertyValue("branches-ignore") = defaultBranchNames() ) ) ) From 17a6d28e18db8b8c61e0ce9275570fc91e18e1f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 15:37:17 +0000 Subject: [PATCH 7/8] Fix OR --- ql/lib/codeql/actions/security/CachePoisoningQuery.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index d2a5909206e..3cb84561b54 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -18,9 +18,9 @@ string defaultBranchNames() { ) or not exist(string default_branch_name | - repositoryDataModel(_, default_branch_name) and - result = ["main", "master"] - ) + repositoryDataModel(_, default_branch_name) + ) and + result = ["main", "master"] } predicate runsOnDefaultBranch(Job j) { From 00052d1ea117af39d03fce6c71d3273421982277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Loba=C4=8Devski?= Date: Wed, 15 May 2024 15:37:57 +0000 Subject: [PATCH 8/8] exists --- ql/lib/codeql/actions/security/CachePoisoningQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index 3cb84561b54..318548859b5 100644 --- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll +++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll @@ -17,7 +17,7 @@ string defaultBranchNames() { result = default_branch_name ) or - not exist(string default_branch_name | + not exists(string default_branch_name | repositoryDataModel(_, default_branch_name) ) and result = ["main", "master"]