Skip to content

Commit ca074c0

Browse files
michaelnebelmichaelnebel
committed
Fix violations of ql/field-only-used-in-charpred.
1 parent b4d6cb6 commit ca074c0

File tree

31 files changed

+150
-183
lines changed

31 files changed

+150
-183
lines changed

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,14 @@ abstract class RemoteFlowSource extends SourceNode {
3131
class GitHubCtxSource extends RemoteFlowSource {
3232
string flag;
3333
string event;
34-
GitHubExpression e;
3534

3635
GitHubCtxSource() {
37-
this.asExpr() = e and
38-
// github.head_ref
39-
e.getFieldName() = "head_ref" and
40-
flag = "branch" and
41-
(
36+
exists(GitHubExpression e |
37+
this.asExpr() = e and
38+
// github.head_ref
39+
e.getFieldName() = "head_ref" and
40+
flag = "branch"
41+
|
4242
event = e.getATriggerEvent().getName() and
4343
event = "pull_request_target"
4444
or
@@ -148,7 +148,6 @@ class GhCLICommandSource extends RemoteFlowSource, CommandSource {
148148
class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
149149
string cmd;
150150
string flag;
151-
string access_path;
152151
Run run;
153152

154153
// Examples
@@ -163,7 +162,7 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
163162
run.getScript().getACommand() = cmd and
164163
cmd.matches("jq%") and
165164
cmd.matches("%GITHUB_EVENT_PATH%") and
166-
exists(string regexp |
165+
exists(string regexp, string access_path |
167166
untrustedEventPropertiesDataModel(regexp, flag) and
168167
not flag = "json" and
169168
access_path = "github.event" + cmd.regexpCapture(".*\\s+([^\\s]+)\\s+.*", 1) and

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ abstract class ArgumentInjectionSink extends DataFlow::Node {
1919
*/
2020
class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
2121
string command;
22-
string argument;
2322

2423
ArgumentInjectionFromEnvVarSink() {
2524
exists(Run run, string var |
@@ -28,7 +27,7 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
2827
exists(run.getInScopeEnvVarExpr(var)) or
2928
var = "GITHUB_HEAD_REF"
3029
) and
31-
run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, argument)
30+
run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, _)
3231
)
3332
}
3433

@@ -44,13 +43,12 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
4443
*/
4544
class ArgumentInjectionFromCommandSink extends ArgumentInjectionSink {
4645
string command;
47-
string argument;
4846

4947
ArgumentInjectionFromCommandSink() {
5048
exists(CommandSource source, Run run |
5149
run = source.getEnclosingRun() and
5250
this.asExpr() = run.getScript() and
53-
run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, argument)
51+
run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, _)
5452
)
5553
}
5654

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

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,6 @@ class LegitLabsDownloadArtifactActionStep extends UntrustedArtifactDownloadStep,
125125
}
126126

127127
class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, UsesStep {
128-
string script;
129-
130128
ActionsGitHubScriptDownloadStep() {
131129
// eg:
132130
// - uses: actions/github-script@v6
@@ -149,12 +147,14 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
149147
// var fs = require('fs');
150148
// fs.writeFileSync('${{github.workspace}}/test-results.zip', Buffer.from(download.data));
151149
this.getCallee() = "actions/github-script" and
152-
this.getArgument("script") = script and
153-
script.matches("%listWorkflowRunArtifacts(%") and
154-
script.matches("%downloadArtifact(%") and
155-
script.matches("%writeFileSync(%") and
156-
// Filter out artifacts that were created by pull-request.
157-
not script.matches("%exclude_pull_requests: true%")
150+
exists(string script |
151+
this.getArgument("script") = script and
152+
script.matches("%listWorkflowRunArtifacts(%") and
153+
script.matches("%downloadArtifact(%") and
154+
script.matches("%writeFileSync(%") and
155+
// Filter out artifacts that were created by pull-request.
156+
not script.matches("%exclude_pull_requests: true%")
157+
)
158158
}
159159

160160
override string getPath() {
@@ -259,15 +259,15 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
259259

260260
class ArtifactPoisoningSink extends DataFlow::Node {
261261
UntrustedArtifactDownloadStep download;
262-
PoisonableStep poisonable;
263262

264263
ArtifactPoisoningSink() {
265-
download.getAFollowingStep() = poisonable and
266-
// excluding artifacts downloaded to the temporary directory
267-
not download.getPath().regexpMatch("^/tmp.*") and
268-
not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
269-
not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*") and
270-
(
264+
exists(PoisonableStep poisonable |
265+
download.getAFollowingStep() = poisonable and
266+
// excluding artifacts downloaded to the temporary directory
267+
not download.getPath().regexpMatch("^/tmp.*") and
268+
not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
269+
not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*")
270+
|
271271
poisonable.(Run).getScript() = this.asExpr() and
272272
(
273273
// Check if the poisonable step is a local script execution step

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,11 +159,8 @@ abstract class CommentVsHeadDateCheck extends ControlCheck {
159159

160160
/* Specific implementations of control checks */
161161
class LabelIfCheck extends LabelCheck instanceof If {
162-
string condition;
163-
164162
LabelIfCheck() {
165-
condition = normalizeExpr(this.getCondition()) and
166-
(
163+
exists(string condition | condition = normalizeExpr(this.getCondition()) |
167164
// eg: contains(github.event.pull_request.labels.*.name, 'safe to test')
168165
condition.regexpMatch(".*(^|[^!])contains\\(\\s*github\\.event\\.pull_request\\.labels\\b.*")
169166
or

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,8 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
5555
* echo "COMMIT_MESSAGE=${COMMIT_MESSAGE}" >> $GITHUB_ENV
5656
*/
5757
class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
58-
CommandSource inCommand;
59-
string injectedVar;
60-
string command;
61-
6258
EnvVarInjectionFromCommandSink() {
63-
exists(Run run |
59+
exists(Run run, CommandSource inCommand, string injectedVar, string command |
6460
this.asExpr() = inCommand.getEnclosingRun().getScript() and
6561
run = inCommand.getEnclosingRun() and
6662
run.getScript().getACmdReachingGitHubEnvWrite(inCommand.getCommand(), injectedVar) and
@@ -86,12 +82,8 @@ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
8682
* echo "FOO=$BODY" >> $GITHUB_ENV
8783
*/
8884
class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink {
89-
string inVar;
90-
string injectedVar;
91-
string command;
92-
9385
EnvVarInjectionFromEnvVarSink() {
94-
exists(Run run |
86+
exists(Run run, string inVar, string injectedVar, string command |
9587
run.getScript() = this.asExpr() and
9688
exists(run.getInScopeEnvVarExpr(inVar)) and
9789
run.getScript().getAnEnvReachingGitHubEnvWrite(inVar, injectedVar) and

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,18 +99,14 @@ class OutputClobberingFromEnvVarSink extends OutputClobberingSink {
9999
* echo $BODY
100100
*/
101101
class WorkflowCommandClobberingFromEnvVarSink extends OutputClobberingSink {
102-
string clobbering_var;
103-
string clobbered_value;
104-
105102
WorkflowCommandClobberingFromEnvVarSink() {
106-
exists(Run run, string workflow_cmd_stmt, string clobbering_stmt |
103+
exists(Run run, string workflow_cmd_stmt, string clobbering_stmt, string clobbering_var |
107104
run.getScript() = this.asExpr() and
108105
run.getScript().getAStmt() = clobbering_stmt and
109106
clobbering_stmt.regexpMatch("echo\\s+(-e\\s+)?(\"|')?\\$(\\{)?" + clobbering_var + ".*") and
110107
exists(run.getInScopeEnvVarExpr(clobbering_var)) and
111108
run.getScript().getAStmt() = workflow_cmd_stmt and
112-
clobbered_value =
113-
trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1))
109+
exists(trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1)))
114110
)
115111
}
116112
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
import actions
22

33
class UnversionedImmutableAction extends UsesStep {
4-
string immutable_action;
5-
64
UnversionedImmutableAction() {
7-
isImmutableAction(this, immutable_action) and
5+
isImmutableAction(this, _) and
86
not isSemVer(this.getVersion())
97
}
108
}

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExpr.qll

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,13 +307,12 @@ class SemStoreExpr extends SemUnaryExpr {
307307
}
308308

309309
class SemConditionalExpr extends SemKnownExpr {
310-
SemExpr condition;
311310
SemExpr trueResult;
312311
SemExpr falseResult;
313312

314313
SemConditionalExpr() {
315314
opcode instanceof Opcode::Conditional and
316-
Specific::conditionalExpr(this, type, condition, trueResult, falseResult)
315+
Specific::conditionalExpr(this, type, any(SemExpr condition), trueResult, falseResult)
317316
}
318317

319318
final SemExpr getBranchExpr(boolean branch) {

‎cpp/ql/src/Security/CWE/CWE-457/UninitializedVariables.qll

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,27 +31,28 @@ private predicate hasConditionalInitialization(
3131
class ConditionallyInitializedVariable extends LocalVariable {
3232
ConditionalInitializationCall call;
3333
ConditionalInitializationFunction f;
34-
VariableAccess initAccess;
3534
Evidence e;
3635

3736
ConditionallyInitializedVariable() {
3837
// Find a call that conditionally initializes this variable
39-
hasConditionalInitialization(f, call, this, initAccess, e) and
40-
// Ignore cases where the variable is assigned prior to the call
41-
not reaches(this.getAnAssignedValue(), initAccess) and
42-
// Ignore cases where the variable is assigned field-wise prior to the call.
43-
not exists(FieldAccess fa |
44-
exists(Assignment a |
45-
fa = getAFieldAccess(this) and
46-
a.getLValue() = fa
38+
exists(VariableAccess initAccess |
39+
hasConditionalInitialization(f, call, this, initAccess, e) and
40+
// Ignore cases where the variable is assigned prior to the call
41+
not reaches(this.getAnAssignedValue(), initAccess) and
42+
// Ignore cases where the variable is assigned field-wise prior to the call.
43+
not exists(FieldAccess fa |
44+
exists(Assignment a |
45+
fa = getAFieldAccess(this) and
46+
a.getLValue() = fa
47+
)
48+
|
49+
reaches(fa, initAccess)
50+
) and
51+
// Ignore cases where the variable is assigned by a prior call to an initialization function
52+
not exists(Call c |
53+
this.getAnAccess() = getAnInitializedArgument(c).(AddressOfExpr).getOperand() and
54+
reaches(c, initAccess)
4755
)
48-
|
49-
reaches(fa, initAccess)
50-
) and
51-
// Ignore cases where the variable is assigned by a prior call to an initialization function
52-
not exists(Call c |
53-
this.getAnAccess() = getAnInitializedArgument(c).(AddressOfExpr).getOperand() and
54-
reaches(c, initAccess)
5556
) and
5657
/*
5758
* Static local variables with constant initializers do not have the initializer expr as part of

‎cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.ql

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,17 @@ import cpp
1919
* Errors when using a variable declaration inside a loop.
2020
*/
2121
class DangerousWhileLoop extends WhileStmt {
22-
Expr exp;
2322
Declaration dl;
2423

2524
DangerousWhileLoop() {
2625
this = dl.getParentScope().(BlockStmt).getParent*() and
27-
exp = this.getCondition().getAChild*() and
28-
not exp instanceof PointerFieldAccess and
29-
not exp instanceof ValueFieldAccess and
30-
exp.(VariableAccess).getTarget().getName() = dl.getName() and
31-
not exp.getParent*() instanceof FunctionCall
26+
exists(Expr exp |
27+
exp = this.getCondition().getAChild*() and
28+
not exp instanceof PointerFieldAccess and
29+
not exp instanceof ValueFieldAccess and
30+
exp.(VariableAccess).getTarget().getName() = dl.getName() and
31+
not exp.getParent*() instanceof FunctionCall
32+
)
3233
}
3334

3435
Declaration getDeclaration() { result = dl }

0 commit comments

Comments
 (0)