Skip to content

Commit 4229f55

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#4751 from erik-krogh/logInjection
Approved by asgerf, mchammer01
2 parents b6107d3 + c98dacf commit 4229f55

File tree

13 files changed

+179
-174
lines changed

13 files changed

+179
-174
lines changed

‎javascript/change-notes/2020-11-30-loginjection.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The `js/log-injection` query has been moved into non-experimental, and the precision of the query has been changed to medium.

‎javascript/ql/src/experimental/Security/CWE-117/LogInjection.help renamed to ‎javascript/ql/src/Security/CWE-117/LogInjection.qhelp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,20 @@ arbitrary HTML may be included to spoof log entries.</p>
1818
User input should be suitably sanitized before it is logged.
1919
</p>
2020
<p>
21-
If the log entries are plain text then line breaks should be removed from user input, using
21+
If the log entries are in plain text then line breaks should be removed from user input, using
2222
<code>String.prototype.replace</code> or similar. Care should also be taken that user input is clearly marked
23-
in log entries, and that a malicious user cannot cause confusion in other ways.
23+
in log entries.
2424
</p>
2525
<p>
26-
For log entries that will be displayed in HTML, user input should be HTML encoded before being logged, to prevent forgery and
26+
For log entries that will be displayed in HTML, user input should be HTML-encoded before being logged, to prevent forgery and
2727
other forms of HTML injection.
2828
</p>
2929

3030
</recommendation>
3131

3232
<example>
3333
<p>In the first example, a username, provided by the user, is logged using `console.info`. In
34-
the first case, it is logged without any sanitization. In the second case the username is used to build an error that is logged using `console.error`.
34+
the first case, it is logged without any sanitization. In the second case, the username is used to build an error that is logged using `console.error`.
3535
If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter,
3636
the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`.
3737
</p>

‎javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql renamed to ‎javascript/ql/src/Security/CWE-117/LogInjection.ql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
/**
2-
* @name Log Injection
2+
* @name Log injection
33
* @description Building log entries from user-controlled sources is vulnerable to
44
* insertion of forged log entries by a malicious user.
55
* @kind path-problem
66
* @problem.severity error
7-
* @precision high
7+
* @precision medium
88
* @id js/log-injection
99
* @tags security
1010
* external/cwe/cwe-117
1111
*/
1212

1313
import javascript
1414
import DataFlow::PathGraph
15-
import LogInjection::LogInjection
15+
import semmle.javascript.security.dataflow.LogInjection::LogInjection
1616

1717
from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
1818
where config.hasFlowPath(source, sink)

‎javascript/ql/src/Security/CWE-117/examples/logInjectionBad.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
const http = require('http');
2+
const url = require('url');
3+
4+
const server = http.createServer((req, res) => {
5+
let q = url.parse(req.url, true);
6+
7+
console.info(`[INFO] User: ${q.query.username}`); // BAD: User input logged as-is
8+
})
9+
10+
server.listen(3000, '127.0.0.1', () => {});

‎javascript/ql/src/Security/CWE-117/examples/logInjectionGood.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
const http = require('http');
2+
const url = require('url');
3+
4+
const server = http.createServer((req, res) => {
5+
let q = url.parse(req.url, true);
6+
7+
// GOOD: remove newlines from user controlled input before logging
8+
let username = q.query.username.replace(/\n|\r/g, "");
9+
10+
console.info(`[INFO] User: ${username}`);
11+
});
12+
13+
server.listen(3000, '127.0.0.1', () => {});

‎javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js

Lines changed: 0 additions & 68 deletions
This file was deleted.

‎javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js

Lines changed: 0 additions & 51 deletions
This file was deleted.

‎javascript/ql/src/semmle/javascript/frameworks/Logging.qll

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,22 @@ string getAStandardLoggerMethodName() {
3838
*/
3939
private module Console {
4040
/**
41-
* Gets a data flow source node for the console library.
41+
* An API entrypoint for the global `console` variable.
4242
*/
43-
private DataFlow::SourceNode console() {
44-
result = DataFlow::moduleImport("console") or
45-
result = DataFlow::globalVarRef("console")
43+
private class ConsoleGlobalEntry extends API::EntryPoint {
44+
ConsoleGlobalEntry() { this = "ConsoleGlobalEntry" }
45+
46+
override DataFlow::SourceNode getAUse() { result = DataFlow::globalVarRef("console") }
47+
48+
override DataFlow::Node getARhs() { none() }
49+
}
50+
51+
/**
52+
* Gets a api node for the console library.
53+
*/
54+
private API::Node console() {
55+
result = API::moduleImport("console") or
56+
result = API::root().getASuccessor(any(ConsoleGlobalEntry e))
4657
}
4758

4859
/**
@@ -56,7 +67,7 @@ private module Console {
5667
name = getAStandardLoggerMethodName() or
5768
name = "assert"
5869
) and
59-
this = console().getAMemberCall(name)
70+
this = console().getMember(name).getACall()
6071
}
6172

6273
override DataFlow::Node getAMessageComponent() {
@@ -85,7 +96,7 @@ private module Loglevel {
8596
*/
8697
class LoglevelLoggerCall extends LoggerCall {
8798
LoglevelLoggerCall() {
88-
this = DataFlow::moduleMember("loglevel", getAStandardLoggerMethodName()).getACall()
99+
this = API::moduleImport("loglevel").getMember(getAStandardLoggerMethodName()).getACall()
89100
}
90101

91102
override DataFlow::Node getAMessageComponent() { result = getAnArgument() }
@@ -102,9 +113,11 @@ private module Winston {
102113
class WinstonLoggerCall extends LoggerCall, DataFlow::MethodCallNode {
103114
WinstonLoggerCall() {
104115
this =
105-
DataFlow::moduleMember("winston", "createLogger")
116+
API::moduleImport("winston")
117+
.getMember("createLogger")
118+
.getReturn()
119+
.getMember(getAStandardLoggerMethodName())
106120
.getACall()
107-
.getAMethodCall(getAStandardLoggerMethodName())
108121
}
109122

110123
override DataFlow::Node getAMessageComponent() {
@@ -125,9 +138,11 @@ private module log4js {
125138
class Log4jsLoggerCall extends LoggerCall {
126139
Log4jsLoggerCall() {
127140
this =
128-
DataFlow::moduleMember("log4js", "getLogger")
141+
API::moduleImport("log4js")
142+
.getMember("getLogger")
143+
.getReturn()
144+
.getMember(getAStandardLoggerMethodName())
129145
.getACall()
130-
.getAMethodCall(getAStandardLoggerMethodName())
131146
}
132147

133148
override DataFlow::Node getAMessageComponent() { result = getAnArgument() }
@@ -145,7 +160,7 @@ private module Npmlog {
145160
string name;
146161

147162
Npmlog() {
148-
this = DataFlow::moduleMember("npmlog", name).getACall() and
163+
this = API::moduleImport("npmlog").getMember(name).getACall() and
149164
name = getAStandardLoggerMethodName()
150165
}
151166

@@ -170,8 +185,8 @@ private module Fancylog {
170185
*/
171186
class Fancylog extends LoggerCall {
172187
Fancylog() {
173-
this = DataFlow::moduleMember("fancy-log", getAStandardLoggerMethodName()).getACall() or
174-
this = DataFlow::moduleImport("fancy-log").getACall()
188+
this = API::moduleImport("fancy-log").getMember(getAStandardLoggerMethodName()).getACall() or
189+
this = API::moduleImport("fancy-log").getACall()
175190
}
176191

177192
override DataFlow::Node getAMessageComponent() { result = getAnArgument() }

‎javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll renamed to ‎javascript/ql/src/semmle/javascript/security/dataflow/LogInjection.qll

Lines changed: 4 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44

55
import javascript
66

7+
/**
8+
* Provides default sources, sink, and sanitizers for reasoning about untrusted user input used in log entries.
9+
*/
710
module LogInjection {
811
/**
912
* A data flow source for user input used in log entries.
@@ -40,45 +43,11 @@ module LogInjection {
4043
RemoteSource() { this instanceof RemoteFlowSource }
4144
}
4245

43-
/**
44-
* An source node representing a logging mechanism.
45-
*/
46-
class ConsoleSource extends DataFlow::SourceNode {
47-
ConsoleSource() {
48-
exists(DataFlow::SourceNode node |
49-
node = this and this = DataFlow::moduleImport("console")
50-
or
51-
this = DataFlow::globalVarRef("console")
52-
)
53-
}
54-
}
55-
56-
/**
57-
* A call to a logging mechanism. For example, the call could be in the following forms:
58-
* `console.log('hello')` or
59-
*
60-
* `let logger = console.log;`
61-
* `logger('hello')` or
62-
*
63-
* `let logger = {info: console.log};`
64-
* `logger.info('hello')`
65-
*/
66-
class LoggingCall extends DataFlow::CallNode {
67-
LoggingCall() {
68-
exists(DataFlow::SourceNode node, string propName |
69-
any(ConsoleSource console).getAPropertyRead() = node.getAPropertySource(propName) and
70-
this = node.getAPropertyRead(propName).getACall()
71-
)
72-
or
73-
this = any(LoggerCall call)
74-
}
75-
}
76-
7746
/**
7847
* An argument to a logging mechanism.
7948
*/
8049
class LoggingSink extends Sink {
81-
LoggingSink() { this = any(LoggingCall console).getAnArgument() }
50+
LoggingSink() { this = any(LoggerCall console).getAMessageComponent() }
8251
}
8352

8453
/**

‎javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
nodes
2+
| logInjectionBad.js:19:9:19:36 | q |
3+
| logInjectionBad.js:19:13:19:36 | url.par ... , true) |
4+
| logInjectionBad.js:19:23:19:29 | req.url |
5+
| logInjectionBad.js:19:23:19:29 | req.url |
6+
| logInjectionBad.js:20:9:20:35 | username |
7+
| logInjectionBad.js:20:20:20:20 | q |
8+
| logInjectionBad.js:20:20:20:26 | q.query |
9+
| logInjectionBad.js:20:20:20:35 | q.query.username |
10+
| logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` |
11+
| logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` |
12+
| logInjectionBad.js:22:34:22:41 | username |
13+
| logInjectionBad.js:23:37:23:44 | username |
14+
| logInjectionBad.js:23:37:23:44 | username |
15+
| logInjectionBad.js:24:35:24:42 | username |
16+
| logInjectionBad.js:24:35:24:42 | username |
17+
| logInjectionBad.js:25:36:25:43 | username |
18+
| logInjectionBad.js:25:36:25:43 | username |
19+
| logInjectionBad.js:28:9:28:32 | exceptional return of check_u ... ername) |
20+
| logInjectionBad.js:28:24:28:31 | username |
21+
| logInjectionBad.js:29:14:29:18 | error |
22+
| logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` |
23+
| logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` |
24+
| logInjectionBad.js:30:42:30:46 | error |
25+
edges
26+
| logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
27+
| logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
28+
| logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:19:13:19:36 | url.par ... , true) |
29+
| logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:19:13:19:36 | url.par ... , true) |
30+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:22:34:22:41 | username |
31+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:23:37:23:44 | username |
32+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:23:37:23:44 | username |
33+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:24:35:24:42 | username |
34+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:24:35:24:42 | username |
35+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:25:36:25:43 | username |
36+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:25:36:25:43 | username |
37+
| logInjectionBad.js:20:9:20:35 | username | logInjectionBad.js:28:24:28:31 | username |
38+
| logInjectionBad.js:20:20:20:20 | q | logInjectionBad.js:20:20:20:26 | q.query |
39+
| logInjectionBad.js:20:20:20:26 | q.query | logInjectionBad.js:20:20:20:35 | q.query.username |
40+
| logInjectionBad.js:20:20:20:35 | q.query.username | logInjectionBad.js:20:9:20:35 | username |
41+
| logInjectionBad.js:22:34:22:41 | username | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` |
42+
| logInjectionBad.js:22:34:22:41 | username | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` |
43+
| logInjectionBad.js:28:9:28:32 | exceptional return of check_u ... ername) | logInjectionBad.js:29:14:29:18 | error |
44+
| logInjectionBad.js:28:24:28:31 | username | logInjectionBad.js:28:9:28:32 | exceptional return of check_u ... ername) |
45+
| logInjectionBad.js:29:14:29:18 | error | logInjectionBad.js:30:42:30:46 | error |
46+
| logInjectionBad.js:30:42:30:46 | error | logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` |
47+
| logInjectionBad.js:30:42:30:46 | error | logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` |
48+
#select
49+
| logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
50+
| logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
51+
| logInjectionBad.js:24:35:24:42 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:24:35:24:42 | username | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
52+
| logInjectionBad.js:25:36:25:43 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:25:36:25:43 | username | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
53+
| logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:30:23:30:49 | `[ERROR ... rror}"` | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |

0 commit comments

Comments
 (0)