Skip to content

Commit

Permalink
Merge pull request #15907 from joefarebrother/ruby-uploaded-file
Browse files Browse the repository at this point in the history 
Ruby: Model ActiveDispatch::Http::UploadedFile
  • Loading branch information
@joefarebrother
joefarebrother authored Mar 18, 2024
2 parents dbf1682 + 8c5fff2 commit 4177c38
Show file tree
Hide file tree
Showing 9 changed files with 197 additions and 2 deletions.
4 changes: 4 additions & 0 deletions ruby/ql/lib/change-notes/2024-03-14-actiondispatch-uploadedfile.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Modeled instances of `ActionDispatch::Http::UploadedFile` that can be obtained from element reads of `ActionController::Parameters`, with calls to `original_filename`, `content_type`, and `read` now propagating taint from their receiver.
6 changes: 5 additions & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,11 @@ private class Argument extends CfgNodes::ExprCfgNode {

/** Holds if `n` is not a constant expression. */
predicate isNonConstantExpr(CfgNodes::ExprCfgNode n) {
not exists(n.getConstantValue()) and
not exists(ConstantValue cv |
cv = n.getConstantValue() and
// strings are mutable in Ruby
not cv.isString(_)
) and
not n.getExpr() instanceof ConstantAccess
}

Expand Down
64 changes: 64 additions & 0 deletions ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.DataFlow
private import codeql.ruby.dataflow.RemoteFlowSources
private import codeql.ruby.ApiGraphs
private import codeql.ruby.typetracking.TypeTracking
private import codeql.ruby.frameworks.ActionDispatch
private import codeql.ruby.frameworks.ActionView
private import codeql.ruby.frameworks.Rails
Expand Down Expand Up @@ -505,6 +506,27 @@ private module ParamsSummaries {
]
}

/** Gets a node that may be tainted from an `ActionController::Parameters` instance, through field accesses and hash/array element reads. */
private DataFlow::LocalSourceNode taintFromParamsBase() {
result =
[
paramsInstance(),
paramsInstance().getAMethodCall(methodReturnsTaintFromSelf()).getAnElementRead*()
]
}

private DataFlow::LocalSourceNode taintFromParamsType(TypeTracker t) {
t.start() and
result = taintFromParamsBase()
or
exists(TypeTracker t2 | result = taintFromParamsType(t2).track(t2, t))
}

/** Gets a node with a type that may be tainted from an `ActionController::Parameters` instance. */
private DataFlow::LocalSourceNode taintFromParamsType() {
taintFromParamsType(TypeTracker::end()).flowsTo(result)
}

/**
* A flow summary for methods on `ActionController::Parameters` which
* propagate taint from receiver to return value.
Expand Down Expand Up @@ -569,6 +591,48 @@ private module ParamsSummaries {
preservesValue = false
}
}

/** Flow summaries for `ActiveDispatch::Http::UploadedFile`, which can be an field of `ActionController::Parameters`. */
module UploadedFileSummaries {
/** Flow summary for various string attributes of `UploadedFile`, including `original_filename`, `content_type`, and `headers`. */
private class UploadedFileStringAttributeSummary extends SummarizedCallable {
UploadedFileStringAttributeSummary() {
this = "ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers]"
}

override MethodCall getACall() {
result =
taintFromParamsType()
.getAMethodCall(["original_filename", "content_type", "headers"])
.asExpr()
.getExpr() and
result.getNumberOfArguments() = 0
}

override predicate propagatesFlow(string input, string output, boolean preservesValue) {
input = "Argument[self]" and output = "ReturnValue" and preservesValue = false
}
}

/**
* Flow summary for `ActiveDispatch::Http::UploadedFile#read`,
* which propagates taint from the receiver to the return value or to the second (out string) argument
*/
private class UploadedFileReadSummary extends SummarizedCallable {
UploadedFileReadSummary() { this = "ActionDispatch::Http::UploadedFile#read" }

override MethodCall getACall() {
result = taintFromParamsType().getAMethodCall("read").asExpr().getExpr() and
result.getNumberOfArguments() in [0 .. 2]
}

override predicate propagatesFlow(string input, string output, boolean preservesValue) {
input = "Argument[self]" and
output = ["ReturnValue", "Argument[1]"] and
preservesValue = false
}
}
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/test/library-tests/dataflow/global/instance_variables.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def initialize x
foo3.set_field(taint(22))
sink(foo3.field) # $ hasValueFlow=22

foo4 = "hello"
foo4 = 4
foo4.other = taint(23)
sink(foo4.other) # no field flow for constants

Expand Down
9 changes: 9 additions & 0 deletions ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2676,6 +2676,7 @@
| local_dataflow.rb:131:7:131:8 | "" | local_dataflow.rb:131:3:131:8 | ... = ... |
| local_dataflow.rb:132:6:132:11 | [post] self | local_dataflow.rb:133:8:133:13 | self |
| local_dataflow.rb:132:6:132:11 | self | local_dataflow.rb:133:8:133:13 | self |
| local_dataflow.rb:132:10:132:10 | [post] x | local_dataflow.rb:133:12:133:12 | x |
| local_dataflow.rb:132:10:132:10 | x | local_dataflow.rb:133:12:133:12 | x |
| local_dataflow.rb:132:12:148:10 | then ... | local_dataflow.rb:132:3:149:5 | if ... |
| local_dataflow.rb:133:5:139:7 | SSA phi read(self) | local_dataflow.rb:141:9:141:14 | self |
Expand All @@ -2686,17 +2687,20 @@
| local_dataflow.rb:133:8:133:13 | self | local_dataflow.rb:133:18:133:23 | self |
| local_dataflow.rb:133:8:133:23 | SSA phi read(self) | local_dataflow.rb:134:7:134:12 | self |
| local_dataflow.rb:133:8:133:23 | SSA phi read(x) | local_dataflow.rb:134:11:134:11 | x |
| local_dataflow.rb:133:12:133:12 | [post] x | local_dataflow.rb:133:22:133:22 | x |
| local_dataflow.rb:133:12:133:12 | x | local_dataflow.rb:133:22:133:22 | x |
| local_dataflow.rb:133:18:133:23 | [post] self | local_dataflow.rb:136:7:136:12 | self |
| local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [false] ... \|\| ... |
| local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [true] ... \|\| ... |
| local_dataflow.rb:133:18:133:23 | self | local_dataflow.rb:136:7:136:12 | self |
| local_dataflow.rb:133:22:133:22 | [post] x | local_dataflow.rb:136:11:136:11 | x |
| local_dataflow.rb:133:22:133:22 | x | local_dataflow.rb:136:11:136:11 | x |
| local_dataflow.rb:133:24:134:12 | then ... | local_dataflow.rb:133:5:139:7 | if ... |
| local_dataflow.rb:134:7:134:12 | call to use | local_dataflow.rb:133:24:134:12 | then ... |
| local_dataflow.rb:135:5:138:9 | else ... | local_dataflow.rb:133:5:139:7 | if ... |
| local_dataflow.rb:136:7:136:12 | [post] self | local_dataflow.rb:137:10:137:15 | self |
| local_dataflow.rb:136:7:136:12 | self | local_dataflow.rb:137:10:137:15 | self |
| local_dataflow.rb:136:11:136:11 | [post] x | local_dataflow.rb:137:14:137:14 | x |
| local_dataflow.rb:136:11:136:11 | x | local_dataflow.rb:137:14:137:14 | x |
| local_dataflow.rb:137:7:138:9 | SSA phi read(self) | local_dataflow.rb:133:5:139:7 | SSA phi read(self) |
| local_dataflow.rb:137:7:138:9 | SSA phi read(x) | local_dataflow.rb:133:5:139:7 | SSA phi read(x) |
Expand All @@ -2705,6 +2709,7 @@
| local_dataflow.rb:137:10:137:15 | self | local_dataflow.rb:137:21:137:26 | self |
| local_dataflow.rb:137:10:137:26 | SSA phi read(self) | local_dataflow.rb:137:7:138:9 | SSA phi read(self) |
| local_dataflow.rb:137:10:137:26 | SSA phi read(x) | local_dataflow.rb:137:7:138:9 | SSA phi read(x) |
| local_dataflow.rb:137:14:137:14 | [post] x | local_dataflow.rb:137:25:137:25 | x |
| local_dataflow.rb:137:14:137:14 | x | local_dataflow.rb:137:25:137:25 | x |
| local_dataflow.rb:137:20:137:26 | [false] ! ... | local_dataflow.rb:137:10:137:26 | [false] ... && ... |
| local_dataflow.rb:137:20:137:26 | [true] ! ... | local_dataflow.rb:137:10:137:26 | [true] ... && ... |
Expand All @@ -2717,6 +2722,7 @@
| local_dataflow.rb:141:8:141:37 | SSA phi read(x) | local_dataflow.rb:141:5:145:7 | SSA phi read(x) |
| local_dataflow.rb:141:9:141:14 | [post] self | local_dataflow.rb:141:20:141:25 | self |
| local_dataflow.rb:141:9:141:14 | self | local_dataflow.rb:141:20:141:25 | self |
| local_dataflow.rb:141:13:141:13 | [post] x | local_dataflow.rb:141:24:141:24 | x |
| local_dataflow.rb:141:13:141:13 | x | local_dataflow.rb:141:24:141:24 | x |
| local_dataflow.rb:141:19:141:37 | [false] ( ... ) | local_dataflow.rb:141:8:141:37 | [false] ... \|\| ... |
| local_dataflow.rb:141:19:141:37 | [true] ( ... ) | local_dataflow.rb:141:8:141:37 | [true] ... \|\| ... |
Expand All @@ -2726,6 +2732,7 @@
| local_dataflow.rb:141:20:141:36 | SSA phi read(x) | local_dataflow.rb:143:15:143:15 | x |
| local_dataflow.rb:141:20:141:36 | [false] ... && ... | local_dataflow.rb:141:19:141:37 | [false] ( ... ) |
| local_dataflow.rb:141:20:141:36 | [true] ... && ... | local_dataflow.rb:141:19:141:37 | [true] ( ... ) |
| local_dataflow.rb:141:24:141:24 | [post] x | local_dataflow.rb:141:35:141:35 | x |
| local_dataflow.rb:141:24:141:24 | x | local_dataflow.rb:141:35:141:35 | x |
| local_dataflow.rb:141:30:141:36 | [false] ! ... | local_dataflow.rb:141:20:141:36 | [false] ... && ... |
| local_dataflow.rb:141:30:141:36 | [true] ! ... | local_dataflow.rb:141:20:141:36 | [true] ... && ... |
Expand All @@ -2740,12 +2747,14 @@
| local_dataflow.rb:143:11:143:16 | self | local_dataflow.rb:143:21:143:26 | self |
| local_dataflow.rb:143:11:143:26 | SSA phi read(self) | local_dataflow.rb:144:11:144:16 | self |
| local_dataflow.rb:143:11:143:26 | SSA phi read(x) | local_dataflow.rb:144:15:144:15 | x |
| local_dataflow.rb:143:15:143:15 | [post] x | local_dataflow.rb:143:25:143:25 | x |
| local_dataflow.rb:143:15:143:15 | x | local_dataflow.rb:143:25:143:25 | x |
| local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [false] ... \|\| ... |
| local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [true] ... \|\| ... |
| local_dataflow.rb:143:27:144:16 | then ... | local_dataflow.rb:143:5:144:16 | elsif ... |
| local_dataflow.rb:144:11:144:16 | call to use | local_dataflow.rb:143:27:144:16 | then ... |
| local_dataflow.rb:147:5:147:10 | [post] self | local_dataflow.rb:148:5:148:10 | self |
| local_dataflow.rb:147:5:147:10 | self | local_dataflow.rb:148:5:148:10 | self |
| local_dataflow.rb:147:9:147:9 | [post] x | local_dataflow.rb:148:9:148:9 | x |
| local_dataflow.rb:147:9:147:9 | x | local_dataflow.rb:148:9:148:9 | x |
| local_dataflow.rb:148:5:148:10 | call to use | local_dataflow.rb:132:12:148:10 | then ... |
12 changes: 12 additions & 0 deletions ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2835,6 +2835,9 @@
| file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge |
| file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: Argument[self] in ActionController::Parameters#merge! |
| file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge! |
| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers] | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers] |
| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#read | file://:0:0:0:0 | [summary] to write: Argument[1] in ActionDispatch::Http::UploadedFile#read |
| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#read | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile#read |
| file://:0:0:0:0 | [summary param] self in ActiveSupportStringTransform | file://:0:0:0:0 | [summary] to write: ReturnValue in ActiveSupportStringTransform |
| file://:0:0:0:0 | [summary param] self in [] | file://:0:0:0:0 | [summary] to write: ReturnValue in [] |
| file://:0:0:0:0 | [summary param] self in \| | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in \| |
Expand Down Expand Up @@ -3164,6 +3167,7 @@
| local_dataflow.rb:131:7:131:8 | "" | local_dataflow.rb:131:3:131:8 | ... = ... |
| local_dataflow.rb:132:6:132:11 | [post] self | local_dataflow.rb:133:8:133:13 | self |
| local_dataflow.rb:132:6:132:11 | self | local_dataflow.rb:133:8:133:13 | self |
| local_dataflow.rb:132:10:132:10 | [post] x | local_dataflow.rb:133:12:133:12 | x |
| local_dataflow.rb:132:10:132:10 | x | local_dataflow.rb:133:12:133:12 | x |
| local_dataflow.rb:132:12:148:10 | then ... | local_dataflow.rb:132:3:149:5 | if ... |
| local_dataflow.rb:133:5:139:7 | SSA phi read(self) | local_dataflow.rb:141:9:141:14 | self |
Expand All @@ -3174,17 +3178,20 @@
| local_dataflow.rb:133:8:133:13 | self | local_dataflow.rb:133:18:133:23 | self |
| local_dataflow.rb:133:8:133:23 | SSA phi read(self) | local_dataflow.rb:134:7:134:12 | self |
| local_dataflow.rb:133:8:133:23 | SSA phi read(x) | local_dataflow.rb:134:11:134:11 | x |
| local_dataflow.rb:133:12:133:12 | [post] x | local_dataflow.rb:133:22:133:22 | x |
| local_dataflow.rb:133:12:133:12 | x | local_dataflow.rb:133:22:133:22 | x |
| local_dataflow.rb:133:18:133:23 | [post] self | local_dataflow.rb:136:7:136:12 | self |
| local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [false] ... \|\| ... |
| local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [true] ... \|\| ... |
| local_dataflow.rb:133:18:133:23 | self | local_dataflow.rb:136:7:136:12 | self |
| local_dataflow.rb:133:22:133:22 | [post] x | local_dataflow.rb:136:11:136:11 | x |
| local_dataflow.rb:133:22:133:22 | x | local_dataflow.rb:136:11:136:11 | x |
| local_dataflow.rb:133:24:134:12 | then ... | local_dataflow.rb:133:5:139:7 | if ... |
| local_dataflow.rb:134:7:134:12 | call to use | local_dataflow.rb:133:24:134:12 | then ... |
| local_dataflow.rb:135:5:138:9 | else ... | local_dataflow.rb:133:5:139:7 | if ... |
| local_dataflow.rb:136:7:136:12 | [post] self | local_dataflow.rb:137:10:137:15 | self |
| local_dataflow.rb:136:7:136:12 | self | local_dataflow.rb:137:10:137:15 | self |
| local_dataflow.rb:136:11:136:11 | [post] x | local_dataflow.rb:137:14:137:14 | x |
| local_dataflow.rb:136:11:136:11 | x | local_dataflow.rb:137:14:137:14 | x |
| local_dataflow.rb:137:7:138:9 | SSA phi read(self) | local_dataflow.rb:133:5:139:7 | SSA phi read(self) |
| local_dataflow.rb:137:7:138:9 | SSA phi read(x) | local_dataflow.rb:133:5:139:7 | SSA phi read(x) |
Expand All @@ -3193,6 +3200,7 @@
| local_dataflow.rb:137:10:137:15 | self | local_dataflow.rb:137:21:137:26 | self |
| local_dataflow.rb:137:10:137:26 | SSA phi read(self) | local_dataflow.rb:137:7:138:9 | SSA phi read(self) |
| local_dataflow.rb:137:10:137:26 | SSA phi read(x) | local_dataflow.rb:137:7:138:9 | SSA phi read(x) |
| local_dataflow.rb:137:14:137:14 | [post] x | local_dataflow.rb:137:25:137:25 | x |
| local_dataflow.rb:137:14:137:14 | x | local_dataflow.rb:137:25:137:25 | x |
| local_dataflow.rb:137:20:137:26 | [false] ! ... | local_dataflow.rb:137:10:137:26 | [false] ... && ... |
| local_dataflow.rb:137:20:137:26 | [true] ! ... | local_dataflow.rb:137:10:137:26 | [true] ... && ... |
Expand All @@ -3209,6 +3217,7 @@
| local_dataflow.rb:141:9:141:14 | call to use | local_dataflow.rb:141:8:141:14 | [false] ! ... |
| local_dataflow.rb:141:9:141:14 | call to use | local_dataflow.rb:141:8:141:14 | [true] ! ... |
| local_dataflow.rb:141:9:141:14 | self | local_dataflow.rb:141:20:141:25 | self |
| local_dataflow.rb:141:13:141:13 | [post] x | local_dataflow.rb:141:24:141:24 | x |
| local_dataflow.rb:141:13:141:13 | x | local_dataflow.rb:141:24:141:24 | x |
| local_dataflow.rb:141:19:141:37 | [false] ( ... ) | local_dataflow.rb:141:8:141:37 | [false] ... \|\| ... |
| local_dataflow.rb:141:19:141:37 | [true] ( ... ) | local_dataflow.rb:141:8:141:37 | [true] ... \|\| ... |
Expand All @@ -3218,6 +3227,7 @@
| local_dataflow.rb:141:20:141:36 | SSA phi read(x) | local_dataflow.rb:143:15:143:15 | x |
| local_dataflow.rb:141:20:141:36 | [false] ... && ... | local_dataflow.rb:141:19:141:37 | [false] ( ... ) |
| local_dataflow.rb:141:20:141:36 | [true] ... && ... | local_dataflow.rb:141:19:141:37 | [true] ( ... ) |
| local_dataflow.rb:141:24:141:24 | [post] x | local_dataflow.rb:141:35:141:35 | x |
| local_dataflow.rb:141:24:141:24 | x | local_dataflow.rb:141:35:141:35 | x |
| local_dataflow.rb:141:30:141:36 | [false] ! ... | local_dataflow.rb:141:20:141:36 | [false] ... && ... |
| local_dataflow.rb:141:30:141:36 | [true] ! ... | local_dataflow.rb:141:20:141:36 | [true] ... && ... |
Expand All @@ -3234,12 +3244,14 @@
| local_dataflow.rb:143:11:143:16 | self | local_dataflow.rb:143:21:143:26 | self |
| local_dataflow.rb:143:11:143:26 | SSA phi read(self) | local_dataflow.rb:144:11:144:16 | self |
| local_dataflow.rb:143:11:143:26 | SSA phi read(x) | local_dataflow.rb:144:15:144:15 | x |
| local_dataflow.rb:143:15:143:15 | [post] x | local_dataflow.rb:143:25:143:25 | x |
| local_dataflow.rb:143:15:143:15 | x | local_dataflow.rb:143:25:143:25 | x |
| local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [false] ... \|\| ... |
| local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [true] ... \|\| ... |
| local_dataflow.rb:143:27:144:16 | then ... | local_dataflow.rb:143:5:144:16 | elsif ... |
| local_dataflow.rb:144:11:144:16 | call to use | local_dataflow.rb:143:27:144:16 | then ... |
| local_dataflow.rb:147:5:147:10 | [post] self | local_dataflow.rb:148:5:148:10 | self |
| local_dataflow.rb:147:5:147:10 | self | local_dataflow.rb:148:5:148:10 | self |
| local_dataflow.rb:147:9:147:9 | [post] x | local_dataflow.rb:148:9:148:9 | x |
| local_dataflow.rb:147:9:147:9 | x | local_dataflow.rb:148:9:148:9 | x |
| local_dataflow.rb:148:5:148:10 | call to use | local_dataflow.rb:132:12:148:10 | then ... |
Loading

0 comments on commit 4177c38

Please sign in to comment.