Skip to content

Commit e3502e2

Browse files
redsun82redsun82
authored
Merge branch 'main' into redsun82/swift-open-redirection
2 parents 874fe2b + 51bd1ef commit e3502e2

38 files changed

+968
-26
lines changed

‎.github/workflows/ql-for-ql-tests.yml

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,73 @@ jobs:
3333
~/.cargo/registry
3434
~/.cargo/git
3535
ql/target
36-
key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/**/Cargo.lock') }}
36+
key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/rust-toolchain.toml', 'ql/**/Cargo.lock') }}
3737
- name: Build extractor
3838
run: |
3939
cd ql;
4040
codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }});
4141
env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh
42+
- name: Cache compilation cache
43+
id: query-cache
44+
uses: ./.github/actions/cache-query-compilation
45+
with:
46+
key: ql-for-ql-tests
4247
- name: Run QL tests
4348
run: |
44-
"${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries ql/ql/test
49+
"${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries --compilation-cache "${{ steps.query-cache.outputs.cache-dir }}" ql/ql/test
4550
env:
4651
CODEQL: ${{ steps.find-codeql.outputs.codeql-path }}
47-
- name: Check QL formatting
52+
53+
other-os:
54+
strategy:
55+
matrix:
56+
os: [macos-latest, windows-latest]
57+
needs: [qltest]
58+
runs-on: ${{ matrix.os }}
59+
steps:
60+
- uses: actions/checkout@v3
61+
- name: Install GNU tar
62+
if: runner.os == 'macOS'
63+
run: |
64+
brew install gnu-tar
65+
echo "/usr/local/opt/gnu-tar/libexec/gnubin" >> $GITHUB_PATH
66+
- name: Find codeql
67+
id: find-codeql
68+
uses: github/codeql-action/init@77a8d2d10c0b403a8b4aadbd223dc489ecd22683
69+
with:
70+
languages: javascript # does not matter
71+
- uses: ./.github/actions/os-version
72+
id: os_version
73+
- uses: actions/cache@v3
74+
with:
75+
path: |
76+
~/.cargo/registry
77+
~/.cargo/git
78+
ql/target
79+
key: ${{ runner.os }}-${{ steps.os_version.outputs.version }}-qltest-cargo-${{ hashFiles('ql/rust-toolchain.toml', 'ql/**/Cargo.lock') }}
80+
- name: Build extractor
81+
if: runner.os != 'Windows'
82+
run: |
83+
cd ql;
84+
codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }});
85+
env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh
86+
- name: Build extractor (Windows)
87+
if: runner.os == 'Windows'
88+
shell: pwsh
4889
run: |
49-
find ql/ql/src "(" -name "*.ql" -or -name "*.qll" ")" -print0 | xargs -0 "${CODEQL}" query format --check-only
90+
cd ql;
91+
$Env:PATH += ";$(dirname ${{ steps.find-codeql.outputs.codeql-path }})"
92+
pwsh ./scripts/create-extractor-pack.ps1
93+
- name: Run a single QL tests - Unix
94+
if: runner.os != 'Windows'
95+
run: |
96+
"${CODEQL}" test run --check-databases --search-path "${{ github.workspace }}/ql/extractor-pack" ql/ql/test/queries/style/DeadCode/DeadCode.qlref
5097
env:
5198
CODEQL: ${{ steps.find-codeql.outputs.codeql-path }}
99+
- name: Run a single QL tests - Windows
100+
if: runner.os == 'Windows'
101+
shell: pwsh
102+
run: |
103+
$Env:PATH += ";$(dirname ${{ steps.find-codeql.outputs.codeql-path }})"
104+
codeql test run --check-databases --search-path "${{ github.workspace }}/ql/extractor-pack" ql/ql/test/queries/style/DeadCode/DeadCode.qlref
105+

‎.github/workflows/ruby-build.yml

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -205,11 +205,6 @@ jobs:
205205
- name: Fetch CodeQL
206206
uses: ./.github/actions/fetch-codeql
207207

208-
- uses: actions/checkout@v3
209-
with:
210-
repository: Shopify/example-ruby-app
211-
ref: 67a0decc5eb550f3a9228eda53925c3afd40dfe9
212-
213208
- name: Download Ruby bundle
214209
uses: actions/download-artifact@v3
215210
with:
@@ -218,26 +213,15 @@ jobs:
218213
- name: Unzip Ruby bundle
219214
shell: bash
220215
run: unzip -q -d "${{ runner.temp }}/ruby-bundle" "${{ runner.temp }}/codeql-ruby-bundle.zip"
221-
- name: Prepare test files
222-
shell: bash
223-
run: |
224-
echo "import codeql.ruby.AST select count(File f)" > "test.ql"
225-
echo "| 4 |" > "test.expected"
226-
echo 'name: sample-tests
227-
version: 0.0.0
228-
dependencies:
229-
codeql/ruby-all: "*"
230-
extractor: ruby
231-
tests: .
232-
' > qlpack.yml
216+
233217
- name: Run QL test
234218
shell: bash
235219
run: |
236-
codeql test run --search-path "${{ runner.temp }}/ruby-bundle" --additional-packs "${{ runner.temp }}/ruby-bundle" .
220+
codeql test run --search-path "${{ runner.temp }}/ruby-bundle" --additional-packs "${{ runner.temp }}/ruby-bundle" ruby/ql/test/library-tests/ast/constants/
237221
- name: Create database
238222
shell: bash
239223
run: |
240-
codeql database create --search-path "${{ runner.temp }}/ruby-bundle" --language ruby --source-root . ../database
224+
codeql database create --search-path "${{ runner.temp }}/ruby-bundle" --language ruby --source-root ruby/ql/test/library-tests/ast/constants/ ../database
241225
- name: Analyze database
242226
shell: bash
243227
run: |

‎java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,6 @@
77
// ...
88

99
// GOOD: AES is a strong algorithm
10-
Cipher des = Cipher.getInstance("AES");
10+
Cipher aes = Cipher.getInstance("AES");
1111

12-
// ...
12+
// ...

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,11 @@ module UnsafeShellCommandConstruction {
166166
.asExpr()
167167
.(BooleanLiteral)
168168
.getValue() = "true"
169+
or
170+
exists(API::Node node |
171+
node.asSink() = sys.getOptionsArg() and
172+
node.getMember("shell").asSink().mayHaveBooleanValue(true)
173+
)
169174
}
170175

171176
/**

‎javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,14 @@ nodes
282282
| lib/lib.js:543:23:543:26 | name |
283283
| lib/lib.js:545:23:545:26 | name |
284284
| lib/lib.js:545:23:545:26 | name |
285+
| lib/lib.js:550:39:550:42 | name |
286+
| lib/lib.js:550:39:550:42 | name |
287+
| lib/lib.js:551:33:551:36 | args |
288+
| lib/lib.js:552:23:552:26 | args |
289+
| lib/lib.js:552:23:552:26 | args |
290+
| lib/lib.js:555:25:555:37 | ["-rf", name] |
291+
| lib/lib.js:555:33:555:36 | name |
292+
| lib/lib.js:555:33:555:36 | name |
285293
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
286294
| lib/subLib2/compiled-file.ts:3:26:3:29 | name |
287295
| lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -659,6 +667,14 @@ edges
659667
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
660668
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
661669
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name |
670+
| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name |
671+
| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name |
672+
| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name |
673+
| lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name |
674+
| lib/lib.js:551:33:551:36 | args | lib/lib.js:552:23:552:26 | args |
675+
| lib/lib.js:551:33:551:36 | args | lib/lib.js:552:23:552:26 | args |
676+
| lib/lib.js:555:25:555:37 | ["-rf", name] | lib/lib.js:551:33:551:36 | args |
677+
| lib/lib.js:555:33:555:36 | name | lib/lib.js:555:25:555:37 | ["-rf", name] |
662678
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
663679
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
664680
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -775,6 +791,8 @@ edges
775791
| lib/lib.js:537:11:537:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:537:3:537:27 | cp.exec ... + name) | shell command |
776792
| lib/lib.js:543:11:543:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:543:3:543:27 | cp.exec ... + name) | shell command |
777793
| lib/lib.js:545:11:545:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:545:3:545:27 | cp.exec ... + name) | shell command |
794+
| lib/lib.js:552:23:552:26 | args | lib/lib.js:550:39:550:42 | name | lib/lib.js:552:23:552:26 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:550:39:550:42 | name | library input | lib/lib.js:552:9:552:38 | cp.spaw ... wnOpts) | shell command |
795+
| lib/lib.js:555:33:555:36 | name | lib/lib.js:550:39:550:42 | name | lib/lib.js:555:33:555:36 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:550:39:550:42 | name | library input | lib/lib.js:552:9:552:38 | cp.spaw ... wnOpts) | shell command |
778796
| lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command |
779797
| lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command |
780798
| lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command |

‎javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,3 +545,12 @@ module.exports.sanitizer4 = function (name) {
545545
cp.exec("rm -rf " + name); // NOT OK
546546
}
547547
}
548+
549+
550+
module.exports.shellThing = function (name) {
551+
function indirectShell(cmd, args, spawnOpts) {
552+
cp.spawn(cmd, args, spawnOpts); // NOT OK
553+
}
554+
555+
indirectShell("rm", ["-rf", name], {shell: true});
556+
}

‎ql/rust-toolchain.toml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# This file specifies the Rust version used to develop and test the QL
2+
# extractor. It is set to the lowest version of Rust we want to support.
3+
4+
[toolchain]
5+
channel = "1.54"
6+
profile = "minimal"
7+
components = [ "rustfmt" ]

‎ql/tools/qltest.cmd

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ type NUL && "%CODEQL_DIST%\codeql.exe" database index-files ^
88
--include-extension=.yml ^
99
--size-limit=5m ^
1010
--language=ql ^
11+
--working-dir=. ^
1112
"%CODEQL_EXTRACTOR_QL_WIP_DATABASE%"
1213

1314
exit /b %ERRORLEVEL%

‎ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionCustomizations.qll

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* code constructed from library input vulnerabilities, as
4+
* well as extension points for adding your own.
5+
*/
6+
7+
private import ruby
8+
private import codeql.ruby.ApiGraphs
9+
private import codeql.ruby.frameworks.core.Gem::Gem as Gem
10+
private import codeql.ruby.Concepts as Concepts
11+
12+
/**
13+
* Module containing sources, sinks, and sanitizers for code constructed from library input.
14+
*/
15+
module UnsafeCodeConstruction {
16+
/** A source for code constructed from library input vulnerabilities. */
17+
abstract class Source extends DataFlow::Node { }
18+
19+
/** An input parameter to a gem seen as a source. */
20+
private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
21+
LibraryInputAsSource() {
22+
this = Gem::getALibraryInput() and
23+
not this.getName() = "code"
24+
}
25+
}
26+
27+
/** A sink for code constructed from library input vulnerabilities. */
28+
abstract class Sink extends DataFlow::Node {
29+
/**
30+
* Gets the node where the unsafe code is executed.
31+
*/
32+
abstract DataFlow::Node getCodeSink();
33+
34+
/**
35+
* Gets the type of sink.
36+
*/
37+
string getSinkType() { result = "code construction" }
38+
}
39+
40+
/** Gets a node that is eventually executed as code at `codeExec`. */
41+
DataFlow::Node getANodeExecutedAsCode(Concepts::CodeExecution codeExec) {
42+
result = getANodeExecutedAsCode(TypeTracker::TypeBackTracker::end(), codeExec)
43+
}
44+
45+
import codeql.ruby.typetracking.TypeTracker as TypeTracker
46+
47+
/** Gets a node that is eventually executed as code at `codeExec`, type-tracked with `t`. */
48+
private DataFlow::LocalSourceNode getANodeExecutedAsCode(
49+
TypeTracker::TypeBackTracker t, Concepts::CodeExecution codeExec
50+
) {
51+
t.start() and
52+
result = codeExec.getCode().getALocalSource() and
53+
codeExec.runsArbitraryCode() // methods like `Object.send` is benign here, because of the string-construction the attacker cannot control the entire method name
54+
or
55+
exists(TypeTracker::TypeBackTracker t2 |
56+
result = getANodeExecutedAsCode(t2, codeExec).backtrack(t2, t)
57+
)
58+
}
59+
60+
/**
61+
* A string constructed using a `.join(...)` call, where the resulting string ends up being executed as code.
62+
*/
63+
class ArrayJoin extends Sink {
64+
Concepts::CodeExecution s;
65+
66+
ArrayJoin() {
67+
exists(DataFlow::CallNode call |
68+
call.getMethodName() = "join" and
69+
call.getNumberOfArguments() = 1 and // any string. E.g. ";" or "\n".
70+
call = getANodeExecutedAsCode(s) and
71+
this = call.getReceiver()
72+
)
73+
}
74+
75+
override DataFlow::Node getCodeSink() { result = s }
76+
77+
override string getSinkType() { result = "array" }
78+
}
79+
80+
/**
81+
* A string constructed from a string-literal (e.g. `"foo #{sink}"`),
82+
* where the resulting string ends up being executed as a code.
83+
*/
84+
class StringInterpolationAsSink extends Sink {
85+
Concepts::CodeExecution s;
86+
87+
StringInterpolationAsSink() {
88+
exists(Ast::StringlikeLiteral lit |
89+
any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and
90+
this.asExpr().getExpr() = lit.getComponent(_)
91+
)
92+
}
93+
94+
override DataFlow::Node getCodeSink() { result = s }
95+
96+
override string getSinkType() { result = "string interpolation" }
97+
}
98+
99+
import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
100+
101+
/**
102+
* A string constructed from a printf-style call,
103+
* where the resulting string ends up being executed as a code.
104+
*/
105+
class TaintedFormatStringAsSink extends Sink {
106+
Concepts::CodeExecution s;
107+
108+
TaintedFormatStringAsSink() {
109+
exists(TaintedFormat::PrintfStyleCall call |
110+
call = getANodeExecutedAsCode(s) and
111+
this = [call.getFormatArgument(_), call.getFormatString()]
112+
)
113+
}
114+
115+
override DataFlow::Node getCodeSink() { result = s }
116+
117+
override string getSinkType() { result = "string format" }
118+
}
119+
}

‎ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about code
3+
* constructed from library input vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeCodeConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import codeql.ruby.DataFlow
10+
import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
11+
private import codeql.ruby.TaintTracking
12+
private import codeql.ruby.dataflow.BarrierGuards
13+
14+
/**
15+
* A taint-tracking configuration for detecting code constructed from library input vulnerabilities.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "UnsafeShellCommandConstruction" }
19+
20+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
21+
22+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
23+
24+
override predicate isSanitizer(DataFlow::Node node) {
25+
node instanceof StringConstCompareBarrier or
26+
node instanceof StringConstArrayInclusionCallBarrier
27+
}
28+
29+
// override to require the path doesn't have unmatched return steps
30+
override DataFlow::FlowFeature getAFeature() {
31+
result instanceof DataFlow::FeatureHasSourceCallContext
32+
}
33+
34+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
35+
// if an array element gets tainted, then we treat the entire array as tainted
36+
exists(DataFlow::CallNode call |
37+
call.getMethodName() = ["<<", "push", "append"] and
38+
call.getReceiver() = succ and
39+
pred = call.getArgument(0) and
40+
call.getNumberOfArguments() = 1
41+
)
42+
or
43+
exists(DataFlow::CallNode call |
44+
call.getMethodName() = "[]" and
45+
succ = call and
46+
pred = call.getArgument(_)
47+
)
48+
}
49+
}

0 commit comments

Comments
 (0)