Skip to content

Commit 76210f5

Browse files
author
Alvaro Muñoz
authored
Merge pull request #69 from github/improve_cache_poisoning
Improve Cache Poisoning Query
2 parents 0990774 + d181798 commit 76210f5

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

46 files changed

+452
-657
lines changed

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

Lines changed: 8 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -44,70 +44,27 @@ predicate runsOnDefaultBranch(Event e) {
4444
)
4545
}
4646

47-
abstract class CacheWritingStep extends Step { }
47+
abstract class CacheWritingStep extends Step {
48+
abstract string getPath();
49+
}
4850

4951
class CacheActionUsesStep extends CacheWritingStep, UsesStep {
5052
CacheActionUsesStep() { this.getCallee() = "actions/cache" }
53+
54+
override string getPath() { result = this.(UsesStep).getArgument("path").splitAt("\n") }
5155
}
5256

5357
class CacheActionSaveUsesStep extends CacheWritingStep, UsesStep {
5458
CacheActionSaveUsesStep() { this.getCallee() = "actions/cache/save" }
55-
}
56-
57-
class SetupJavaUsesStep extends CacheWritingStep, UsesStep {
58-
SetupJavaUsesStep() {
59-
this.getCallee() = "actions/setup-java" and
60-
(
61-
exists(this.getArgument("cache")) or
62-
exists(this.getArgument("cache-dependency-path"))
63-
)
64-
}
65-
}
66-
67-
class SetupGoUsesStep extends CacheWritingStep, UsesStep {
68-
SetupGoUsesStep() {
69-
this.getCallee() = "actions/setup-go" and
70-
(
71-
not exists(this.getArgument("cache"))
72-
or
73-
this.getArgument("cache") = "true"
74-
)
75-
}
76-
}
7759

78-
class SetupNodeUsesStep extends CacheWritingStep, UsesStep {
79-
SetupNodeUsesStep() {
80-
this.getCallee() = "actions/setup-node" and
81-
(
82-
exists(this.getArgument("cache")) or
83-
exists(this.getArgument("cache-dependency-path"))
84-
)
85-
}
86-
}
87-
88-
class SetupPythonUsesStep extends CacheWritingStep, UsesStep {
89-
SetupPythonUsesStep() {
90-
this.getCallee() = "actions/setup-python" and
91-
(
92-
exists(this.getArgument("cache")) or
93-
exists(this.getArgument("cache-dependency-path"))
94-
)
95-
}
96-
}
97-
98-
class SetupDotnetUsesStep extends CacheWritingStep, UsesStep {
99-
SetupDotnetUsesStep() {
100-
this.getCallee() = "actions/setup-dotnet" and
101-
(
102-
this.getArgument("cache") = "true" or
103-
exists(this.getArgument("cache-dependency-path"))
104-
)
105-
}
60+
override string getPath() { result = this.(UsesStep).getArgument("path").splitAt("\n") }
10661
}
10762

10863
class SetupRubyUsesStep extends CacheWritingStep, UsesStep {
10964
SetupRubyUsesStep() {
11065
this.getCallee() = ["actions/setup-ruby", "ruby/setup-ruby"] and
11166
this.getArgument("bundler-cache") = "true"
11267
}
68+
69+
override string getPath() { result = "vendor/bundle" }
11370
}

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
import actions
22
import codeql.actions.DataFlow
33

4+
string getStepCWD() {
5+
// TODO: This should be the path of the git command.
6+
// Read if from the step's CWD, workspace or look for a cd command.
7+
result = "?"
8+
}
9+
410
bindingset[s]
511
predicate containsPullRequestNumber(string s) {
612
exists(
@@ -68,7 +74,9 @@ predicate containsHeadRef(string s) {
6874
}
6975

7076
/** Checkout of a Pull Request HEAD */
71-
abstract class PRHeadCheckoutStep extends Step { }
77+
abstract class PRHeadCheckoutStep extends Step {
78+
abstract string getPath();
79+
}
7280

7381
/** Checkout of a Pull Request HEAD ref */
7482
abstract class MutableRefCheckoutStep extends PRHeadCheckoutStep { }
@@ -138,6 +146,12 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
138146
)
139147
)
140148
}
149+
150+
override string getPath() {
151+
if exists(this.(UsesStep).getArgument("path"))
152+
then result = this.(UsesStep).getArgument("path")
153+
else result = "?"
154+
}
141155
}
142156

143157
/** Checkout of a Pull Request HEAD ref using actions/checkout action */
@@ -194,6 +208,12 @@ class ActionsSHACheckout extends SHACheckoutStep instanceof UsesStep {
194208
)
195209
)
196210
}
211+
212+
override string getPath() {
213+
if exists(this.(UsesStep).getArgument("path"))
214+
then result = this.(UsesStep).getArgument("path")
215+
else result = "?"
216+
}
197217
}
198218

199219
/** Checkout of a Pull Request HEAD ref using git within a Run step */
@@ -216,6 +236,8 @@ class GitMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
216236
)
217237
)
218238
}
239+
240+
override string getPath() { result = getStepCWD() }
219241
}
220242

221243
/** Checkout of a Pull Request HEAD ref using git within a Run step */
@@ -235,6 +257,8 @@ class GitSHACheckout extends SHACheckoutStep instanceof Run {
235257
)
236258
)
237259
}
260+
261+
override string getPath() { result = getStepCWD() }
238262
}
239263

240264
/** Checkout of a Pull Request HEAD ref using gh within a Run step */
@@ -256,6 +280,8 @@ class GhMutableRefCheckout extends MutableRefCheckoutStep instanceof Run {
256280
)
257281
)
258282
}
283+
284+
override string getPath() { result = getStepCWD() }
259285
}
260286

261287
/** Checkout of a Pull Request HEAD ref using gh within a Run step */
@@ -274,4 +300,6 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
274300
)
275301
)
276302
}
303+
304+
override string getPath() { result = getStepCWD() }
277305
}

‎ql/src/Security/CWE-349/CachePoisoning.ql

Lines changed: 0 additions & 61 deletions
This file was deleted.

‎ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql renamed to ‎ql/src/Security/CWE-349/CachePoisoningViaCodeInjection.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* @name Cache Poisoning via low-privilege code injection
2+
* @name Cache Poisoning via low-privileged code injection
33
* @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
44
* @kind path-problem
55
* @problem.severity error

‎ql/src/Security/CWE-349/CachePoisoningViaDirectCache.ql

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/**
2+
* @name Cache Poisoning via caching of untrusted files
3+
* @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 7.5
8+
* @id actions/cache-poisoning/direct-cache
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-349
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.ArtifactPoisoningQuery
16+
import codeql.actions.security.UntrustedCheckoutQuery
17+
import codeql.actions.security.CachePoisoningQuery
18+
import codeql.actions.security.PoisonableSteps
19+
import codeql.actions.security.ControlChecks
20+
21+
/**
22+
* Holds if the path cache_path is a subpath of the path untrusted_path.
23+
*/
24+
bindingset[cache_path, untrusted_path]
25+
predicate controlledCachePath(string cache_path, string untrusted_path) {
26+
exists(string normalized_cache_path, string normalized_untrusted_path |
27+
(
28+
cache_path.regexpMatch("^[a-zA-Z0-9_-].*") and
29+
normalized_cache_path = "./" + cache_path.regexpReplaceAll("/$", "")
30+
or
31+
normalized_cache_path = cache_path.regexpReplaceAll("/$", "")
32+
) and
33+
(
34+
untrusted_path.regexpMatch("^[a-zA-Z0-9_-].*") and
35+
normalized_untrusted_path = "./" + untrusted_path.regexpReplaceAll("/$", "")
36+
or
37+
normalized_untrusted_path = untrusted_path.regexpReplaceAll("/$", "")
38+
) and
39+
normalized_cache_path.substring(0, normalized_untrusted_path.length()) =
40+
normalized_untrusted_path
41+
)
42+
}
43+
44+
query predicate edges(Step a, Step b) { a.getNextStep() = b }
45+
46+
from LocalJob j, Event e, Step source, Step s, string message, string path
47+
where
48+
// the job checkouts untrusted code from a pull request or downloads an untrusted artifact
49+
j.getAStep() = source and
50+
(
51+
source instanceof PRHeadCheckoutStep and
52+
message = "due to privilege checkout of untrusted code." and
53+
path = source.(PRHeadCheckoutStep).getPath()
54+
or
55+
source instanceof UntrustedArtifactDownloadStep and
56+
message = "due to downloading an untrusted artifact." and
57+
path = source.(UntrustedArtifactDownloadStep).getPath()
58+
) and
59+
// the checkout/download is not controlled by an access check
60+
not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
61+
j.getATriggerEvent() = e and
62+
// job can be triggered by an external user
63+
e.isExternallyTriggerable() and
64+
(
65+
// the workflow runs in the context of the default branch
66+
runsOnDefaultBranch(e)
67+
or
68+
// the workflow's caller runs in the context of the default branch
69+
e.getName() = "workflow_call" and
70+
exists(ExternalJob caller |
71+
caller.getCallee() = j.getLocation().getFile().getRelativePath() and
72+
runsOnDefaultBranch(caller.getATriggerEvent())
73+
)
74+
) and
75+
// the job writes to the cache
76+
// (No need to follow the checkout/download step since the cache is normally write after the job completes)
77+
j.getAStep() = s and
78+
s instanceof CacheWritingStep and
79+
(
80+
// we dont know what code can be controlled by the attacker
81+
path = "?"
82+
or
83+
// we dont know what files are being cached
84+
s.(CacheWritingStep).getPath() = "?"
85+
or
86+
// the cache writing step reads from a path the attacker can control
87+
not path = "?" and controlledCachePath(s.(CacheWritingStep).getPath(), path)
88+
) and
89+
not s instanceof PoisonableStep
90+
select s, source, s, "Potential cache poisoning in the context of the default branch " + message

‎ql/src/Security/CWE-349/CachePoisoningViaPoisonableStep.ql

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* @name Cache Poisoning via execution of untrusted code
3+
* @description The cache can be poisoned by untrusted code, leading to a cache poisoning attack.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 7.5
8+
* @id actions/cache-poisoning/poisonable-step
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-349
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.ArtifactPoisoningQuery
16+
import codeql.actions.security.UntrustedCheckoutQuery
17+
import codeql.actions.security.CachePoisoningQuery
18+
import codeql.actions.security.PoisonableSteps
19+
import codeql.actions.security.ControlChecks
20+
21+
query predicate edges(Step a, Step b) { a.getNextStep() = b }
22+
23+
from LocalJob j, Event e, Step source, Step s, string message, string path
24+
where
25+
// the job checkouts untrusted code from a pull request or downloads an untrusted artifact
26+
j.getAStep() = source and
27+
(
28+
source instanceof PRHeadCheckoutStep and
29+
message = "due to privilege checkout of untrusted code." and
30+
path = source.(PRHeadCheckoutStep).getPath()
31+
or
32+
source instanceof UntrustedArtifactDownloadStep and
33+
message = "due to downloading an untrusted artifact." and
34+
path = source.(UntrustedArtifactDownloadStep).getPath()
35+
) and
36+
// the checkout/download is not controlled by an access check
37+
not exists(ControlCheck check | check.protects(source, j.getATriggerEvent())) and
38+
j.getATriggerEvent() = e and
39+
// job can be triggered by an external user
40+
e.isExternallyTriggerable() and
41+
(
42+
// the workflow runs in the context of the default branch
43+
runsOnDefaultBranch(e)
44+
or
45+
// the workflow's caller runs in the context of the default branch
46+
e.getName() = "workflow_call" and
47+
exists(ExternalJob caller |
48+
caller.getCallee() = j.getLocation().getFile().getRelativePath() and
49+
runsOnDefaultBranch(caller.getATriggerEvent())
50+
)
51+
) and
52+
// the job executes checked-out code
53+
// (The cache specific token can be leaked even for non-privileged workflows)
54+
source.getAFollowingStep() = s and
55+
s instanceof PoisonableStep and
56+
// excluding privileged workflows since they can be exploited in easier circumstances
57+
not j.isPrivileged()
58+
select s, source, s, "Potential cache poisoning in the context of the default branch " + message

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test9.yml renamed to ‎ql/test/query-tests/Security/CWE-349/.github/workflows/code_injection1.yml

File renamed without changes.

‎ql/test/query-tests/Security/CWE-349/.github/workflows/code_injection2.yml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
name: Test
2+
3+
on:
4+
pull_request_target:
5+
branches: [ master, main, dev ]
6+
7+
jobs:
8+
test:
9+
name: Test
10+
runs-on: ubuntu-latest
11+
steps:
12+
- id: modified_files
13+
uses: trilom/[email protected]
14+
with:
15+
output: ","
16+
- run: echo "${{ steps.modified_files.outputs.files_modified }}"

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test1.yml renamed to ‎ql/test/query-tests/Security/CWE-349/.github/workflows/direct_cache1.yml

File renamed without changes.

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test2.yml renamed to ‎ql/test/query-tests/Security/CWE-349/.github/workflows/direct_cache2.yml

File renamed without changes.

0 commit comments

Comments
 (0)