mirror of
https://github.com/github/codeql.git
synced 2026-01-09 20:50:21 +01:00
Add CodeQL query to detect Log Injection in JS code
This commit is contained in:
ubuntu
@@ -0,0 +1,47 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
|
||||
<p>If unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries.</p>
|
||||
|
||||
<p>Forgery can occur if a user provides some input with characters that are interpreted
|
||||
when the log output is displayed. If the log is displayed as a plain text file, then new
|
||||
line characters can be used by a malicious user. If the log is displayed as HTML, then
|
||||
arbitrary HTML may be include to spoof log entries.</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
User input should be suitably sanitized before it is logged.
|
||||
</p>
|
||||
<p>
|
||||
If the log entries are plain text then line breaks should be removed from user input, using
|
||||
<code>String.prototype.replace</code> or similar. Care should also be taken that user input is clearly marked
|
||||
in log entries, and that a malicious user cannot cause confusion in other ways.
|
||||
</p>
|
||||
<p>
|
||||
For log entries that will be displayed in HTML, user input should be HTML encoded before being logged, to prevent forgery and
|
||||
other forms of HTML injection.
|
||||
</p>
|
||||
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>In the first example, a username, provided by the user, is logged using `console.info`. In
|
||||
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`.
|
||||
If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter,
|
||||
the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`.
|
||||
</p>
|
||||
<sample src="examples/logInjectionBad.js" />
|
||||
|
||||
<p> In the second example, <code>String.prototype.replace</code> is used to ensure no line endings are present in the user input.</p>
|
||||
<sample src="examples/logInjectionGood.js" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -0,0 +1,20 @@
|
||||
/**
|
||||
* @name Log Injection
|
||||
* @description Building log entries from user-controlled sources is vulnerable to
|
||||
* insertion of forged log entries by a malicious user.
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @id js/log-injection
|
||||
* @tags security
|
||||
* external/cwe/cwe-117
|
||||
*/
|
||||
|
||||
import javascript
|
||||
import DataFlow::PathGraph
|
||||
import LogInjection::LogInjection
|
||||
|
||||
from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where config.hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
|
||||
"User-provided value"
|
||||
105
javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll
Normal file
105
javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll
Normal file
@@ -0,0 +1,105 @@
|
||||
/**
|
||||
* Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
|
||||
*/
|
||||
|
||||
import javascript
|
||||
|
||||
module LogInjection {
|
||||
/**
|
||||
* A data flow source for user input used in log entries.
|
||||
*/
|
||||
abstract class Source extends DataFlow::Node { }
|
||||
|
||||
/**
|
||||
* A data flow sink for user input used in log entries.
|
||||
*/
|
||||
abstract class Sink extends DataFlow::Node { }
|
||||
|
||||
/**
|
||||
* A sanitizer for malicious user input used in log entries.
|
||||
*/
|
||||
abstract class Sanitizer extends DataFlow::Node { }
|
||||
|
||||
/**
|
||||
* A taint-tracking configuration for untrusted user input used in log entries.
|
||||
*/
|
||||
class LogInjectionConfiguration extends TaintTracking::Configuration {
|
||||
LogInjectionConfiguration() { this = "LogInjection" }
|
||||
|
||||
override predicate isSource(DataFlow::Node source) { source instanceof Source }
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
|
||||
|
||||
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
|
||||
}
|
||||
|
||||
/**
|
||||
* A source of remote user controlled input.
|
||||
*/
|
||||
class RemoteSource extends Source {
|
||||
RemoteSource() { this instanceof RemoteFlowSource }
|
||||
}
|
||||
|
||||
/**
|
||||
* An source node representing a logging mechanism.
|
||||
*/
|
||||
class ConsoleSource extends DataFlow::SourceNode {
|
||||
ConsoleSource() {
|
||||
exists(DataFlow::SourceNode node |
|
||||
node = this and this = DataFlow::moduleImport("console")
|
||||
or
|
||||
this = DataFlow::globalVarRef("console")
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to a logging mechanism. For example, the call could be in the following forms:
|
||||
* `console.log('hello')` or
|
||||
*
|
||||
* `let logger = console.log; `
|
||||
* `logger('hello')` or
|
||||
*
|
||||
* `let logger = {info: console.log};`
|
||||
* `logger.info('hello')`
|
||||
*/
|
||||
class LoggingCall extends DataFlow::CallNode {
|
||||
LoggingCall() {
|
||||
this = any(ConsoleSource console).getAMemberCall(getAStandardLoggerMethodName())
|
||||
or
|
||||
exists(DataFlow::SourceNode node, string propName |
|
||||
any(ConsoleSource console).getAPropertyRead() = node.getAPropertySource(propName) and
|
||||
this = node.getAPropertyRead(propName).getACall()
|
||||
)
|
||||
or
|
||||
this = any(LoggerCall call)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* An argument to a logging mechanism.
|
||||
*/
|
||||
class LoggingSink extends Sink {
|
||||
LoggingSink() { this = any(LoggingCall console).getAnArgument() }
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `String.prototype.replace` that replaces `\n` is considered to sanitize the replaced string (reduce false positive).
|
||||
*/
|
||||
class StringReplaceSanitizer extends Sanitizer {
|
||||
StringReplaceSanitizer() {
|
||||
exists(StringReplaceCall replace, string s |
|
||||
replace.replaces(s, "") and s.regexpMatch("\\n")
|
||||
|
|
||||
this = replace
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to an HTML sanitizer is considered to sanitize the user input.
|
||||
*/
|
||||
class HtmlSanitizer extends Sanitizer {
|
||||
HtmlSanitizer() { this instanceof HtmlSanitizerCall }
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,68 @@
|
||||
const http = require('http');
|
||||
const hostname = '127.0.0.1';
|
||||
const port = 3000;
|
||||
const url = require('url');
|
||||
|
||||
|
||||
const check_username = (username) => {
|
||||
if (username != 'name') throw `${username} is not valid`;
|
||||
// do something
|
||||
}
|
||||
|
||||
const my_logger = {
|
||||
log: console.log
|
||||
}
|
||||
|
||||
const another_logger = console.log
|
||||
|
||||
// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
|
||||
|
||||
|
||||
|
||||
const server = http.createServer((req, res) => {
|
||||
let q = url.parse(req.url, true);
|
||||
|
||||
let username = q.query.username;
|
||||
|
||||
// BAD: User input logged as-is
|
||||
console.info(`[INFO] User: ${username}`);
|
||||
// [INFO] User: Guest
|
||||
// [INFO] User: Admin
|
||||
//
|
||||
|
||||
// BAD: User input logged as-is
|
||||
console.info(`[INFO] User: %s`, username);
|
||||
// [INFO] User: Guest
|
||||
// [INFO] User: Admin
|
||||
//
|
||||
|
||||
|
||||
// BAD: User input logged as-is
|
||||
my_logger.log('[INFO] User:', username);
|
||||
// [INFO] User: Guest
|
||||
// [INFO] User: Admin
|
||||
//
|
||||
|
||||
// BAD: User input logged as-is
|
||||
another_logger('[INFO] User:', username);
|
||||
// [INFO] User: Guest
|
||||
// [INFO] User: Admin
|
||||
//
|
||||
|
||||
try {
|
||||
check_username(username)
|
||||
|
||||
} catch (error) {
|
||||
// BAD: Error with user input logged as-is
|
||||
console.error(`[ERROR] Error: "${error}"`);
|
||||
// [ERROR] Error: "Guest
|
||||
// [INFO] User: Admin
|
||||
// is not valid"
|
||||
|
||||
}
|
||||
|
||||
})
|
||||
|
||||
server.listen(port, hostname, () => {
|
||||
console.log(`Server running at http://${hostname}:${port}/`);
|
||||
});
|
||||
@@ -0,0 +1,51 @@
|
||||
const http = require('http');
|
||||
const hostname = '127.0.0.1';
|
||||
const port = 3000;
|
||||
const url = require('url');
|
||||
|
||||
const check_username = (username) => {
|
||||
if (username != 'name') throw `${username} is not valid`;
|
||||
// do something
|
||||
}
|
||||
|
||||
const logger = {
|
||||
log: console.log
|
||||
}
|
||||
|
||||
const another_logger = console.log
|
||||
|
||||
// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
|
||||
|
||||
const server = http.createServer((req, res) => {
|
||||
let q = url.parse(req.url, true);
|
||||
|
||||
// GOOD: remove `\n` line from user controlled input before logging
|
||||
let username = q.query.username.replace(/\n/g, "");
|
||||
|
||||
console.info(`[INFO] User: ${username}`);
|
||||
// [INFO] User: Guest[INFO] User: Admin
|
||||
|
||||
console.info(`[INFO] User: %s`, username);
|
||||
// [INFO] User: Guest[INFO] User: Admin
|
||||
|
||||
logger.log('[INFO] User:', username);
|
||||
// [INFO] User: Guest[INFO] User: Admin
|
||||
|
||||
another_logger('[INFO] User:', username);
|
||||
// [INFO] User: Guest[INFO] User: Admin
|
||||
|
||||
try {
|
||||
check_username(username)
|
||||
|
||||
} catch (error) {
|
||||
console.error(`[ERROR] Error: "${error}"`);
|
||||
// [ERROR] Error: "Guest[INFO] User: Admin is not valid"
|
||||
|
||||
}
|
||||
|
||||
})
|
||||
|
||||
server.listen(port, hostname, () => {
|
||||
console.log(`Server running at http://${hostname}:${port}/`);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user