Merge pull request #10599 from hmac/hmac/actioncontroller-datastreaming

Ruby: Model send_file

Author: hmac

ruby/ql/lib/change-notes/2022-09-28-actioncontroller-sendfile.md

@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* `ActionController::DataStreaming::send_file` is now recognized as a
+  `FileSystemAccess`.
+

ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

@@ -362,3 +362,15 @@ private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetti
     if this.getWithValueText() = "exception" then result = true else result = false
   }
 }
+
+/**
+ * A call to `send_file`, which sends the file at the given path to the client.
+ */
+private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode {
+  SendFile() {
+    this.asExpr().getExpr() instanceof ActionControllerContextCall and
+    this.getMethodName() = "send_file"
+  }
+
+  override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
+}

ruby/ql/test/query-tests/security/cwe-022/PathInjection.expected

@@ -36,6 +36,9 @@ edges
 | tainted_path.rb:59:12:59:53 | call to new :  | tainted_path.rb:60:26:60:29 | path |
 | tainted_path.rb:59:40:59:45 | call to params :  | tainted_path.rb:59:40:59:52 | ...[...] :  |
 | tainted_path.rb:59:40:59:52 | ...[...] :  | tainted_path.rb:59:12:59:53 | call to new :  |
+| tainted_path.rb:71:12:71:53 | call to new :  | tainted_path.rb:72:15:72:18 | path |
+| tainted_path.rb:71:40:71:45 | call to params :  | tainted_path.rb:71:40:71:52 | ...[...] :  |
+| tainted_path.rb:71:40:71:52 | ...[...] :  | tainted_path.rb:71:12:71:53 | call to new :  |
 nodes
 | ArchiveApiPathTraversal.rb:5:26:5:31 | call to params :  | semmle.label | call to params :  |
 | ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] :  | semmle.label | ...[...] :  |
@@ -86,6 +89,10 @@ nodes
 | tainted_path.rb:59:40:59:45 | call to params :  | semmle.label | call to params :  |
 | tainted_path.rb:59:40:59:52 | ...[...] :  | semmle.label | ...[...] :  |
 | tainted_path.rb:60:26:60:29 | path | semmle.label | path |
+| tainted_path.rb:71:12:71:53 | call to new :  | semmle.label | call to new :  |
+| tainted_path.rb:71:40:71:45 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:71:40:71:52 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:72:15:72:18 | path | semmle.label | path |
 subpaths
 #select
 | ArchiveApiPathTraversal.rb:59:21:59:36 | destination_file | ArchiveApiPathTraversal.rb:5:26:5:31 | call to params :  | ArchiveApiPathTraversal.rb:59:21:59:36 | destination_file | This path depends on a $@. | ArchiveApiPathTraversal.rb:5:26:5:31 | call to params | user-provided value |
@@ -100,3 +107,4 @@ subpaths
 | tainted_path.rb:41:26:41:29 | path | tainted_path.rb:40:26:40:31 | call to params :  | tainted_path.rb:41:26:41:29 | path | This path depends on a $@. | tainted_path.rb:40:26:40:31 | call to params | user-provided value |
 | tainted_path.rb:48:26:48:29 | path | tainted_path.rb:47:43:47:48 | call to params :  | tainted_path.rb:48:26:48:29 | path | This path depends on a $@. | tainted_path.rb:47:43:47:48 | call to params | user-provided value |
 | tainted_path.rb:60:26:60:29 | path | tainted_path.rb:59:40:59:45 | call to params :  | tainted_path.rb:60:26:60:29 | path | This path depends on a $@. | tainted_path.rb:59:40:59:45 | call to params | user-provided value |
+| tainted_path.rb:72:15:72:18 | path | tainted_path.rb:71:40:71:45 | call to params :  | tainted_path.rb:72:15:72:18 | path | This path depends on a $@. | tainted_path.rb:71:40:71:45 | call to params | user-provided value |

ruby/ql/test/query-tests/security/cwe-022/tainted_path.rb

@@ -65,4 +65,10 @@ def route10
     path = ActiveStorage::Filename.new(params[:path]).sanitized
     @content = File.read path
   end
+
+  # BAD
+  def route11
+    path = ActiveStorage::Filename.new(params[:path])
+    send_file path
+  end
 end