Skip to content

Commit bd0c762

Browse files
author
Alvaro Muñoz
committed
Refactor: Do not use PRHeadCheckoutStep on any dependency of TaintTracking
Problem is that there are StoreSteps that depend on PRHeadCheckout so there is a non-monotic recursion error since PRHeadCheckout depends on TaintTracking module, but this module depends on PRHeadCheckout
1 parent 42b487b commit bd0c762

File tree

2 files changed

+29
-17
lines changed

2 files changed

+29
-17
lines changed

ql/lib/codeql/actions/dataflow/FlowSteps.qll

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,20 @@ predicate envToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::
155155
)
156156
}
157157

158+
predicate controlledCWD(Step artifact) {
159+
artifact instanceof UntrustedArtifactDownloadStep or
160+
// This shoould be:
161+
// artifact instanceof PRHeadCheckoutStep
162+
// but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
163+
// so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
164+
// instead of using ActionsMutableRefCheckout and ActionsSHACheckout
165+
artifact.(Uses).getCallee() = "actions/checkout" or
166+
artifact instanceof GitMutableRefCheckout or
167+
artifact instanceof GitSHACheckout or
168+
artifact instanceof GhMutableRefCheckout or
169+
artifact instanceof GhSHACheckout
170+
}
171+
158172
/**
159173
* A downloaded artifact that gets assigned to a Run step output.
160174
* - uses: actions/download-artifact@v2
@@ -165,10 +179,7 @@ predicate envToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::
165179
*/
166180
predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
167181
exists(Run run, Step artifact, string content, string key, string value |
168-
(
169-
artifact instanceof UntrustedArtifactDownloadStep or
170-
artifact instanceof PRHeadCheckoutStep
171-
) and
182+
controlledCWD(artifact) and
172183
(
173184
// A file is read and its content is assigned to an env var
174185
// - run: |
@@ -207,10 +218,7 @@ predicate artifactToOutputStoreStep(DataFlow::Node pred, DataFlow::Node succ, Da
207218
*/
208219
predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::ContentSet c) {
209220
exists(Run run, string content, string key, string value, Step artifact |
210-
(
211-
artifact instanceof UntrustedArtifactDownloadStep or
212-
artifact instanceof PRHeadCheckoutStep
213-
) and
221+
controlledCWD(artifact) and
214222
(
215223
// A file is read and its content is assigned to an env var
216224
// - run: |
@@ -246,25 +254,20 @@ predicate artifactToEnvStoreStep(DataFlow::Node pred, DataFlow::Node succ, DataF
246254
*/
247255
predicate artifactDownloadToRunStep(DataFlow::Node pred, DataFlow::Node succ) {
248256
exists(Step artifact, Run run |
249-
(
250-
artifact instanceof UntrustedArtifactDownloadStep or
251-
artifact instanceof PRHeadCheckoutStep
252-
) and
257+
controlledCWD(artifact) and
253258
pred.asExpr() = artifact and
254259
succ.asExpr() = run.getScriptScalar() and
255260
artifact.getAFollowingStep() = run
256261
)
257262
}
258263

264+
//
259265
/**
260266
* A download artifact step followed by a envvar-injection uses step .
261267
*/
262268
predicate artifactDownloadToUsesStep(DataFlow::Node pred, DataFlow::Node succ) {
263269
exists(Step artifact, Uses uses |
264-
(
265-
artifact instanceof UntrustedArtifactDownloadStep or
266-
artifact instanceof PRHeadCheckoutStep
267-
) and
270+
controlledCWD(artifact) and
268271
madSink(succ, "envvar-injection") and
269272
pred.asExpr() = artifact and
270273
succ.asExpr() = uses and

ql/lib/codeql/actions/security/OutputClobberingQuery.qll

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,16 @@ class OutputClobberingFromFileReadSink extends OutputClobberingSink {
2222
exists(Run run, Step step |
2323
(
2424
step instanceof UntrustedArtifactDownloadStep or
25-
step instanceof PRHeadCheckoutStep
25+
// This shoould be:
26+
// artifact instanceof PRHeadCheckoutStep
27+
// but PRHeadCheckoutStep uses Taint Tracking anc causes a non-Monolitic Recursion error
28+
// so we list all the subclasses of PRHeadCheckoutStep here and use actions/checkout as a workaround
29+
// instead of using ActionsMutableRefCheckout and ActionsSHACheckout
30+
step.(Uses).getCallee() = "actions/checkout" or
31+
step instanceof GitMutableRefCheckout or
32+
step instanceof GitSHACheckout or
33+
step instanceof GhMutableRefCheckout or
34+
step instanceof GhSHACheckout
2635
) and
2736
this.asExpr() = run.getScriptScalar() and
2837
step.getAFollowingStep() = run and

0 commit comments

Comments
 (0)