Skip to content

Commit d032d19

Browse files
MathiasVPMathiasVP
authored
Merge pull request #371 from microsoft/add-cwe-494-for-ps
PS: Add a query for CWE-494
2 parents f4bc4ba + acbf8e1 commit d032d19

9 files changed

Lines changed: 415 additions & 3 deletions

File tree

‎powershell/ql/lib/semmle/code/powershell/frameworks/System.IO.model.yml‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,13 @@ extensions:
3636
pack: microsoft/powershell-all
3737
extensible: summaryModel
3838
data:
39-
- ["system.io.path!", "Method[getfullpath]", "Argument[0]", "ReturnValue", "taint"]
39+
- ["system.io.path!", "Method[getfullpath]", "Argument[0]", "ReturnValue", "taint"]
40+
- ["system.io.file!", "Method[readallbytes]", "Argument[0]", "ReturnValue", "taint"]
41+
- ["system.io.file!", "Method[readallbytesasync]", "Argument[0]", "ReturnValue", "taint"]
42+
- ["system.io.file!", "Method[appendtext]", "Argument[0]", "ReturnValue", "taint"]
43+
- ["system.io.file!", "Method[createtext]", "Argument[0]", "ReturnValue", "taint"]
44+
- ["system.io.file!", "Method[readalllines]", "Argument[0]", "ReturnValue", "taint"]
45+
- ["system.io.file!", "Method[readalllinesasync]", "Argument[0]", "ReturnValue", "taint"]
46+
- ["system.io.file!", "Method[readalltext]", "Argument[0]", "ReturnValue", "taint"]
47+
- ["system.io.file!", "Method[readalltextasync]", "Argument[0]", "ReturnValue", "taint"]
48+
- ["system.io.fileinfo", "Method[createtext]", "Argument[0]", "ReturnValue", "taint"]

‎powershell/ql/src/queries/security/cwe-494/DownloadWithoutIntegrityCheck.qhelp‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
5+
<qhelp>
6+
7+
<overview>
8+
<p>
9+
Downloading an artifact (such as an executable, installer, archive, or script) and then
10+
using it without verifying its integrity allows an attacker who can tamper with the artifact
11+
to execute arbitrary code on the machine. Even when the artifact is retrieved from a trusted
12+
source such as GitHub over HTTPS, the contents can still be replaced through a compromised
13+
account, a malicious release asset, a poisoned cache or mirror, or a man-in-the-middle attack.
14+
</p>
15+
<p>
16+
Without an integrity check, there is no guarantee that the bytes that were downloaded are the
17+
bytes that were intended, so the downloaded artifact must not be trusted.
18+
</p>
19+
</overview>
20+
21+
<recommendation>
22+
<p>
23+
Verify the integrity of every downloaded artifact before using it. Compute a cryptographic hash
24+
of the downloaded file with <code>Get-FileHash</code> and compare it against a known-good hash
25+
that is obtained through a trusted, out-of-band channel (for example, the signed release notes
26+
or a published checksum file). Only use the artifact when the computed hash matches the expected
27+
value; otherwise, discard it and fail. Where available, prefer verifying a digital signature of
28+
the artifact in addition to, or instead of, a plain hash comparison.
29+
</p>
30+
</recommendation>
31+
32+
<example>
33+
<p>
34+
In the following example, an executable is downloaded from GitHub and then run directly. Because
35+
the artifact is never verified, a tampered download is executed without detection.
36+
</p>
37+
<sample src="examples/powershell/DownloadWithoutIntegrityCheckBad.ps1" />
38+
39+
<p>
40+
In the following example, the downloaded artifact's SHA-256 hash is compared against the expected
41+
value before it is used. The artifact is removed and an error is raised if the integrity check
42+
fails, so a tampered download is never executed.
43+
</p>
44+
<sample src="examples/powershell/DownloadWithoutIntegrityCheckGood.ps1" />
45+
</example>
46+
47+
<references>
48+
<li>
49+
Common Weakness Enumeration:
50+
<a href="https://cwe.mitre.org/data/definitions/494.html">CWE-494: Download of Code Without Integrity Check</a>.
51+
</li>
52+
<li>
53+
Common Weakness Enumeration:
54+
<a href="https://cwe.mitre.org/data/definitions/829.html">CWE-829: Inclusion of Functionality from Untrusted Control Sphere</a>.
55+
</li>
56+
<li>
57+
Microsoft Learn:
58+
<a href="https://learn.microsoft.com/powershell/module/microsoft.powershell.utility/get-filehash">Get-FileHash</a>.
59+
</li>
60+
</references>
61+
62+
</qhelp>

‎powershell/ql/src/queries/security/cwe-494/DownloadWithoutIntegrityCheck.ql‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/**
2+
* @name Unvalidated Artifact Download
3+
* @description Download of artifact without integrity check.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @security-severity 7.5
7+
* @precision high
8+
* @id powershell/download-without-integrity-check
9+
* @tags security
10+
* external/cwe/cwe-494
11+
* external/cwe/cwe-829
12+
*/
13+
14+
import powershell
15+
import semmle.code.powershell.dataflow.DataFlow
16+
import semmle.code.powershell.dataflow.TaintTracking
17+
18+
/** Holds if `s` looks like a URL pointing at a trusted artifact host. */
19+
bindingset[s]
20+
predicate isTrustedArtifactHost(string s) {
21+
s.matches([
22+
"%github%",
23+
"%gitlab%",
24+
"%bitbucket%",
25+
"%sourceforge%",
26+
"%powershellgallery%",
27+
"%nuget%",
28+
"%npmjs%",
29+
"%pypi%",
30+
"%repo1.maven%",
31+
"%repo.maven.apache%",
32+
"%blob.core.windows%",
33+
"%amazonaws%",
34+
"%googleapis%",
35+
"%azure%",
36+
"%visualstudio%",
37+
"%jfrog%",
38+
"%artifactory%"
39+
])
40+
}
41+
42+
/** A data-flow node that is tainted by a string constant looking like an artifact URL. */
43+
class ArtifactUrl extends DataFlow::Node {
44+
ArtifactUrl() {
45+
exists(DataFlow::Node source, string s |
46+
TaintTracking::localTaint(source, this) and
47+
s =
48+
[
49+
source.asExpr().getValue().asString(),
50+
source.asExpr().getExpr().(ExpandableStringExpr).getUnexpandedValue()
51+
].toLowerCase() and
52+
isTrustedArtifactHost(s) and
53+
// Exclude API metadata endpoints (e.g. api.github.com/.../releases/latest),
54+
// which return JSON metadata rather than a downloadable artifact.
55+
not s.matches(["%api.github.com%", "%api.bitbucket.org%"])
56+
)
57+
}
58+
}
59+
60+
/**
61+
* A call that downloads an artifact from a trusted host.
62+
*
63+
* This covers cmdlets and their aliases (`Invoke-WebRequest`/`iwr`,
64+
* `Invoke-RestMethod`/`irm`, `Start-BitsTransfer`), native download tools
65+
* (`curl`, `wget`, `azcopy`, `aria2c`) and the .NET `WebClient`/`HttpClient`
66+
* download methods. The URL may be passed as a named argument (e.g. `-Uri`,
67+
* `-Source`), positionally, or as a method argument.
68+
*/
69+
class DownloadCall extends DataFlow::CallNode {
70+
ArtifactUrl url;
71+
72+
DownloadCall() {
73+
this.getAName() =
74+
[
75+
// cmdlets and aliases
76+
"Invoke-WebRequest", "iwr", "Invoke-RestMethod", "irm", "Start-BitsTransfer",
77+
// native command-line download tools
78+
"curl", "curl.exe", "wget", "wget.exe", "azcopy", "azcopy.exe", "aria2c", "aria2c.exe",
79+
// .NET WebClient / HttpClient download methods
80+
"DownloadFile", "DownloadFileAsync", "DownloadFileTaskAsync", "DownloadData",
81+
"DownloadDataAsync", "DownloadDataTaskAsync", "DownloadString", "DownloadStringAsync",
82+
"GetByteArrayAsync", "GetStreamAsync"
83+
] and
84+
url = this.getAnArgument()
85+
}
86+
87+
/**
88+
* Gets the argument that names the file the artifact is written to, if any.
89+
* Downloads that consume the response inline (e.g. `irm ... | iex`) have no
90+
* such argument.
91+
*/
92+
DataFlow::Node getOutFileArg() {
93+
result =
94+
this.getNamedArgument([
95+
"outfile", "destination", "outputfile", "outpath", "literalpath", "path", "o"
96+
])
97+
or
98+
// WebClient.DownloadFile(url, destinationFile): the destination is the 2nd argument.
99+
this.getAName() = ["DownloadFile", "DownloadFileAsync", "DownloadFileTaskAsync"] and
100+
result = this.getArgument(1)
101+
}
102+
}
103+
104+
/**
105+
* A call that verifies the integrity of a file, by computing/comparing a hash
106+
* or by checking a signature.
107+
*/
108+
class IntegrityCheck extends DataFlow::CallNode {
109+
IntegrityCheck() {
110+
this.getAName() =
111+
[
112+
"Get-FileHash", "gfh", // hash a file
113+
"certutil", "certutil.exe", // certutil -hashfile <file> SHA256
114+
"ComputeHash", // [SHA256]::Create().ComputeHash(...)
115+
"Get-AuthenticodeSignature", "Test-FileCatalog", // signature / catalog checks
116+
"cosign", "cosign.exe", "gpg", "gpg.exe" // external signature verification
117+
]
118+
}
119+
120+
/** Gets an argument referring to the file whose integrity is being checked. */
121+
DataFlow::Node getFile() { result = this.getAnArgument() }
122+
}
123+
124+
module Conf implements DataFlow::ConfigSig {
125+
predicate isSource(DataFlow::Node source) { source = any(DownloadCall c).getOutFileArg() }
126+
127+
predicate isSink(DataFlow::Node sink) { sink = any(IntegrityCheck c).getFile() }
128+
}
129+
130+
module Flow = TaintTracking::Global<Conf>;
131+
132+
/** Holds if the downloaded file `out` flows to an integrity check. */
133+
predicate isVerified(DataFlow::Node out) {
134+
exists(Flow::PathNode source |
135+
source.getNode() = out and
136+
source.isSource()
137+
)
138+
}
139+
140+
from DownloadCall call, DataFlow::Node out
141+
where
142+
out = call.getOutFileArg() and
143+
not isVerified(out)
144+
select call,
145+
"This downloads an artifact without verifying its integrity (e.g. a hash or signature check)."

‎powershell/ql/src/queries/security/cwe-494/examples/powershell/DownloadWithoutIntegrityCheckBad.ps1‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# BAD: An artifact is downloaded from GitHub and then executed without
2+
# verifying that its contents match a known-good hash. If the release asset is
3+
# replaced (for example, through a compromised account, a tampered mirror, or a
4+
# man-in-the-middle attack), arbitrary code runs on the machine.
5+
$uri = "https://github.com/example/project/releases/download/v1.2.3/installer.exe"
6+
$outFile = Join-Path $env:TEMP "installer.exe"
7+
8+
Invoke-WebRequest -Uri $uri -OutFile $outFile
9+
10+
Start-Process -FilePath $outFile -Wait

‎powershell/ql/src/queries/security/cwe-494/examples/powershell/DownloadWithoutIntegrityCheckGood.ps1‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# GOOD: The artifact is downloaded from GitHub and its SHA-256 hash is compared
2+
# against the expected value (published by the author through a trusted channel,
3+
# such as the signed release notes) before it is used. The artifact is only
4+
# executed once its integrity has been confirmed.
5+
$uri = "https://github.com/example/project/releases/download/v1.2.3/installer.exe"
6+
$outFile = Join-Path $env:TEMP "installer.exe"
7+
$expectedHash = "8c954cd9b6c8f8180a05f1de66ee555f0c44b4c6738120785d827521bb8d54df"
8+
9+
Invoke-WebRequest -Uri $uri -OutFile $outFile
10+
11+
$actualHash = (Get-FileHash -Path $outFile -Algorithm SHA256).Hash
12+
13+
if ($actualHash -ne $expectedHash) {
14+
Remove-Item -Path $outFile -Force
15+
throw "Integrity check failed: '$outFile' does not match the expected hash."
16+
}
17+
18+
Start-Process -FilePath $outFile -Wait

‎powershell/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ edges
33
| test.ps1:9:11:9:20 | userinput | test.ps1:10:9:10:38 | Get-Process -Name $UserInput | provenance | |
44
| test.ps1:15:11:15:20 | userinput | test.ps1:16:50:16:79 | Get-Process -Name $UserInput | provenance | |
55
| test.ps1:21:11:21:20 | userinput | test.ps1:22:41:22:70 | Get-Process -Name $UserInput | provenance | |
6-
| test.ps1:27:11:27:20 | userinput | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | provenance | Sink:MaD:106 |
6+
| test.ps1:27:11:27:20 | userinput | test.ps1:28:38:28:67 | Get-Process -Name $UserInput | provenance | Sink:MaD:115 |
77
| test.ps1:33:11:33:20 | userinput | test.ps1:34:14:34:46 | public class Foo { $UserInput } | provenance | |
88
| test.ps1:39:11:39:20 | userinput | test.ps1:40:30:40:62 | public class Foo { $UserInput } | provenance | |
99
| test.ps1:45:11:45:20 | userinput | test.ps1:47:13:47:45 | public class Foo { $UserInput } | provenance | |
@@ -12,7 +12,7 @@ edges
1212
| test.ps1:73:11:73:20 | userinput | test.ps1:75:25:75:54 | Get-Process -Name $UserInput | provenance | |
1313
| test.ps1:80:11:80:20 | userinput | test.ps1:82:16:82:45 | Get-Process -Name $UserInput | provenance | |
1414
| test.ps1:87:11:87:20 | userinput | test.ps1:89:12:89:28 | ping $UserInput | provenance | |
15-
| test.ps1:94:11:94:20 | userinput | test.ps1:98:33:98:62 | Get-Process -Name $UserInput | provenance | Sink:MaD:105 |
15+
| test.ps1:94:11:94:20 | userinput | test.ps1:98:33:98:62 | Get-Process -Name $UserInput | provenance | Sink:MaD:114 |
1616
| test.ps1:104:11:104:20 | userinput | test.ps1:108:58:108:87 | Get-Process -Name $UserInput | provenance | |
1717
| test.ps1:114:11:114:20 | userinput | test.ps1:116:34:116:43 | UserInput | provenance | |
1818
| test.ps1:121:11:121:20 | userinput | test.ps1:123:28:123:37 | UserInput | provenance | |

‎powershell/ql/test/query-tests/security/cwe-494/DownloadWithoutIntegrityCheck.expected‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
| test.ps1:22:3:22:47 | Call to invoke-webrequest | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
2+
| test.ps1:40:3:40:103 | Call to invoke-webrequest | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
3+
| test.ps1:45:3:45:89 | Call to iwr | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
4+
| test.ps1:66:3:66:95 | Call to downloadfile | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
5+
| test.ps1:71:3:71:118 | Call to downloadfile | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
6+
| test.ps1:76:3:76:116 | Call to start-bitstransfer | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |
7+
| test.ps1:81:3:81:91 | Call to curl | This downloads an artifact without verifying its integrity (e.g. a hash or signature check). |

‎powershell/ql/test/query-tests/security/cwe-494/DownloadWithoutIntegrityCheck.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: queries/security/cwe-494/DownloadWithoutIntegrityCheck.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

0 commit comments

Comments
 (0)