Skip to content

Commit 53f82d3

Browse files
author
Alvaro Muñoz
committed
Control Checks in Run/Uses steps also protect Jobs that depend on them
1 parent 269c1de commit 53f82d3

File tree

6 files changed

+571
-16
lines changed

6 files changed

+571
-16
lines changed

‎ql/lib/codeql/actions/security/ControlChecks.qll

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,15 @@ abstract class ControlCheck extends AstNode {
5959
step.getEnclosingJob().getANeededJob().getEnvironment() = this
6060
)
6161
or
62-
this.(Step).getAFollowingStep() = step
62+
(
63+
this instanceof Run or
64+
this instanceof UsesStep
65+
) and
66+
(
67+
this.(Step).getAFollowingStep() = step
68+
or
69+
step.getEnclosingJob().getANeededJob().(LocalJob).getAStep() = this.(Step)
70+
)
6371
}
6472

6573
abstract predicate protectsCategoryAndEvent(string category, string event);
@@ -188,9 +196,7 @@ class AssociationIfCheck extends AssociationCheck instanceof If {
188196
".*\\bgithub\\.event\\.comment\\.author_association\\b.*",
189197
".*\\bgithub\\.event\\.issue\\.author_association\\b.*",
190198
".*\\bgithub\\.event\\.pull_request\\.author_association\\b.*",
191-
]) and
192-
normalizeExpr(this.getCondition()).splitAt("\n").regexpMatch(".*\\bMEMBER\\b.*") and
193-
normalizeExpr(this.getCondition()).splitAt("\n").regexpMatch(".*\\bOWNER\\b.*")
199+
])
194200
}
195201
}
196202

‎ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,14 @@ where
2929
(
3030
// issue_comment: check for date comparison checks and actor/access control checks
3131
exists(Event event |
32-
event.getName() = "issue_comment" and
3332
event = checkout.getEnclosingJob().getATriggerEvent() and
33+
(
34+
event.getName() = "issue_comment"
35+
or
36+
event.getName() = "workflow_call" and
37+
checkout.getEnclosingWorkflow().(ReusableWorkflow).getACaller().getATriggerEvent().getName() =
38+
"issue_comment"
39+
) and
3440
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
3541
(
3642
check instanceof ActorCheck or

‎ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,31 @@ where
2626
inPrivilegedContext(checkout) and
2727
(
2828
// issue_comment: check for date comparison checks and actor/access control checks
29-
exists(Event e |
30-
e.getName() = "issue_comment" and
31-
checkout.getEnclosingJob().getATriggerEvent() = e and
32-
not exists(ControlCheck write_check, CommentVsHeadDateCheck data_check |
33-
(write_check instanceof ActorCheck or write_check instanceof AssociationCheck) and
34-
write_check.dominates(checkout) and
35-
data_check.dominates(checkout)
29+
exists(Event event |
30+
event = checkout.getEnclosingJob().getATriggerEvent() and
31+
(
32+
event.getName() = "issue_comment"
33+
or
34+
event.getName() = "workflow_call" and
35+
checkout.getEnclosingWorkflow().(ReusableWorkflow).getACaller().getATriggerEvent().getName() =
36+
"issue_comment"
37+
) and
38+
not exists(ControlCheck check, CommentVsHeadDateCheck date_check |
39+
(
40+
check instanceof ActorCheck or
41+
check instanceof AssociationCheck or
42+
check instanceof PermissionCheck
43+
) and
44+
check.dominates(checkout) and
45+
date_check.dominates(checkout)
3646
)
3747
)
3848
or
3949
// not issue_comment triggered workflows
4050
exists(Event event |
4151
not event.getName() = "issue_comment" and
42-
not exists(ControlCheck check |
43-
check
44-
.protects(checkout, checkout.getEnclosingJob().getATriggerEvent(), "untrusted-checkout")
45-
)
52+
event = checkout.getEnclosingJob().getATriggerEvent() and
53+
not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout"))
4654
)
4755
)
4856
select checkout, "Potential execution of untrusted code on a privileged workflow."

‎ql/test/query-tests/Security/CWE-829/.github/workflows/test14.yml

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
name: Autodeploy Model to AML
2+
3+
on:
4+
5+
issue_comment:
6+
types: [created]
7+
8+
jobs:
9+
10+
security-checks:
11+
12+
name: Carry out security checks
13+
if: >-
14+
${{
15+
github.event.issue.pull_request &&
16+
(contains(github.event.comment.body, '/deploy') || contains(github.event.comment.body, '/rollback')) &&
17+
contains(github.event.issue.labels.*.name, 'Deployment Update') &&
18+
github.event.comment.user.type != 'Bot' &&
19+
github.event.pull_request.author_association != 'FIRST_TIMER' &&
20+
github.event.pull_request.author_association != 'FIRST_TIME_CONTRIBUTOR' &&
21+
github.event.pull_request.author_association != 'MANNEQUIN' &&
22+
github.event.pull_request.author_association != 'NONE'
23+
}}
24+
25+
runs-on: ubuntu-latest
26+
27+
defaults:
28+
run:
29+
shell: bash
30+
31+
permissions:
32+
contents: write
33+
issues: write
34+
pull-requests: write
35+
36+
steps:
37+
38+
- name: Install GH CLI
39+
uses: dev-hanz-ops/install-gh-cli-action@8fff9050dae2d81b38f94500d8b74ad1d1d47410 #v0.2.0
40+
41+
- name: Install jq
42+
run: sudo apt-get update && sudo apt-get install -y jq
43+
44+
- name: Check comment keywords
45+
env:
46+
COMMENT_BODY: ${{ github.event.comment.body }}
47+
PR_COMMENT_ALLOW_LIST: ${{ secrets.PR_COMMENT_ALLOW_LIST }}
48+
run: |
49+
function list_subset { local list1="$1"; local list2="$2"; result=0; for item in $list2; do if ! [[ $list1 =~ (^|[[:space:]])"$item"($|[[:space:]]) ]]; then result=1; fi; done; return $result; }
50+
51+
if `list_subset "echo $PR_COMMENT_ALLOW_LIST" "echo $COMMENT_BODY"` ; then
52+
echo "Command keywords allowed. Proceeding!"
53+
else
54+
echo "Command keywords not allowed. Skipping!"
55+
exit 1
56+
fi
57+
58+
- name: Check for conflicting pushes
59+
id: environment
60+
shell: bash
61+
env:
62+
COMMENT_BODY: ${{ github.event.comment.body }}
63+
COMMENT_AT: ${{ github.event.comment.created_at }}
64+
GH_REPO: ${{ github.repository }}
65+
PR_NUMBER: ${{ github.event.issue.number }}
66+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
67+
run: |
68+
pr="$(gh api /repos/${GH_REPO}/pulls/${PR_NUMBER})"
69+
pushed_at="$(echo "$pr" | jq -r .pushed_at)"
70+
71+
if [[ $(date -d "$pushed_at" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then
72+
echo "Deployment not allowed because the PR was pushed to (at $pushed_at) after the triggering comment was issued (at $COMMENT_AT)"
73+
exit 1
74+
fi
75+
76+
deploy:
77+
78+
name: Update deployment
79+
needs: security-checks
80+
runs-on: [self-hosted, production]
81+
82+
permissions:
83+
contents: write
84+
issues: write
85+
pull-requests: write
86+
statuses: write
87+
88+
steps:
89+
90+
- name: Get PR branch
91+
uses: xt0rted/pull-request-comment-branch@d97294d304604fa98a2600a6e2f916a84b596dc7 # v2.0.0
92+
id: comment-branch
93+
94+
- name: Set latest commit status as pending
95+
uses: myrotvorets/set-commit-status-action@3730c0a348a2ace3c110851bed53331bc6406e9f # v2.0.1
96+
with:
97+
sha: ${{ steps.comment-branch.outputs.head_sha }}
98+
token: ${{ secrets.GITHUB_TOKEN }}
99+
status: pending
100+
101+
- name: Checkout main
102+
if: contains(github.event.comment.body, '/rollback')
103+
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4
104+
105+
- name: Checkout PR branch
106+
if: contains(github.event.comment.body, '/deploy')
107+
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4
108+
with:
109+
ref: ${{ steps.comment-branch.outputs.head_ref }}
110+
111+
- name: Get environment from comment
112+
id: environment
113+
shell: bash
114+
env:
115+
COMMENT_BODY: ${{ github.event.comment.body }}
116+
run: |
117+
target=$(echo "$COMMENT_BODY" | sed 's/.* //') && \
118+
deploy_type=$(echo "$COMMENT_BODY" | sed 's/ .*//')
119+
120+
if [[ $target == "scorer" ]]; then
121+
echo "env=async scorer" >> $GITHUB_OUTPUT
122+
else
123+
env=$(echo "$target")
124+
echo "env=$env" >> $GITHUB_OUTPUT
125+
fi
126+
127+
if [[ $deploy_type == "/deploy" ]]; then
128+
echo "depl=deployment" >> $GITHUB_OUTPUT
129+
elif [[ $deploy_type == "/rollback" ]]; then
130+
echo "depl=rollback" >> $GITHUB_OUTPUT
131+
else
132+
echo "depl=unknown deployment type" >> $GITHUB_OUTPUT
133+
fi
134+
135+
- name: Get email of actor
136+
id: email
137+
run: |
138+
email="${{ github.actor }}@github.com"
139+
echo "email=$email" >> $GITHUB_OUTPUT
140+
141+
- name: Lookup Slack ID
142+
id: slack-id
143+
env:
144+
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}
145+
run: |
146+
slack_id=$(curl -s -H "Authorization: Bearer $SLACK_BOT_TOKEN" "https://slack.com/api/users.lookupByEmail?email=${{ steps.email.outputs.email }}" | jq -r '.user.id')
147+
echo "slack-id=$slack_id" >> $GITHUB_OUTPUT
148+
149+
- name: Notify deployment start in slack
150+
id: slack-initiate
151+
uses: slackapi/slack-github-action@37ebaef184d7626c5f204ab8d3baff4262dd30f0 # v1.27.0
152+
with:
153+
channel-id: 'C05N5U3HH2M' # platform-health-ml-ops
154+
payload: |
155+
{
156+
"blocks": [
157+
{
158+
"type": "section",
159+
"text": {
160+
"type": "mrkdwn",
161+
"text": "<@${{ steps.slack-id.outputs.slack-id }}>'s ${{ steps.environment.outputs.depl }} of <${{ github.event.issue.html_url }}|${{ github.event.issue.title }} #${{ github.event.issue.number }}> to ${{ steps.environment.outputs.env }} is in progress..."
162+
}
163+
}
164+
]
165+
}
166+
env:
167+
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}
168+
169+
- name: Environment setup
170+
uses: ./.github/actions/setup-env
171+
with:
172+
azure_creds: ${{ secrets.AZURE_CREDENTIALS }}
173+
174+
- name: Deploy server
175+
if: >-
176+
${{
177+
(contains(github.event.comment.body, '/deploy to') ||
178+
contains(github.event.comment.body, '/rollback')) &&
179+
!contains(github.event.comment.body, 'scorer')
180+
}}
181+
env:
182+
BOT_TOKEN: ${{ secrets.GITHUB_TOKEN }}
183+
PR_NUMBER: ${{ github.event.issue.number }}
184+
COMMENT_BODY: ${{ github.event.comment.body }}
185+
run: poetry run python server.py --endpoint_location=remote --autodeploy=True
186+
187+
- name: Deploy scorer
188+
if: >-
189+
${{
190+
contains(github.event.comment.body, '/deploy as async scorer') ||
191+
contains(github.event.comment.body, '/rollback async scorer')
192+
}}
193+
env:
194+
BOT_TOKEN: ${{ secrets.GITHUB_TOKEN }}
195+
PR_NUMBER: ${{ github.event.issue.number }}
196+
run: poetry run python scorer.py --as_pipeline=True --schedule=True --autodeploy=True
197+
198+
- name: Set latest commit status as ${{ job.status }}
199+
uses: myrotvorets/set-commit-status-action@3730c0a348a2ace3c110851bed53331bc6406e9f # v2.0.1
200+
if: always()
201+
with:
202+
sha: ${{ steps.comment-branch.outputs.head_sha }}
203+
token: ${{ secrets.GITHUB_TOKEN }}
204+
status: ${{ job.status }}
205+
206+
- name: Report deployment outcome in slack
207+
uses: slackapi/slack-github-action@37ebaef184d7626c5f204ab8d3baff4262dd30f0 # v1.27.0
208+
if: always()
209+
with:
210+
channel-id: 'C05N5U3HH2M' # platform-health-ml-ops
211+
payload: |
212+
{
213+
"blocks": [
214+
{
215+
"type": "section",
216+
"text": {
217+
"type": "mrkdwn",
218+
"text": "<@${{ steps.slack-id.outputs.slack-id }}>'s ${{ steps.environment.outputs.depl }} of <${{ github.event.issue.html_url }}|${{ github.event.issue.title }} #${{ github.event.issue.number }}> to ${{ steps.environment.outputs.env }} is complete!\n*Status: ${{ job.status }}*"
219+
}
220+
}
221+
]
222+
}
223+
env:
224+
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}
225+
226+
- name: prune docker images
227+
run: docker system prune --all --force

0 commit comments

Comments
 (0)