Skip to content

Commit 0f1106b

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #15753 from joefarebrother/ruby-i18n-translation
Ruby: Model Rails translation methods
2 parents e051526 + abdae2c commit 0f1106b

File tree

7 files changed

+121
-0
lines changed

7 files changed

+121
-0
lines changed

‎ruby/ql/lib/change-notes/2024-02-29-i18n-translate.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
Calls to `I18n.translate` as well as Rails helper translate methods now propagate taint from their keyword arguments. The Rails translate methods are also recognized as XSS sanitizers when using keys marked as html safe.

‎ruby/ql/lib/codeql/ruby/Frameworks.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ private import codeql.ruby.frameworks.Json
3030
private import codeql.ruby.frameworks.Erb
3131
private import codeql.ruby.frameworks.Slim
3232
private import codeql.ruby.frameworks.Sinatra
33+
private import codeql.ruby.frameworks.Translation
3334
private import codeql.ruby.frameworks.Twirp
3435
private import codeql.ruby.frameworks.Sqlite3
3536
private import codeql.ruby.frameworks.Mysql2

‎ruby/ql/lib/codeql/ruby/frameworks/Translation.qll

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/** Provides modeling for the `I18n` translation method and the rails translation helpers. */
2+
3+
private import codeql.ruby.ApiGraphs
4+
private import codeql.ruby.dataflow.FlowSummary
5+
private import codeql.ruby.Concepts
6+
private import codeql.ruby.frameworks.ActionView
7+
private import codeql.ruby.frameworks.ActionController
8+
9+
/** Gets a call to `I18n.translate`, which can use keyword arguments as inputs for string interpolation. */
10+
private MethodCall getI18nTranslateCall() {
11+
result = API::getTopLevelMember("I18n").getAMethodCall(["t", "translate"]).asExpr().getExpr()
12+
}
13+
14+
/**
15+
* Gets a call to the rails view helper translate method, which is a wrapper around `I18n.translate`
16+
* that can also escape html if the key ends in `_html` or `.html`.
17+
*/
18+
private MethodCall getViewHelperTranslateCall() {
19+
result.getMethodName() = ["t", "translate"] and
20+
inActionViewContext(result)
21+
}
22+
23+
/**
24+
* Gets a call to the rails controller helper translate method, which is a wrapper around `I18n.translate`
25+
* that can also escape html if the key ends in `_html` or `.html`.
26+
*/
27+
private MethodCall getControllerHelperTranslateCall() {
28+
result =
29+
any(ActionControllerClass c)
30+
.getSelf()
31+
.track()
32+
.getAMethodCall(["t", "translate"])
33+
.asExpr()
34+
.getExpr()
35+
}
36+
37+
/** Flow summary for translation methods. */
38+
private class TranslateSummary extends SummarizedCallable {
39+
TranslateSummary() { this = "I18n.translate" }
40+
41+
override MethodCall getACall() {
42+
result =
43+
[getI18nTranslateCall(), getViewHelperTranslateCall(), getControllerHelperTranslateCall()]
44+
}
45+
46+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
47+
input = "Argument[hash-splat].Element[any]" and
48+
output = "ReturnValue" and
49+
preservesValue = false
50+
}
51+
}
52+
53+
/** A call to a translation helper method that escapes HTML. */
54+
private class TranslateHtmlEscape extends Escaping::Range, DataFlow::CallNode {
55+
TranslateHtmlEscape() {
56+
exists(MethodCall mc | mc = this.asExpr().getExpr() |
57+
mc = [getViewHelperTranslateCall(), getControllerHelperTranslateCall()] and
58+
mc.getArgument(0).getConstantValue().getStringlikeValue().matches(["%\\_html", "%.html"])
59+
)
60+
}
61+
62+
override string getKind() { result = Escaping::getHtmlKind() }
63+
64+
override DataFlow::Node getAnInput() { result = this.getKeywordArgument(_) }
65+
66+
override DataFlow::Node getOutput() { result = this }
67+
}

‎ruby/ql/test/library-tests/dataflow/local/TaintStep.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2798,6 +2798,7 @@
27982798
| UseUseExplosion.rb:21:3691:21:3696 | call to use | UseUseExplosion.rb:21:3686:21:3696 | else ... |
27992799
| UseUseExplosion.rb:24:5:25:7 | synthetic splat parameter | UseUseExplosion.rb:24:13:24:13 | i |
28002800
| UseUseExplosion.rb:24:5:25:7 | use | UseUseExplosion.rb:1:1:26:3 | C |
2801+
| file://:0:0:0:0 | [summary param] ** in I18n.translate | file://:0:0:0:0 | [summary] read: Argument[hash-splat].Element[any] in I18n.translate |
28012802
| file://:0:0:0:0 | [summary param] position 0 in & | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in & |
28022803
| file://:0:0:0:0 | [summary param] position 0 in + | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in + |
28032804
| file://:0:0:0:0 | [summary param] position 0 in ActionController::Parameters#merge | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge |
@@ -2840,6 +2841,7 @@
28402841
| file://:0:0:0:0 | [summary param] self in assoc-unknown-arg | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in assoc-unknown-arg |
28412842
| file://:0:0:0:0 | [summary param] self in each(0) | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in each(0) |
28422843
| file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in Hash[] | file://:0:0:0:0 | [summary] read: Argument[0].Element[any].Element[1] in Hash[] |
2844+
| file://:0:0:0:0 | [summary] read: Argument[hash-splat].Element[any] in I18n.translate | file://:0:0:0:0 | [summary] to write: ReturnValue in I18n.translate |
28432845
| local_dataflow.rb:1:1:7:3 | self (foo) | local_dataflow.rb:3:8:3:10 | self |
28442846
| local_dataflow.rb:1:1:7:3 | self in foo | local_dataflow.rb:1:1:7:3 | self (foo) |
28452847
| local_dataflow.rb:1:1:7:3 | synthetic splat parameter | local_dataflow.rb:1:9:1:9 | a |

‎ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,20 @@ edges
2222
| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:35:3:35:14 | call to display_text | provenance | |
2323
| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text | provenance | |
2424
| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | provenance | |
25+
| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | provenance | |
26+
| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | provenance | |
2527
| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | provenance | |
2628
| app/controllers/foo/bars_controller.rb:30:5:30:7 | str | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | provenance | |
2729
| app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | provenance | |
2830
| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | app/controllers/foo/bars_controller.rb:30:5:30:7 | str | provenance | |
31+
| app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | provenance | |
32+
| app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | provenance | |
33+
| app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | provenance | |
34+
| app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | provenance | |
35+
| app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | provenance | |
36+
| app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | provenance | |
37+
| app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | provenance | |
38+
| app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | provenance | |
2939
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text [element] | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | provenance | |
3040
| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text, element] | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] [element] | provenance | |
3141
| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | provenance | |
@@ -45,6 +55,8 @@ edges
4555
| app/views/foo/bars/show.html.erb:56:13:56:18 | call to params | app/views/foo/bars/show.html.erb:56:13:56:28 | ...[...] | provenance | |
4656
| app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | provenance | |
4757
| app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | provenance | |
58+
| app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | provenance | |
59+
| app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | provenance | |
4860
nodes
4961
| app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params | semmle.label | call to params |
5062
| app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] | semmle.label | ...[...] |
@@ -66,6 +78,18 @@ nodes
6678
| app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | semmle.label | call to params |
6779
| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | semmle.label | ...[...] |
6880
| app/controllers/foo/bars_controller.rb:31:5:31:7 | str | semmle.label | str |
81+
| app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | semmle.label | call to translate |
82+
| app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | semmle.label | call to params |
83+
| app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | semmle.label | ...[...] |
84+
| app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | semmle.label | call to t |
85+
| app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | semmle.label | call to params |
86+
| app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | semmle.label | ...[...] |
87+
| app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | semmle.label | call to t |
88+
| app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | semmle.label | call to params |
89+
| app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | semmle.label | ...[...] |
90+
| app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | semmle.label | call to translate |
91+
| app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | semmle.label | call to params |
92+
| app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | semmle.label | ...[...] |
6993
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
7094
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text [element] | semmle.label | call to display_text [element] |
7195
| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text, element] | semmle.label | call to local_assigns [element :display_text, element] |
@@ -98,10 +122,18 @@ nodes
98122
| app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | semmle.label | call to params |
99123
| app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | semmle.label | ...[...] |
100124
| app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | semmle.label | call to display_text |
125+
| app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | semmle.label | call to translate |
126+
| app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | semmle.label | call to display_text |
127+
| app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | semmle.label | call to t |
128+
| app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | semmle.label | call to display_text |
101129
subpaths
102130
#select
103131
| app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | user-provided value |
104132
| app/controllers/foo/bars_controller.rb:31:5:31:7 | str | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | user-provided value |
133+
| app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | user-provided value |
134+
| app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | user-provided value |
135+
| app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | user-provided value |
136+
| app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | user-provided value |
105137
| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
106138
| app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
107139
| app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | user-provided value |
@@ -118,3 +150,5 @@ subpaths
118150
| app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | user-provided value |
119151
| app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | user-provided value |
120152
| app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
153+
| app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
154+
| app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |

‎ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,5 +29,11 @@ def show
2929
def make_safe_html
3030
str = params[:user_name]
3131
str.html_safe
32+
33+
translate("welcome", name: params[:user_name]).html_safe # NOT OK - translate preserves taint
34+
t("welcome", name: params[:user_name]).html_safe # NOT OK - t is an alias of translate
35+
t("welcome_html", name: params[:user_name]).html_safe # OK - t escapes html when key ends in _html
36+
I18n.t("welcome_html", name: params[:user_name]).html_safe # NOT OK - I18n.t does not escape html
37+
I18n.translate("welcome_html", name: params[:user_name]).html_safe # NOT OK - alias
3238
end
3339
end

‎ruby/ql/test/query-tests/security/cwe-079/app/views/foo/bars/show.html.erb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,3 +80,10 @@
8080

8181
<%# BAD: A local rendered raw as a local variable %>
8282
<%== display_text %>
83+
84+
<%# BAD: translate preserves taint %>
85+
<%= raw translate("welcome", name: display_text) %>
86+
<%= raw t("welcome", name: display_text) %>
87+
88+
<%# GOOD: translate sanitizes for html keys %>
89+
<%= raw t("welcome1.html", name: display_text) %>

0 commit comments

Comments
 (0)