Skip to content

Commit 0da8f8d

Browse files
author
Alvaro Muñoz
authored
Merge pull request #36 from GitHubSecurityLab/fix_source_regexps
fix(fn): Apply json wrappers to source regexps
2 parents 27a9bc8 + d9e589c commit 0da8f8d

File tree

6 files changed

+118
-50
lines changed

6 files changed

+118
-50
lines changed

‎ql/lib/codeql/actions/Ast.qll

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,15 @@ module Utils {
99
.regexpReplaceAll("\\[\"([a-zA-Z0-9_\\*\\-]+)\"\\]", ".$1")
1010
.regexpReplaceAll("\\s*\\.\\s*", ".")
1111
}
12+
13+
bindingset[regex]
14+
string wrapRegexp(string regex) {
15+
result =
16+
[
17+
"\\b" + regex + "\\b", "fromJSON\\(\\s*" + regex + "\\s*\\)",
18+
"toJSON\\(\\s*" + regex + "\\s*\\)"
19+
]
20+
}
1221
}
1322

1423
class AstNode instanceof AstNodeImpl {

‎ql/lib/codeql/actions/ast/internal/Ast.qll

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -813,28 +813,24 @@ abstract class SimpleReferenceExpressionImpl extends ExpressionImpl {
813813
}
814814

815815
private string stepsCtxRegex() {
816-
result = wrapRegexp("steps\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
816+
result = Utils::wrapRegexp("steps\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
817817
}
818818

819819
private string needsCtxRegex() {
820-
result = wrapRegexp("needs\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
820+
result = Utils::wrapRegexp("needs\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
821821
}
822822

823823
private string jobsCtxRegex() {
824-
result = wrapRegexp("jobs\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
824+
result = Utils::wrapRegexp("jobs\\.([A-Za-z0-9_-]+)\\.outputs\\.([A-Za-z0-9_-]+)")
825825
}
826826

827-
private string envCtxRegex() { result = wrapRegexp("env\\.([A-Za-z0-9_-]+)") }
827+
private string envCtxRegex() { result = Utils::wrapRegexp("env\\.([A-Za-z0-9_-]+)") }
828828

829-
private string matrixCtxRegex() { result = wrapRegexp("matrix\\.([A-Za-z0-9_-]+)") }
829+
private string matrixCtxRegex() { result = Utils::wrapRegexp("matrix\\.([A-Za-z0-9_-]+)") }
830830

831831
private string inputsCtxRegex() {
832-
result = wrapRegexp(["inputs\\.([A-Za-z0-9_-]+)", "github\\.event\\.inputs\\.([A-Za-z0-9_-]+)"])
833-
}
834-
835-
bindingset[regex]
836-
private string wrapRegexp(string regex) {
837-
result = ["\\b" + regex + "\\b", "fromJSON\\(" + regex + "\\)", "toJSON\\(" + regex + "\\)"]
832+
result =
833+
Utils::wrapRegexp(["inputs\\.([A-Za-z0-9_-]+)", "github\\.event\\.inputs\\.([A-Za-z0-9_-]+)"])
838834
}
839835

840836
/**

‎ql/lib/codeql/actions/dataflow/FlowSources.qll

Lines changed: 35 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,8 @@ abstract class RemoteFlowSource extends SourceNode {
2525

2626
bindingset[context]
2727
private predicate isExternalUserControlledIssue(string context) {
28-
exists(string reg |
29-
reg = ["\\bgithub\\.event\\.issue\\.title\\b", "\\bgithub\\.event\\.issue\\.body\\b"]
30-
|
31-
Utils::normalizeExpr(context).regexpMatch(reg)
28+
exists(string reg | reg = ["github\\.event\\.issue\\.title", "github\\.event\\.issue\\.body"] |
29+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
3230
)
3331
}
3432

@@ -37,38 +35,38 @@ private predicate isExternalUserControlledPullRequest(string context) {
3735
exists(string reg |
3836
reg =
3937
[
40-
"\\bgithub\\.event\\.pull_request\\.title\\b", "\\bgithub\\.event\\.pull_request\\.body\\b",
41-
"\\bgithub\\.event\\.pull_request\\.head\\.label\\b",
42-
"\\bgithub\\.event\\.pull_request\\.head\\.repo\\.default_branch\\b",
43-
"\\bgithub\\.event\\.pull_request\\.head\\.repo\\.description\\b",
44-
"\\bgithub\\.event\\.pull_request\\.head\\.repo\\.homepage\\b",
45-
"\\bgithub\\.event\\.pull_request\\.head\\.ref\\b", "\\bgithub\\.head_ref\\b"
38+
"github\\.event\\.pull_request\\.title", "github\\.event\\.pull_request\\.body",
39+
"github\\.event\\.pull_request\\.head\\.label",
40+
"github\\.event\\.pull_request\\.head\\.repo\\.default_branch",
41+
"github\\.event\\.pull_request\\.head\\.repo\\.description",
42+
"github\\.event\\.pull_request\\.head\\.repo\\.homepage",
43+
"github\\.event\\.pull_request\\.head\\.ref", "github\\.head_ref"
4644
]
4745
|
48-
Utils::normalizeExpr(context).regexpMatch(reg)
46+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
4947
)
5048
}
5149

5250
bindingset[context]
5351
private predicate isExternalUserControlledReview(string context) {
54-
Utils::normalizeExpr(context).regexpMatch("\\bgithub\\.event\\.review\\.body\\b")
52+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp("github\\.event\\.review\\.body"))
5553
}
5654

5755
bindingset[context]
5856
private predicate isExternalUserControlledComment(string context) {
59-
Utils::normalizeExpr(context).regexpMatch("\\bgithub\\.event\\.comment\\.body\\b")
57+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp("github\\.event\\.comment\\.body"))
6058
}
6159

6260
bindingset[context]
6361
private predicate isExternalUserControlledGollum(string context) {
6462
exists(string reg |
6563
reg =
6664
[
67-
"\\bgithub\\.event\\.pages\\[[0-9]+\\]\\.page_name\\b",
68-
"\\bgithub\\.event\\.pages\\[[0-9]+\\]\\.title\\b"
65+
"github\\.event\\.pages\\[[0-9]+\\]\\.page_name",
66+
"github\\.event\\.pages\\[[0-9]+\\]\\.title"
6967
]
7068
|
71-
Utils::normalizeExpr(context).regexpMatch(reg)
69+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
7270
)
7371
}
7472

@@ -77,28 +75,27 @@ private predicate isExternalUserControlledCommit(string context) {
7775
exists(string reg |
7876
reg =
7977
[
80-
"\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.message\\b",
81-
"\\bgithub\\.event\\.head_commit\\.message\\b",
82-
"\\bgithub\\.event\\.head_commit\\.author\\.email\\b",
83-
"\\bgithub\\.event\\.head_commit\\.author\\.name\\b",
84-
"\\bgithub\\.event\\.head_commit\\.committer\\.email\\b",
85-
"\\bgithub\\.event\\.head_commit\\.committer\\.name\\b",
86-
"\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.author\\.email\\b",
87-
"\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.author\\.name\\b",
88-
"\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.committer\\.email\\b",
89-
"\\bgithub\\.event\\.commits\\[[0-9]+\\]\\.committer\\.name\\b",
78+
"github\\.event\\.commits\\[[0-9]+\\]\\.message", "github\\.event\\.head_commit\\.message",
79+
"github\\.event\\.head_commit\\.author\\.email",
80+
"github\\.event\\.head_commit\\.author\\.name",
81+
"github\\.event\\.head_commit\\.committer\\.email",
82+
"github\\.event\\.head_commit\\.committer\\.name",
83+
"github\\.event\\.commits\\[[0-9]+\\]\\.author\\.email",
84+
"github\\.event\\.commits\\[[0-9]+\\]\\.author\\.name",
85+
"github\\.event\\.commits\\[[0-9]+\\]\\.committer\\.email",
86+
"github\\.event\\.commits\\[[0-9]+\\]\\.committer\\.name",
9087
]
9188
|
92-
Utils::normalizeExpr(context).regexpMatch(reg)
89+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
9390
)
9491
}
9592

9693
bindingset[context]
9794
private predicate isExternalUserControlledDiscussion(string context) {
9895
exists(string reg |
99-
reg = ["\\bgithub\\.event\\.discussion\\.title\\b", "\\bgithub\\.event\\.discussion\\.body\\b"]
96+
reg = ["github\\.event\\.discussion\\.title", "github\\.event\\.discussion\\.body"]
10097
|
101-
Utils::normalizeExpr(context).regexpMatch(reg)
98+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
10299
)
103100
}
104101

@@ -107,18 +104,17 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
107104
exists(string reg |
108105
reg =
109106
[
110-
"\\bgithub\\.event\\.workflow\\.path\\b",
111-
"\\bgithub\\.event\\.workflow_run\\.head_branch\\b",
112-
"\\bgithub\\.event\\.workflow_run\\.display_title\\b",
113-
"\\bgithub\\.event\\.workflow_run\\.head_repository\\.description\\b",
114-
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.message\\b",
115-
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.author\\.email\\b",
116-
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.author\\.name\\b",
117-
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.committer\\.email\\b",
118-
"\\bgithub\\.event\\.workflow_run\\.head_commit\\.committer\\.name\\b",
107+
"github\\.event\\.workflow\\.path", "github\\.event\\.workflow_run\\.head_branch",
108+
"github\\.event\\.workflow_run\\.display_title",
109+
"github\\.event\\.workflow_run\\.head_repository\\.description",
110+
"github\\.event\\.workflow_run\\.head_commit\\.message",
111+
"github\\.event\\.workflow_run\\.head_commit\\.author\\.email",
112+
"github\\.event\\.workflow_run\\.head_commit\\.author\\.name",
113+
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.email",
114+
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.name",
119115
]
120116
|
121-
Utils::normalizeExpr(context).regexpMatch(reg)
117+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
122118
)
123119
}
124120

‎ql/test/query-tests/Security/CWE-094/.github/workflows/json_wrap.yml

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
name: Issue Comment Created
2+
3+
on:
4+
issue_comment:
5+
types:
6+
- created
7+
8+
jobs:
9+
jira:
10+
runs-on: ubuntu-latest
11+
if: ${{ github.event.comment.body == '/jira ticket' }}
12+
steps:
13+
- run: echo ${{ github.event.comment.body }}
14+
15+
- name: Login
16+
uses: atlassian/gajira-login@v3
17+
env:
18+
JIRA_BASE_URL: ${{ secrets.JIRA_BASE_URL }}
19+
JIRA_USER_EMAIL: ${{ secrets.JIRA_USER_EMAIL }}
20+
JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }}
21+
22+
- name: SearchParam
23+
run: echo 'summary ~ ${{ toJSON(github.event.issue.title)}} AND project=${{ secrets.JIRA_PROJECT }}'
24+
25+
- name: Search
26+
id: search
27+
uses: tomhjp/[email protected]
28+
with:
29+
jql: 'summary ~ ${{ toJSON(github.event.issue.title)}} AND project=${{ secrets.JIRA_PROJECT }}'
30+
31+
- name: Log
32+
run: echo "Found issue ${{ steps.search.outputs.issue }}"
33+
34+
- name: Create
35+
id: create
36+
if: steps.search.outputs.issue == ''
37+
uses: atlassian/gajira-create@v3
38+
with:
39+
project: ${{ secrets.JIRA_PROJECT }}
40+
issuetype: Task
41+
summary: '${{ github.event.repository.name }}: ${{ github.event.issue.title }}'
42+
description: |
43+
*Issue Link:* ${{ github.event.issue.html_url }}
44+
45+
${{ github.event.issue.body }}
46+
fields: '{"customfield_10006": ${{ toJSON(secrets.JIRA_EPIC_TICKET) }}, "customfield_17401":{"value":${{ toJSON( secrets.JIRA_LAYER_CAKE )}}}}'
47+
48+
- name: Add Comment
49+
if: steps.search.outputs.issue == '' && steps.create.outputs.issue != ''
50+
uses: actions/github-script@v6
51+
with:
52+
github-token: ${{secrets.GITHUB_TOKEN}}
53+
script: |
54+
github.rest.issues.createComment({
55+
issue_number: context.issue.number,
56+
owner: context.repo.owner,
57+
repo: context.repo.repo,
58+
body: '👋 Thanks, Jira [${{steps.create.outputs.issue}}] ticket created.'
59+
})

‎ql/test/query-tests/Security/CWE-094/CodeInjection.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ nodes
131131
| .github/workflows/issues.yaml:17:19:17:36 | env.job_env | semmle.label | env.job_env |
132132
| .github/workflows/issues.yaml:18:19:18:37 | env.step_env | semmle.label | env.step_env |
133133
| .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | semmle.label | github.event.issue.title |
134+
| .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | semmle.label | github.event.comment.body |
135+
| .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | semmle.label | toJSON(github.event.issue.title) |
134136
| .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
135137
| .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
136138
| .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | semmle.label | github.event.pull_request.head.label |
@@ -234,6 +236,8 @@ subpaths
234236
| .github/workflows/issues.yaml:15:19:15:39 | env.global_env | .github/workflows/issues.yaml:4:16:4:46 | github.event.issue.title | .github/workflows/issues.yaml:15:19:15:39 | env.global_env | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:15:19:15:39 | env.global_env | ${{ env.global_env }} |
235237
| .github/workflows/issues.yaml:17:19:17:36 | env.job_env | .github/workflows/issues.yaml:10:17:10:47 | github.event.issue.title | .github/workflows/issues.yaml:17:19:17:36 | env.job_env | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:17:19:17:36 | env.job_env | ${{ env.job_env }} |
236238
| .github/workflows/issues.yaml:18:19:18:37 | env.step_env | .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | .github/workflows/issues.yaml:18:19:18:37 | env.step_env | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:18:19:18:37 | env.step_env | ${{ env.step_env }} |
239+
| .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | ${{ github.event.comment.body }} |
240+
| .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | ${{ toJSON(github.event.issue.title)}} |
237241
| .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | ${{ github.event.pull_request.title }} |
238242
| .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
239243
| .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | ${{ github.event.pull_request.head.label }} |

‎ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ nodes
131131
| .github/workflows/issues.yaml:17:19:17:36 | env.job_env | semmle.label | env.job_env |
132132
| .github/workflows/issues.yaml:18:19:18:37 | env.step_env | semmle.label | env.step_env |
133133
| .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | semmle.label | github.event.issue.title |
134+
| .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | semmle.label | github.event.comment.body |
135+
| .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | semmle.label | toJSON(github.event.issue.title) |
134136
| .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
135137
| .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
136138
| .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | semmle.label | github.event.pull_request.head.label |
@@ -229,6 +231,8 @@ subpaths
229231
| .github/workflows/issues.yaml:15:19:15:39 | env.global_env | .github/workflows/issues.yaml:4:16:4:46 | github.event.issue.title | .github/workflows/issues.yaml:15:19:15:39 | env.global_env | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:15:19:15:39 | env.global_env | ${{ env.global_env }} |
230232
| .github/workflows/issues.yaml:17:19:17:36 | env.job_env | .github/workflows/issues.yaml:10:17:10:47 | github.event.issue.title | .github/workflows/issues.yaml:17:19:17:36 | env.job_env | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:17:19:17:36 | env.job_env | ${{ env.job_env }} |
231233
| .github/workflows/issues.yaml:18:19:18:37 | env.step_env | .github/workflows/issues.yaml:20:20:20:50 | github.event.issue.title | .github/workflows/issues.yaml:18:19:18:37 | env.step_env | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/issues.yaml:18:19:18:37 | env.step_env | ${{ env.step_env }} |
234+
| .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/json_wrap.yml:13:20:13:51 | github.event.comment.body | ${{ github.event.comment.body }} |
235+
| .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/json_wrap.yml:23:31:23:68 | toJSON(github.event.issue.title) | ${{ toJSON(github.event.issue.title)}} |
232236
| .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:7:19:7:56 | github.event.pull_request.title | ${{ github.event.pull_request.title }} |
233237
| .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:8:19:8:55 | github.event.pull_request.body | ${{ github.event.pull_request.body }} |
234238
| .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/pull_request_review.yml:9:19:9:61 | github.event.pull_request.head.label | ${{ github.event.pull_request.head.label }} |

0 commit comments

Comments
 (0)