Skip to content

Commit 7d6dac4

Browse files
erik-krogherik-krogh
authored
Merge branch 'js-team-sprint' into https-fix
2 parents 27a20b2 + 20e9679 commit 7d6dac4

File tree

13 files changed

+303
-22
lines changed

13 files changed

+303
-22
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
| Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
3838
| Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
3939
| Download of sensitive file through insecure connection (`js/insecure-download`) | security, external/cwe/cwe-829 | Highlights downloads of sensitive files through an unencrypted protocol. Results are shown on LGTM by default. |
40+
| Storage of sensitive information in build artifact (`js/build-artifact-leak`) | security, external/cwe/cwe-312 | Highlights storage of sensitive information in build artifacts. Results are shown on LGTM by default. |
4041
| Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. |
4142

4243
## Changes to existing queries

‎javascript/ql/src/Security/CWE-312/BuildArtifactLeak.qhelp

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Sensitive information included in a build artifact can allow an attacker to access
8+
the sensitive information if the artifact is published.
9+
</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>
14+
Only store information that is meant to be publicly available in a build artifact.
15+
</p>
16+
</recommendation>
17+
18+
<example>
19+
<p>
20+
The following example creates a <code>webpack</code> configuration that inserts all environment
21+
variables from the host into the build artifact:
22+
</p>
23+
<sample src="examples/build-leak.js"/>
24+
<p>
25+
The environment variables might include API keys or other sensitive information, and the build-system
26+
should instead insert only the environment variables that are supposed to be public.
27+
</p>
28+
<p>
29+
The issue has been fixed below, where only the <code>DEBUG</code> environment variable is inserted into the artifact.
30+
</p>
31+
<sample src="examples/build-leak-fixed.js"/>
32+
</example>
33+
<references>
34+
<li>webpack: <a href="https://webpack.js.org/plugins/define-plugin/">DefinePlugin API</a>.</li>
35+
</references>
36+
</qhelp>

‎javascript/ql/src/Security/CWE-312/BuildArtifactLeak.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Storage of sensitive information in build artifact
3+
* @description Including sensitive information in a build artifact can
4+
* expose it to an attacker.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/build-artifact-leak
9+
* @tags security
10+
* external/cwe/cwe-312
11+
* external/cwe/cwe-315
12+
* external/cwe/cwe-359
13+
*/
14+
15+
import javascript
16+
import semmle.javascript.security.dataflow.BuildArtifactLeak::BuildArtifactLeak
17+
import DataFlow::PathGraph
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where cfg.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink,
22+
"Sensitive data returned by $@ is stored in a build artifact here.", source.getNode(),
23+
source.getNode().(CleartextLogging::Source).describe()

‎javascript/ql/src/Security/CWE-312/examples/build-leak-fixed.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const webpack = require("webpack");
2+
3+
module.exports = [{
4+
plugins: [
5+
new webpack.DefinePlugin({
6+
'process.env': JSON.stringify({ DEBUG: process.env.DEBUG })
7+
})
8+
]
9+
}];

‎javascript/ql/src/Security/CWE-312/examples/build-leak.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const webpack = require("webpack");
2+
3+
module.exports = [{
4+
plugins: [
5+
new webpack.DefinePlugin({
6+
"process.env": JSON.stringify(process.env)
7+
})
8+
]
9+
}];

‎javascript/ql/src/semmle/javascript/Arrays.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ module ArrayTaintTracking {
4040
succ = call
4141
)
4242
or
43+
// `array.reduce` with tainted value in callback
44+
call.(DataFlow::MethodCallNode).getMethodName() = "reduce" and
45+
pred = call.getArgument(0).(DataFlow::FunctionNode).getAReturn() and // Require the argument to be a closure to avoid spurious call/return flow
46+
succ = call
47+
or
4348
// `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
4449
exists(string name |
4550
name = "push" or

‎javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeak.qll

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/**
2+
* Provides a dataflow tracking configuration for reasoning about
3+
* storage of sensitive information in build artifact.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `CleartextLogging::Configuration` is needed, otherwise
7+
* `CleartextLoggingCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
12+
/**
13+
* Classes and predicates for storage of sensitive information in build artifact query.
14+
*/
15+
module BuildArtifactLeak {
16+
import BuildArtifactLeakCustomizations::BuildArtifactLeak
17+
import CleartextLoggingCustomizations::CleartextLogging as CleartextLogging
18+
19+
/**
20+
* A taint tracking configuration for storage of sensitive information in build artifact.
21+
*/
22+
class Configuration extends TaintTracking::Configuration {
23+
Configuration() { this = "BuildArtifactLeak" }
24+
25+
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
26+
source.(CleartextLogging::Source).getLabel() = lbl
27+
}
28+
29+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel lbl) {
30+
sink.(Sink).getLabel() = lbl
31+
}
32+
33+
override predicate isSanitizer(DataFlow::Node node) {
34+
node instanceof CleartextLogging::Barrier
35+
}
36+
37+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
38+
CleartextLogging::isSanitizerEdge(pred, succ)
39+
}
40+
41+
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
42+
CleartextLogging::isAdditionalTaintStep(src, trg)
43+
}
44+
}
45+
}

‎javascript/ql/src/semmle/javascript/security/dataflow/BuildArtifactLeakCustomizations.qll

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/**
2+
* Provides default sinks for reasoning about storage of sensitive information
3+
* in build artifact, as well as extension points for adding your own.
4+
*/
5+
6+
import javascript
7+
private import semmle.javascript.dataflow.InferredTypes
8+
private import semmle.javascript.security.SensitiveActions::HeuristicNames
9+
10+
/**
11+
* Sinks for storage of sensitive information in build artifact.
12+
*/
13+
module BuildArtifactLeak {
14+
/**
15+
* A data flow sink for storage of sensitive information in a build artifact.
16+
*/
17+
abstract class Sink extends DataFlow::Node {
18+
/**
19+
* Gets a data-flow label that leaks information for this sink.
20+
*/
21+
DataFlow::FlowLabel getLabel() { result.isTaint() }
22+
}
23+
24+
/**
25+
* An instantiation of `webpack.DefintePlugin` that stores information in a compiled JavaScript file.
26+
*/
27+
class WebpackDefinePluginSink extends Sink {
28+
WebpackDefinePluginSink() {
29+
this = DataFlow::moduleMember("webpack", "DefinePlugin").getAnInstantiation().getAnArgument()
30+
}
31+
}
32+
}

‎javascript/ql/src/semmle/javascript/security/dataflow/CleartextLogging.qll

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,23 +35,11 @@ module CleartextLogging {
3535
override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier }
3636

3737
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
38-
succ.(DataFlow::PropRead).getBase() = pred
38+
CleartextLogging::isSanitizerEdge(pred, succ)
3939
}
4040

4141
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
42-
// A taint propagating data flow edge through objects: a tainted write taints the entire object.
43-
exists(DataFlow::PropWrite write |
44-
write.getRhs() = src and
45-
trg.(DataFlow::SourceNode).flowsTo(write.getBase())
46-
)
47-
or
48-
// Taint through the arguments object.
49-
exists(DataFlow::CallNode call, Function f |
50-
src = call.getAnArgument() and
51-
f = call.getACallee() and
52-
not call.isImprecise() and
53-
trg.asExpr() = f.getArgumentsVariable().getAnAccess()
54-
)
42+
CleartextLogging::isAdditionalTaintStep(src, trg)
5543
}
5644
}
5745
}

‎javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -176,17 +176,50 @@ module CleartextLogging {
176176

177177
override string describe() { result = "process environment" }
178178

179-
override DataFlow::FlowLabel getLabel() {
180-
result.isTaint() or
181-
result instanceof PartiallySensitiveMap
182-
}
179+
override DataFlow::FlowLabel getLabel() { result.isTaint() }
180+
}
181+
182+
/**
183+
* Holds if the edge `pred` -> `succ` should be sanitized for clear-text logging of sensitive information.
184+
*/
185+
predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
186+
succ.(DataFlow::PropRead).getBase() = pred
183187
}
184188

185189
/**
186-
* A flow label describing a map that might contain sensitive information in some properties.
187-
* Property reads on such maps where the property name is fixed is unlikely to leak sensitive information.
190+
* Holds if the edge `src` -> `trg` is an additional taint-step for clear-text logging of sensitive information.
188191
*/
189-
class PartiallySensitiveMap extends DataFlow::FlowLabel {
190-
PartiallySensitiveMap() { this = "PartiallySensitiveMap" }
192+
predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
193+
// A taint propagating data flow edge through objects: a tainted write taints the entire object.
194+
exists(DataFlow::PropWrite write |
195+
write.getRhs() = src and
196+
trg.(DataFlow::SourceNode).flowsTo(write.getBase())
197+
)
198+
or
199+
// A property-copy step,
200+
// dst[x] = src[x]
201+
// dst[x] = JSON.stringify(src[x])
202+
exists(DataFlow::PropWrite write, DataFlow::PropRead read |
203+
read = write.getRhs()
204+
or
205+
exists(DataFlow::MethodCallNode stringify |
206+
stringify = write.getRhs() and
207+
stringify = DataFlow::globalVarRef("JSON").getAMethodCall("stringify") and
208+
stringify.getArgument(0) = read
209+
)
210+
|
211+
not exists(write.getPropertyName()) and
212+
not exists(read.getPropertyName()) and
213+
src = read.getBase() and
214+
trg = write.getBase().getALocalSource()
215+
)
216+
or
217+
// Taint through the arguments object.
218+
exists(DataFlow::CallNode call, Function f |
219+
src = call.getAnArgument() and
220+
f = call.getACallee() and
221+
not call.isImprecise() and
222+
trg.asExpr() = f.getArgumentsVariable().getAnAccess()
223+
)
191224
}
192225
}

0 commit comments

Comments
 (0)