Skip to content

Commit 24a10aa

Browse files
committed
Recognise send_file as a FileSystemAccess
This method is available in ActionController actions, and sends the file at the given path to the client.
1 parent 82bbe67 commit 24a10aa

File tree

3 files changed

+26
-0
lines changed

3 files changed

+26
-0
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,3 +357,15 @@ private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetti
357357
if this.getWithValueText() = "exception" then result = true else result = false
358358
}
359359
}
360+
361+
/**
362+
* A call to `send_file`, which sends the file at the given path to the client.
363+
*/
364+
private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode {
365+
SendFile() {
366+
this.asExpr().getExpr() instanceof ActionControllerContextCall and
367+
this.getMethodName() = "send_file"
368+
}
369+
370+
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
371+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ edges
3636
| tainted_path.rb:59:12:59:53 | call to new : | tainted_path.rb:60:26:60:29 | path |
3737
| tainted_path.rb:59:40:59:45 | call to params : | tainted_path.rb:59:40:59:52 | ...[...] : |
3838
| tainted_path.rb:59:40:59:52 | ...[...] : | tainted_path.rb:59:12:59:53 | call to new : |
39+
| tainted_path.rb:71:12:71:53 | call to new : | tainted_path.rb:72:15:72:18 | path |
40+
| tainted_path.rb:71:40:71:45 | call to params : | tainted_path.rb:71:40:71:52 | ...[...] : |
41+
| tainted_path.rb:71:40:71:52 | ...[...] : | tainted_path.rb:71:12:71:53 | call to new : |
3942
nodes
4043
| ArchiveApiPathTraversal.rb:5:26:5:31 | call to params : | semmle.label | call to params : |
4144
| ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] : | semmle.label | ...[...] : |
@@ -86,6 +89,10 @@ nodes
8689
| tainted_path.rb:59:40:59:45 | call to params : | semmle.label | call to params : |
8790
| tainted_path.rb:59:40:59:52 | ...[...] : | semmle.label | ...[...] : |
8891
| tainted_path.rb:60:26:60:29 | path | semmle.label | path |
92+
| tainted_path.rb:71:12:71:53 | call to new : | semmle.label | call to new : |
93+
| tainted_path.rb:71:40:71:45 | call to params : | semmle.label | call to params : |
94+
| tainted_path.rb:71:40:71:52 | ...[...] : | semmle.label | ...[...] : |
95+
| tainted_path.rb:72:15:72:18 | path | semmle.label | path |
8996
subpaths
9097
#select
9198
| 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
100107
| 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 |
101108
| 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 |
102109
| 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 |
110+
| 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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,4 +65,10 @@ def route10
6565
path = ActiveStorage::Filename.new(params[:path]).sanitized
6666
@content = File.read path
6767
end
68+
69+
# BAD
70+
def route11
71+
path = ActiveStorage::Filename.new(params[:path])
72+
send_file path
73+
end
6874
end

0 commit comments

Comments
 (0)