Skip to content

Commit b24fba8

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #3734 from dellalibera/loginjection
Approved by esbena
2 parents 4642bb2 + d9a0dc0 commit b24fba8

File tree

5 files changed

+285
-0
lines changed

5 files changed

+285
-0
lines changed

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
8+
<p>If unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries.</p>
9+
10+
<p>Forgery can occur if a user provides some input with characters that are interpreted
11+
when the log output is displayed. If the log is displayed as a plain text file, then new
12+
line characters can be used by a malicious user. If the log is displayed as HTML, then
13+
arbitrary HTML may be included to spoof log entries.</p>
14+
</overview>
15+
16+
<recommendation>
17+
<p>
18+
User input should be suitably sanitized before it is logged.
19+
</p>
20+
<p>
21+
If the log entries are plain text then line breaks should be removed from user input, using
22+
<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.
24+
</p>
25+
<p>
26+
For log entries that will be displayed in HTML, user input should be HTML encoded before being logged, to prevent forgery and
27+
other forms of HTML injection.
28+
</p>
29+
30+
</recommendation>
31+
32+
<example>
33+
<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`.
35+
If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter,
36+
the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`.
37+
</p>
38+
<sample src="examples/logInjectionBad.js" />
39+
40+
<p> In the second example, <code>String.prototype.replace</code> is used to ensure no line endings are present in the user input.</p>
41+
<sample src="examples/logInjectionGood.js" />
42+
</example>
43+
44+
<references>
45+
<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
46+
</references>
47+
</qhelp>

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Log Injection
3+
* @description Building log entries from user-controlled sources is vulnerable to
4+
* insertion of forged log entries by a malicious user.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/log-injection
9+
* @tags security
10+
* external/cwe/cwe-117
11+
*/
12+
13+
import javascript
14+
import DataFlow::PathGraph
15+
import LogInjection::LogInjection
16+
17+
from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
18+
where config.hasFlowPath(source, sink)
19+
select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
20+
"User-provided value"

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
3+
*/
4+
5+
import javascript
6+
7+
module LogInjection {
8+
/**
9+
* A data flow source for user input used in log entries.
10+
*/
11+
abstract class Source extends DataFlow::Node { }
12+
13+
/**
14+
* A data flow sink for user input used in log entries.
15+
*/
16+
abstract class Sink extends DataFlow::Node { }
17+
18+
/**
19+
* A sanitizer for malicious user input used in log entries.
20+
*/
21+
abstract class Sanitizer extends DataFlow::Node { }
22+
23+
/**
24+
* A taint-tracking configuration for untrusted user input used in log entries.
25+
*/
26+
class LogInjectionConfiguration extends TaintTracking::Configuration {
27+
LogInjectionConfiguration() { this = "LogInjection" }
28+
29+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
30+
31+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
32+
33+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
34+
}
35+
36+
/**
37+
* A source of remote user controlled input.
38+
*/
39+
class RemoteSource extends Source {
40+
RemoteSource() { this instanceof RemoteFlowSource }
41+
}
42+
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+
77+
/**
78+
* An argument to a logging mechanism.
79+
*/
80+
class LoggingSink extends Sink {
81+
LoggingSink() { this = any(LoggingCall console).getAnArgument() }
82+
}
83+
84+
/**
85+
* A call to `String.prototype.replace` that replaces `\n` is considered to sanitize the replaced string (reduce false positive).
86+
*/
87+
class StringReplaceSanitizer extends Sanitizer {
88+
StringReplaceSanitizer() {
89+
exists(string s | this.(StringReplaceCall).replaces(s, "") and s.regexpMatch("\\n"))
90+
}
91+
}
92+
93+
/**
94+
* A call to an HTML sanitizer is considered to sanitize the user input.
95+
*/
96+
class HtmlSanitizer extends Sanitizer {
97+
HtmlSanitizer() { this instanceof HtmlSanitizerCall }
98+
}
99+
}

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
const http = require('http');
2+
const hostname = '127.0.0.1';
3+
const port = 3000;
4+
const url = require('url');
5+
6+
7+
const check_username = (username) => {
8+
if (username != 'name') throw `${username} is not valid`;
9+
// do something
10+
}
11+
12+
const my_logger = {
13+
log: console.log
14+
}
15+
16+
const another_logger = console.log
17+
18+
// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
19+
20+
21+
22+
const server = http.createServer((req, res) => {
23+
let q = url.parse(req.url, true);
24+
25+
let username = q.query.username;
26+
27+
// BAD: User input logged as-is
28+
console.info(`[INFO] User: ${username}`);
29+
// [INFO] User: Guest
30+
// [INFO] User: Admin
31+
//
32+
33+
// BAD: User input logged as-is
34+
console.info(`[INFO] User: %s`, username);
35+
// [INFO] User: Guest
36+
// [INFO] User: Admin
37+
//
38+
39+
40+
// BAD: User input logged as-is
41+
my_logger.log('[INFO] User:', username);
42+
// [INFO] User: Guest
43+
// [INFO] User: Admin
44+
//
45+
46+
// BAD: User input logged as-is
47+
another_logger('[INFO] User:', username);
48+
// [INFO] User: Guest
49+
// [INFO] User: Admin
50+
//
51+
52+
try {
53+
check_username(username)
54+
55+
} catch (error) {
56+
// BAD: Error with user input logged as-is
57+
console.error(`[ERROR] Error: "${error}"`);
58+
// [ERROR] Error: "Guest
59+
// [INFO] User: Admin
60+
// is not valid"
61+
62+
}
63+
64+
})
65+
66+
server.listen(port, hostname, () => {
67+
console.log(`Server running at http://${hostname}:${port}/`);
68+
});

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
const http = require('http');
2+
const hostname = '127.0.0.1';
3+
const port = 3000;
4+
const url = require('url');
5+
6+
const check_username = (username) => {
7+
if (username != 'name') throw `${username} is not valid`;
8+
// do something
9+
}
10+
11+
const logger = {
12+
log: console.log
13+
}
14+
15+
const another_logger = console.log
16+
17+
// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
18+
19+
const server = http.createServer((req, res) => {
20+
let q = url.parse(req.url, true);
21+
22+
// GOOD: remove `\n` line from user controlled input before logging
23+
let username = q.query.username.replace(/\n/g, "");
24+
25+
console.info(`[INFO] User: ${username}`);
26+
// [INFO] User: Guest[INFO] User: Admin
27+
28+
console.info(`[INFO] User: %s`, username);
29+
// [INFO] User: Guest[INFO] User: Admin
30+
31+
logger.log('[INFO] User:', username);
32+
// [INFO] User: Guest[INFO] User: Admin
33+
34+
another_logger('[INFO] User:', username);
35+
// [INFO] User: Guest[INFO] User: Admin
36+
37+
try {
38+
check_username(username)
39+
40+
} catch (error) {
41+
console.error(`[ERROR] Error: "${error}"`);
42+
// [ERROR] Error: "Guest[INFO] User: Admin is not valid"
43+
44+
}
45+
46+
})
47+
48+
server.listen(port, hostname, () => {
49+
console.log(`Server running at http://${hostname}:${port}/`);
50+
});
51+

0 commit comments

Comments
 (0)