Skip to content

Commit d4d6657

Browse files
authored
Merge pull request #10008 from alexrford/rb/log-injection
Ruby: Add `rb/log-injection` query
2 parents 1eb0f6a + 7a61f59 commit d4d6657

File tree

9 files changed

+264
-0
lines changed

9 files changed

+264
-0
lines changed
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
3+
*/
4+
5+
import ruby
6+
import codeql.ruby.Concepts
7+
import codeql.ruby.DataFlow
8+
import codeql.ruby.TaintTracking
9+
import codeql.ruby.dataflow.RemoteFlowSources
10+
import codeql.ruby.frameworks.Core
11+
12+
/**
13+
* A data flow source for user input used in log entries.
14+
*/
15+
abstract class Source extends DataFlow::Node { }
16+
17+
/**
18+
* A data flow sink for user input used in log entries.
19+
*/
20+
abstract class Sink extends DataFlow::Node { }
21+
22+
/**
23+
* A sanitizer for malicious user input used in log entries.
24+
*/
25+
abstract class Sanitizer extends DataFlow::Node { }
26+
27+
/**
28+
* A taint-tracking configuration for untrusted user input used in log entries.
29+
*/
30+
class LogInjectionConfiguration extends TaintTracking::Configuration {
31+
LogInjectionConfiguration() { this = "LogInjection" }
32+
33+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
34+
35+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
36+
37+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
38+
}
39+
40+
/**
41+
* A source of remote user controlled input.
42+
*/
43+
class RemoteSource extends Source instanceof RemoteFlowSource { }
44+
45+
/**
46+
* An input to a logging mechanism.
47+
*/
48+
class LoggingSink extends Sink {
49+
LoggingSink() { this = any(Logging logging).getAnInput() }
50+
}
51+
52+
/**
53+
* A call to `String#replace` that replaces `\n` is considered to sanitize the replaced string (reduce false positive).
54+
*/
55+
class StringReplaceSanitizer extends Sanitizer {
56+
StringReplaceSanitizer() {
57+
exists(string s | this.(StringSubstitutionCall).replaces(s, "") and s.regexpMatch("\\n")) and
58+
// exclude replacement methods that may not fully sanitize the string
59+
this.(StringSubstitutionCall).isGlobal()
60+
}
61+
}
62+
63+
/**
64+
* A call to an HTML escape method is considered to sanitize its input.
65+
*/
66+
class HtmlEscapingAsSanitizer extends Sanitizer {
67+
HtmlEscapingAsSanitizer() { this = any(HtmlEscaping esc).getOutput() }
68+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/log-inection`, to detect cases where a malicious user may be able to forge log entries.
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
If unsanitized user input is written to a log entry, a malicious user may
9+
able to forge new log entries.
10+
</p>
11+
12+
<p>
13+
Forgery can occur if a user provides some input with characters that are
14+
interpreted when the log output is displayed. If the log is displayed as a plain
15+
text file, then new line characters can be used by a malicious user. If the log
16+
is displayed as HTML, then arbitrary HTML may be included to spoof log entries.
17+
</p>
18+
</overview>
19+
20+
<recommendation>
21+
<p>
22+
User input should be suitably sanitized before it is logged. Suitable means of
23+
sanitization depend on how the log entries will be displayed or consumed.
24+
</p>
25+
26+
<p>
27+
If the log entries are in plain text then line breaks should be removed from
28+
user input, using <code>String#gsub</code> or similar. Care should also be
29+
taken that user input is clearly marked in log entries.
30+
</p>
31+
32+
<p>
33+
For log entries that will be displayed in HTML, user input should be
34+
HTML-encoded before being logged, to prevent forgery and other forms of HTML
35+
injection.
36+
</p>
37+
</recommendation>
38+
39+
<example>
40+
<p>
41+
In the example, a username, provided by the user, is logged using `Logger#info`.
42+
</p>
43+
44+
<p>
45+
In the first case, it is logged without any sanitization. If a malicious user
46+
provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, the
47+
log entry will be split in two different lines, where the second line will
48+
be `[INFO]+User:+Admin`.
49+
</p>
50+
<sample src="examples/log_injection_bad.rb" />
51+
52+
<p>
53+
In the second example, <code>String#gsub</code> is used to ensure no line
54+
endings are present in the user input.
55+
</p>
56+
<sample src="examples/log_injection_good.rb" />
57+
</example>
58+
59+
<references>
60+
<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
61+
</references>
62+
</qhelp>
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
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+
* @security-severity 7.8
8+
* @precision medium
9+
* @id rb/log-injection
10+
* @tags security
11+
* external/cwe/cwe-117
12+
*/
13+
14+
import ruby
15+
import DataFlow::PathGraph
16+
import codeql.ruby.security.LogInjectionQuery
17+
18+
from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where config.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
21+
"User-provided value"
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
require 'logger'
2+
3+
class UsersController < ApplicationController
4+
def login
5+
logger = Logger.new STDOUT
6+
username = params[:username]
7+
8+
# BAD: log message constructed with unsanitized user input
9+
logger.info "attempting to login user: " + username
10+
11+
# ... login logic ...
12+
end
13+
end
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
require 'logger'
2+
3+
class UsersController < ApplicationController
4+
def login
5+
logger = Logger.new STDOUT
6+
username = params[:username]
7+
8+
# GOOD: log message constructed with unsanitized user input
9+
sanitized_username = username.gsub("\n", "")
10+
logger.info "attempting to login user: " + sanitized_username
11+
12+
# ... login logic ...
13+
end
14+
end
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
edges
2+
| app/controllers/users_controller.rb:15:19:15:24 | call to params : | app/controllers/users_controller.rb:15:19:15:30 | ...[...] : |
3+
| app/controllers/users_controller.rb:15:19:15:30 | ...[...] : | app/controllers/users_controller.rb:16:19:16:29 | unsanitized |
4+
| app/controllers/users_controller.rb:15:19:15:30 | ...[...] : | app/controllers/users_controller.rb:17:19:17:41 | ... + ... |
5+
| app/controllers/users_controller.rb:15:19:15:30 | ...[...] : | app/controllers/users_controller.rb:23:20:23:30 | unsanitized : |
6+
| app/controllers/users_controller.rb:23:5:23:44 | ... = ... : | app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 |
7+
| app/controllers/users_controller.rb:23:20:23:30 | unsanitized : | app/controllers/users_controller.rb:23:20:23:44 | call to sub : |
8+
| app/controllers/users_controller.rb:23:20:23:44 | call to sub : | app/controllers/users_controller.rb:23:5:23:44 | ... = ... : |
9+
| app/controllers/users_controller.rb:23:20:23:44 | call to sub : | app/controllers/users_controller.rb:27:16:27:39 | ... + ... |
10+
| app/controllers/users_controller.rb:33:5:33:31 | ... = ... : | app/controllers/users_controller.rb:34:33:34:43 | unsanitized |
11+
| app/controllers/users_controller.rb:33:5:33:31 | ... = ... : | app/controllers/users_controller.rb:35:33:35:55 | ... + ... |
12+
| app/controllers/users_controller.rb:33:19:33:25 | call to cookies : | app/controllers/users_controller.rb:33:19:33:31 | ...[...] : |
13+
| app/controllers/users_controller.rb:33:19:33:31 | ...[...] : | app/controllers/users_controller.rb:33:5:33:31 | ... = ... : |
14+
nodes
15+
| app/controllers/users_controller.rb:15:19:15:24 | call to params : | semmle.label | call to params : |
16+
| app/controllers/users_controller.rb:15:19:15:30 | ...[...] : | semmle.label | ...[...] : |
17+
| app/controllers/users_controller.rb:16:19:16:29 | unsanitized | semmle.label | unsanitized |
18+
| app/controllers/users_controller.rb:17:19:17:41 | ... + ... | semmle.label | ... + ... |
19+
| app/controllers/users_controller.rb:23:5:23:44 | ... = ... : | semmle.label | ... = ... : |
20+
| app/controllers/users_controller.rb:23:20:23:30 | unsanitized : | semmle.label | unsanitized : |
21+
| app/controllers/users_controller.rb:23:20:23:44 | call to sub : | semmle.label | call to sub : |
22+
| app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | semmle.label | unsanitized2 |
23+
| app/controllers/users_controller.rb:27:16:27:39 | ... + ... | semmle.label | ... + ... |
24+
| app/controllers/users_controller.rb:33:5:33:31 | ... = ... : | semmle.label | ... = ... : |
25+
| app/controllers/users_controller.rb:33:19:33:25 | call to cookies : | semmle.label | call to cookies : |
26+
| app/controllers/users_controller.rb:33:19:33:31 | ...[...] : | semmle.label | ...[...] : |
27+
| app/controllers/users_controller.rb:34:33:34:43 | unsanitized | semmle.label | unsanitized |
28+
| app/controllers/users_controller.rb:35:33:35:55 | ... + ... | semmle.label | ... + ... |
29+
subpaths
30+
#select
31+
| app/controllers/users_controller.rb:16:19:16:29 | unsanitized | app/controllers/users_controller.rb:15:19:15:24 | call to params : | app/controllers/users_controller.rb:16:19:16:29 | unsanitized | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
32+
| app/controllers/users_controller.rb:17:19:17:41 | ... + ... | app/controllers/users_controller.rb:15:19:15:24 | call to params : | app/controllers/users_controller.rb:17:19:17:41 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
33+
| app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | app/controllers/users_controller.rb:15:19:15:24 | call to params : | app/controllers/users_controller.rb:25:7:25:18 | unsanitized2 | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
34+
| app/controllers/users_controller.rb:27:16:27:39 | ... + ... | app/controllers/users_controller.rb:15:19:15:24 | call to params : | app/controllers/users_controller.rb:27:16:27:39 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:15:19:15:24 | call to params | User-provided value |
35+
| app/controllers/users_controller.rb:34:33:34:43 | unsanitized | app/controllers/users_controller.rb:33:19:33:25 | call to cookies : | app/controllers/users_controller.rb:34:33:34:43 | unsanitized | $@ flows to log entry. | app/controllers/users_controller.rb:33:19:33:25 | call to cookies | User-provided value |
36+
| app/controllers/users_controller.rb:35:33:35:55 | ... + ... | app/controllers/users_controller.rb:33:19:33:25 | call to cookies : | app/controllers/users_controller.rb:35:33:35:55 | ... + ... | $@ flows to log entry. | app/controllers/users_controller.rb:33:19:33:25 | call to cookies | User-provided value |
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-117/LogInjection.ql
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
require 'logger'
2+
3+
class UsersController < ApplicationController
4+
include ERB::Util
5+
6+
def init_logger
7+
if @logger == nil
8+
@logger = Logger.new STDOUT
9+
end
10+
end
11+
12+
def read_from_params
13+
init_logger
14+
15+
unsanitized = params[:foo]
16+
@logger.debug unsanitized # BAD: unsanitized user input
17+
@logger.error "input: " + unsanitized # BAD: unsanitized user input
18+
19+
sanitized = unsanitized.gsub("\n", "")
20+
@logger.fatal sanitized # GOOD: sanitized user input
21+
@logger.warn "input: " + sanitized # GOOD: sanitized user input
22+
23+
unsanitized2 = unsanitized.sub("\n", "")
24+
@logger.info do
25+
unsanitized2 # BAD: partially sanitized user input
26+
end
27+
@logger << "input: " + unsanitized2 # BAD: partially sanitized user input
28+
end
29+
30+
def read_from_cookies
31+
init_logger
32+
33+
unsanitized = cookies[:bar]
34+
@logger.add(Logger::INFO) { unsanitized } # BAD: unsanitized user input
35+
@logger.log(Logger::WARN) { "input: " + unsanitized } # BAD: unsanitized user input
36+
end
37+
38+
def html_sanitization
39+
init_logger
40+
41+
sanitized = html_escape params[:baz]
42+
@logger.debug unsanitized # GOOD: sanitized user input
43+
@logger.debug "input: " + unsanitized # GOOD: sanitized user input
44+
end
45+
end

0 commit comments

Comments
 (0)