Skip to content

Commit 23981cb

Browse files
erik-krogherik-krogh
authored
Merge pull request #7626 from erik-krogh/CWE-377
JS: add query for detecting insecure temporary files
2 parents a9f6d20 + 4c317f5 commit 23981cb

File tree

10 files changed

+292
-0
lines changed

10 files changed

+292
-0
lines changed

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* insecure temporary file creation, as well as
4+
* extension points for adding your own.
5+
*/
6+
7+
import javascript
8+
9+
/**
10+
* Classes and predicates for reasoning about insecure temporary file creation.
11+
*/
12+
module InsecureTemporaryFile {
13+
/**
14+
* A data flow source for insecure temporary file creation.
15+
*/
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/**
19+
* A data flow sink for insecure temporary file creation.
20+
*/
21+
abstract class Sink extends DataFlow::Node { }
22+
23+
/**
24+
* A sanitizer for random insecure temporary file creation.
25+
*/
26+
abstract class Sanitizer extends DataFlow::Node { }
27+
28+
/** A call that opens a file with a given path. */
29+
class OpenFileCall extends DataFlow::CallNode {
30+
string methodName;
31+
32+
OpenFileCall() {
33+
methodName =
34+
[
35+
"open", "openSync", "writeFile", "writeFileSync", "writeJson", "writeJSON",
36+
"writeJsonSync", "writeJSONSync", "outputJson", "outputJSON", "outputJsonSync",
37+
"outputJSONSync", "outputFile", "outputFileSync"
38+
] and
39+
this = NodeJSLib::FS::moduleMember(methodName).getACall()
40+
}
41+
42+
DataFlow::Node getPath() { result = this.getArgument(0) }
43+
44+
DataFlow::Node getMode() {
45+
methodName = ["open", "openSync"] and
46+
result = this.getArgument(2)
47+
or
48+
not methodName = ["open", "openSync"] and
49+
result = this.getOptionArgument(2, "mode")
50+
}
51+
}
52+
53+
/** Holds if the `mode` ensure no access to other users. */
54+
bindingset[mode]
55+
private predicate isSecureMode(int mode) {
56+
// the lowest 6 bits should be 0.
57+
// E.g. `0o600` is secure (each digit in a octal number is 3 bits)
58+
mode.bitAnd(1) = 0 and
59+
mode.bitAnd(2) = 0 and
60+
mode.bitAnd(4) = 0 and
61+
mode.bitAnd(8) = 0 and
62+
mode.bitAnd(16) = 0 and
63+
mode.bitAnd(32) = 0
64+
}
65+
66+
/** The path in a call that opens a file without specifying a secure `mode`. Seen as a sink for insecure temporary file creation. */
67+
class InsecureFileOpen extends Sink {
68+
InsecureFileOpen() {
69+
exists(OpenFileCall call |
70+
not exists(call.getMode())
71+
or
72+
exists(int mode | mode = call.getMode().getIntValue() | not isSecureMode(mode))
73+
|
74+
this = call.getPath()
75+
)
76+
}
77+
}
78+
79+
/** A string that references the global tmp dir. Seen as a source for insecure temporary file creation. */
80+
class OSTempDir extends Source {
81+
OSTempDir() {
82+
this = DataFlow::moduleImport("os").getAMemberCall("tmpdir")
83+
or
84+
this.getStringValue().matches("/tmp/%")
85+
}
86+
}
87+
88+
/** A non-first leaf in a string-concatenation. Seen as a sanitizer for insecure temporary file creation. */
89+
class NonFirstStringConcatLeaf extends Sanitizer {
90+
NonFirstStringConcatLeaf() {
91+
exists(StringOps::ConcatenationRoot root |
92+
this = root.getALeaf() and
93+
not this = root.getFirstLeaf()
94+
)
95+
or
96+
exists(DataFlow::CallNode join |
97+
join = DataFlow::moduleMember("path", "join").getACall() and
98+
this = join.getArgument([1 .. join.getNumArgument() - 1])
99+
)
100+
}
101+
}
102+
}

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about insecure temporary
3+
* file creation.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `InsecureTemporaryFile::Configuration` is needed, otherwise
7+
* `InsecureTemporaryFileCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
import InsecureTemporaryFileCustomizations::InsecureTemporaryFile
12+
13+
/**
14+
* A taint-tracking configuration for reasoning about insecure temporary file creation.
15+
*/
16+
class Configuration extends TaintTracking::Configuration {
17+
Configuration() { this = "InsecureTemporaryFile" }
18+
19+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
20+
21+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
22+
23+
override predicate isSanitizer(DataFlow::Node node) {
24+
super.isSanitizer(node) or
25+
node instanceof Sanitizer
26+
}
27+
}

‎javascript/ql/src/Security/CWE-377/InsecureTemporaryFile.qhelp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Temporary files created in the operating system's temporary directory are by default accessible
7+
to other users. In some cases, this can lead to information exposure, or in the worst
8+
case, to remote code execution.
9+
</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>
14+
Use a well-tested library like <a href="https://www.npmjs.com/package/tmp">tmp</a>
15+
for creating temporary files. These libraries ensure both that the file is inaccessible
16+
to other users and that the file does not already exist.
17+
</p>
18+
</recommendation>
19+
20+
<example>
21+
<p>
22+
The following example creates a temporary file in the operating system's temporary directory.
23+
</p>
24+
<sample src="examples/insecure-temporary-file.js" />
25+
26+
<p>
27+
The file created above is accessible to other users, and there is no guarantee that
28+
the file does not already exist.
29+
</p>
30+
<p>
31+
The below example uses the <a href="https://www.npmjs.com/package/tmp">tmp</a> library
32+
to securely create a temporary file.
33+
</p>
34+
<sample src="examples/secure-temporary-file.js" />
35+
36+
</example>
37+
38+
<references>
39+
<li>Mitre.org: <a href="https://cwe.mitre.org/data/definitions/377.html">CWE-377</a>.</li>
40+
<li>NPM: <a href="https://www.npmjs.com/package/tmp">tmp</a>.</li>
41+
</references>
42+
43+
</qhelp>

‎javascript/ql/src/Security/CWE-377/InsecureTemporaryFile.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Insecure temporary file
3+
* @description Creating a temporary file that is accessible by other users TODO:
4+
* @kind path-problem
5+
* @id js/insecure-temporary-file
6+
* @problem.severity warning
7+
* @security-severity 7.0
8+
* @precision medium
9+
* @tags external/cwe/cwe-377
10+
* external/cwe/cwe-378
11+
* security
12+
*/
13+
14+
import javascript
15+
import DataFlow::PathGraph
16+
import semmle.javascript.security.dataflow.InsecureTemporaryFileQuery
17+
18+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where cfg.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "Insecure creation of file in $@.", source.getNode(),
21+
"the os temp dir"

‎javascript/ql/src/Security/CWE-377/examples/insecure-temporary-file.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
const fs = require('fs');
2+
const os = require('os');
3+
const path = require('path');
4+
5+
const file = path.join(os.tmpdir(), "test-" + (new Date()).getTime() + ".txt");
6+
fs.writeFileSync(file, "content");

‎javascript/ql/src/Security/CWE-377/examples/secure-temporary-file.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
const fs = require('fs');
2+
const tmp = require('tmp');
3+
4+
const file = tmp.fileSync().name;
5+
fs.writeFileSync(file, "content");

‎javascript/ql/src/change-notes/2022-01-18-insecure-temporary-file.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* A new query `js/insecure-temporary-file` has been added. The query detects the creation of temporary files that may be accessible by others users. The query is not run by default.

‎javascript/ql/test/query-tests/Security/CWE-377/InsecureTemporaryFile.expected

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
nodes
2+
| insecure-temporary-file.js:7:9:11:5 | tmpLocation |
3+
| insecure-temporary-file.js:7:23:11:5 | path.jo ... )\\n ) |
4+
| insecure-temporary-file.js:8:9:8:45 | os.tmpd ... mpDir() |
5+
| insecure-temporary-file.js:8:21:8:31 | os.tmpdir() |
6+
| insecure-temporary-file.js:8:21:8:31 | os.tmpdir() |
7+
| insecure-temporary-file.js:13:22:13:32 | tmpLocation |
8+
| insecure-temporary-file.js:13:22:13:32 | tmpLocation |
9+
| insecure-temporary-file.js:15:9:15:34 | tmpPath |
10+
| insecure-temporary-file.js:15:19:15:34 | "/tmp/something" |
11+
| insecure-temporary-file.js:15:19:15:34 | "/tmp/something" |
12+
| insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") |
13+
| insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") |
14+
| insecure-temporary-file.js:17:32:17:38 | tmpPath |
15+
| insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") |
16+
| insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") |
17+
| insecure-temporary-file.js:23:32:23:38 | tmpPath |
18+
| insecure-temporary-file.js:25:11:25:92 | tmpPath2 |
19+
| insecure-temporary-file.js:25:22:25:92 | path.jo ... )}.md`) |
20+
| insecure-temporary-file.js:25:32:25:42 | os.tmpdir() |
21+
| insecure-temporary-file.js:25:32:25:42 | os.tmpdir() |
22+
| insecure-temporary-file.js:26:22:26:29 | tmpPath2 |
23+
| insecure-temporary-file.js:26:22:26:29 | tmpPath2 |
24+
| insecure-temporary-file.js:28:17:28:24 | tmpPath2 |
25+
| insecure-temporary-file.js:28:17:28:24 | tmpPath2 |
26+
edges
27+
| insecure-temporary-file.js:7:9:11:5 | tmpLocation | insecure-temporary-file.js:13:22:13:32 | tmpLocation |
28+
| insecure-temporary-file.js:7:9:11:5 | tmpLocation | insecure-temporary-file.js:13:22:13:32 | tmpLocation |
29+
| insecure-temporary-file.js:7:23:11:5 | path.jo ... )\\n ) | insecure-temporary-file.js:7:9:11:5 | tmpLocation |
30+
| insecure-temporary-file.js:8:9:8:45 | os.tmpd ... mpDir() | insecure-temporary-file.js:7:23:11:5 | path.jo ... )\\n ) |
31+
| insecure-temporary-file.js:8:21:8:31 | os.tmpdir() | insecure-temporary-file.js:8:9:8:45 | os.tmpd ... mpDir() |
32+
| insecure-temporary-file.js:8:21:8:31 | os.tmpdir() | insecure-temporary-file.js:8:9:8:45 | os.tmpd ... mpDir() |
33+
| insecure-temporary-file.js:15:9:15:34 | tmpPath | insecure-temporary-file.js:17:32:17:38 | tmpPath |
34+
| insecure-temporary-file.js:15:9:15:34 | tmpPath | insecure-temporary-file.js:23:32:23:38 | tmpPath |
35+
| insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | insecure-temporary-file.js:15:9:15:34 | tmpPath |
36+
| insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | insecure-temporary-file.js:15:9:15:34 | tmpPath |
37+
| insecure-temporary-file.js:17:32:17:38 | tmpPath | insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") |
38+
| insecure-temporary-file.js:17:32:17:38 | tmpPath | insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") |
39+
| insecure-temporary-file.js:23:32:23:38 | tmpPath | insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") |
40+
| insecure-temporary-file.js:23:32:23:38 | tmpPath | insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") |
41+
| insecure-temporary-file.js:25:11:25:92 | tmpPath2 | insecure-temporary-file.js:26:22:26:29 | tmpPath2 |
42+
| insecure-temporary-file.js:25:11:25:92 | tmpPath2 | insecure-temporary-file.js:26:22:26:29 | tmpPath2 |
43+
| insecure-temporary-file.js:25:11:25:92 | tmpPath2 | insecure-temporary-file.js:28:17:28:24 | tmpPath2 |
44+
| insecure-temporary-file.js:25:11:25:92 | tmpPath2 | insecure-temporary-file.js:28:17:28:24 | tmpPath2 |
45+
| insecure-temporary-file.js:25:22:25:92 | path.jo ... )}.md`) | insecure-temporary-file.js:25:11:25:92 | tmpPath2 |
46+
| insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | insecure-temporary-file.js:25:22:25:92 | path.jo ... )}.md`) |
47+
| insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | insecure-temporary-file.js:25:22:25:92 | path.jo ... )}.md`) |
48+
#select
49+
| insecure-temporary-file.js:13:22:13:32 | tmpLocation | insecure-temporary-file.js:8:21:8:31 | os.tmpdir() | insecure-temporary-file.js:13:22:13:32 | tmpLocation | Insecure creation of file in $@. | insecure-temporary-file.js:8:21:8:31 | os.tmpdir() | the os temp dir |
50+
| insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") | insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | insecure-temporary-file.js:17:22:17:49 | path.jo ... /foo/") | Insecure creation of file in $@. | insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | the os temp dir |
51+
| insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") | insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | insecure-temporary-file.js:23:22:23:49 | path.jo ... /foo/") | Insecure creation of file in $@. | insecure-temporary-file.js:15:19:15:34 | "/tmp/something" | the os temp dir |
52+
| insecure-temporary-file.js:26:22:26:29 | tmpPath2 | insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | insecure-temporary-file.js:26:22:26:29 | tmpPath2 | Insecure creation of file in $@. | insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | the os temp dir |
53+
| insecure-temporary-file.js:28:17:28:24 | tmpPath2 | insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | insecure-temporary-file.js:28:17:28:24 | tmpPath2 | Insecure creation of file in $@. | insecure-temporary-file.js:25:32:25:42 | os.tmpdir() | the os temp dir |

‎javascript/ql/test/query-tests/Security/CWE-377/InsecureTemporaryFile.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-377/InsecureTemporaryFile.ql

‎javascript/ql/test/query-tests/Security/CWE-377/insecure-temporary-file.js

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
const os = require('os');
2+
const uuid = require('node-uuid');
3+
const fs = require('fs');
4+
const path = require('path');
5+
6+
(function main() {
7+
var tmpLocation = path.join(
8+
os.tmpdir ? os.tmpdir() : os.tmpDir(),
9+
'something',
10+
uuid.v4().slice(0, 8)
11+
);
12+
13+
fs.writeFileSync(tmpLocation, content); // NOT OK
14+
15+
var tmpPath = "/tmp/something";
16+
fs.writeFileSync(path.join("./foo/", tmpPath), content); // OK
17+
fs.writeFileSync(path.join(tmpPath, "./foo/"), content); // NOT OK
18+
19+
fs.writeFileSync(path.join(tmpPath, "./foo/"), content, {mode: 0o600}); // OK
20+
21+
fs.writeFileSync(path.join(tmpPath, "./foo/"), content, {mode: mode}); // OK - assumed unknown mode is secure
22+
23+
fs.writeFileSync(path.join(tmpPath, "./foo/"), content, {mode: 0o666}); // NOT OK - explicitly insecure
24+
25+
const tmpPath2 = path.join(os.tmpdir(), `tmp_${Math.floor(Math.random() * 1000000)}.md`);
26+
fs.writeFileSync(tmpPath2, content); // NOT OK
27+
28+
fs.openSync(tmpPath2, 'w'); // NOT OK
29+
fs.openSync(tmpPath2, 'w', 0o600); // OK
30+
})

0 commit comments

Comments
 (0)