Skip to content

Commit e95194c

Browse files
aibaarsaibaars
authored
Merge pull request #9477 from thiggy1342/experimental-archive-api
RB: Adding experimental query for detecting path traversal in Archive libraries
2 parents 45af148 + 84fce27 commit e95194c

File tree

8 files changed

+164
-0
lines changed

8 files changed

+164
-0
lines changed

‎ruby/ql/lib/change-notes/released/0.2.3.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
## 0.2.3
2+
3+
### Minor Analysis Improvements
4+
5+
- Calls to `Zip::File.open` and `Zip::File.new` have been added as `FileSystemAccess` sinks. As a result queries like `rb/path-injection` now flag up cases where users may access arbitrary archive files.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ private import codeql.ruby.frameworks.ActiveRecord
88
private import codeql.ruby.frameworks.ActiveStorage
99
private import codeql.ruby.frameworks.ActionView
1010
private import codeql.ruby.frameworks.ActiveSupport
11+
private import codeql.ruby.frameworks.Archive
1112
private import codeql.ruby.frameworks.GraphQL
1213
private import codeql.ruby.frameworks.Rails
1314
private import codeql.ruby.frameworks.Stdlib

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Provides classes for working with archive libraries.
3+
*/
4+
5+
private import ruby
6+
private import codeql.ruby.Concepts
7+
private import codeql.ruby.DataFlow
8+
private import codeql.ruby.ApiGraphs
9+
10+
/**
11+
* Classes and predicates for modeling the RubyZip library
12+
*/
13+
module RubyZip {
14+
/**
15+
* A call to `Zip::File.new`, considered as a `FileSystemAccess`
16+
*/
17+
class RubyZipFileNew extends DataFlow::CallNode, FileSystemAccess::Range {
18+
RubyZipFileNew() { this = API::getTopLevelMember("Zip").getMember("File").getAnInstantiation() }
19+
20+
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
21+
}
22+
23+
/**
24+
* A call to `Zip::File.open`, considered as a `FileSystemAccess`.
25+
*/
26+
class RubyZipFileOpen extends DataFlow::CallNode, FileSystemAccess::Range {
27+
RubyZipFileOpen() {
28+
this = API::getTopLevelMember("Zip").getMember("File").getAMethodCall("open")
29+
}
30+
31+
override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
32+
}
33+
}

‎ruby/ql/test/library-tests/frameworks/archive/Archive.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
rubyZipFileOpens
2+
| Archive.rb:2:12:2:35 | call to open |
3+
rubyZipFileNew
4+
| Archive.rb:5:12:5:34 | call to new |

‎ruby/ql/test/library-tests/frameworks/archive/Archive.ql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
private import ruby
2+
private import codeql.ruby.frameworks.Archive
3+
4+
query predicate rubyZipFileOpens(RubyZip::RubyZipFileOpen f) { any() }
5+
6+
query predicate rubyZipFileNew(RubyZip::RubyZipFileNew f) { any() }

‎ruby/ql/test/library-tests/frameworks/archive/Archive.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# `foo_file` is a RubyZip `Zip::File.open` instance
2+
foo_file = Zip::File.open(filename)
3+
4+
# `new_file` is a RubyZip `Zip::File.new` instance
5+
new_file = Zip::File.new(filename)

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
class TestContoller < ActionController::Base
2+
3+
# this is vulnerable
4+
def upload
5+
untar params[:file], params[:filename]
6+
end
7+
8+
# this is vulnerable
9+
def unpload_zip
10+
unzip params[:file]
11+
end
12+
13+
# this is vulnerable
14+
def create_new_zip
15+
zip params[:filename], files
16+
end
17+
18+
# these are not vulnerable because of the string compare sanitizer
19+
def safe_upload_string_compare
20+
filename = params[:filename]
21+
if filename == "safefile.tar"
22+
untar params[:file], filename
23+
end
24+
end
25+
26+
def safe_upload_zip_string_compare
27+
filename = params[:filename]
28+
if filename == "safefile.zip"
29+
unzip filename
30+
end
31+
end
32+
33+
# these are not vulnerable beacuse of the string array compare sanitizer
34+
def safe_upload_string_array_compare
35+
filename = params[:filename]
36+
if ["safefile1.tar", "safefile2.tar"].include? filename
37+
untar params[:file], filename
38+
end
39+
end
40+
41+
def safe_upload_zip_string_array_compare
42+
filename = params[:filename]
43+
if ["safefile1.zip", "safefile2.zip"].include? filename
44+
unzip filename
45+
end
46+
end
47+
48+
# these are our two sinks
49+
def untar(io, destination)
50+
Gem::Package::TarReader.new io do |tar|
51+
tar.each do |tarfile|
52+
destination_file = File.join destination, tarfile.full_name
53+
54+
if tarfile.directory?
55+
FileUtils.mkdir_p destination_file
56+
else
57+
destination_directory = File.dirname(destination_file)
58+
FileUtils.mkdir_p destination_directory unless File.directory?(destination_directory)
59+
File.open destination_file, "wb" do |f|
60+
f.print tarfile.read
61+
end
62+
end
63+
end
64+
end
65+
end
66+
67+
def unzip(file)
68+
Zip::File.open(file) do |zip_file|
69+
zip_file.each do |entry|
70+
entry.extract
71+
end
72+
end
73+
end
74+
75+
def zip(filename, files = [])
76+
Zip::File.new(filename) do |zf|
77+
files.each do |f|
78+
zf.add f
79+
end
80+
end
81+
end
82+
end

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,15 @@
11
edges
2+
| ArchiveApiPathTraversal.rb:5:26:5:31 | call to params : | ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] : |
3+
| ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] : | ArchiveApiPathTraversal.rb:49:17:49:27 | destination : |
4+
| ArchiveApiPathTraversal.rb:10:11:10:16 | call to params : | ArchiveApiPathTraversal.rb:10:11:10:23 | ...[...] : |
5+
| ArchiveApiPathTraversal.rb:10:11:10:23 | ...[...] : | ArchiveApiPathTraversal.rb:67:13:67:16 | file : |
6+
| ArchiveApiPathTraversal.rb:15:9:15:14 | call to params : | ArchiveApiPathTraversal.rb:15:9:15:25 | ...[...] : |
7+
| ArchiveApiPathTraversal.rb:15:9:15:25 | ...[...] : | ArchiveApiPathTraversal.rb:75:11:75:18 | filename : |
8+
| ArchiveApiPathTraversal.rb:49:17:49:27 | destination : | ArchiveApiPathTraversal.rb:52:38:52:48 | destination : |
9+
| ArchiveApiPathTraversal.rb:52:28:52:67 | call to join : | ArchiveApiPathTraversal.rb:59:21:59:36 | destination_file |
10+
| ArchiveApiPathTraversal.rb:52:38:52:48 | destination : | ArchiveApiPathTraversal.rb:52:28:52:67 | call to join : |
11+
| ArchiveApiPathTraversal.rb:67:13:67:16 | file : | ArchiveApiPathTraversal.rb:68:20:68:23 | file |
12+
| ArchiveApiPathTraversal.rb:75:11:75:18 | filename : | ArchiveApiPathTraversal.rb:76:19:76:26 | filename |
213
| tainted_path.rb:4:12:4:17 | call to params : | tainted_path.rb:4:12:4:24 | ...[...] : |
314
| tainted_path.rb:4:12:4:24 | ...[...] : | tainted_path.rb:5:26:5:29 | path |
415
| tainted_path.rb:10:12:10:43 | call to absolute_path : | tainted_path.rb:11:26:11:29 | path |
@@ -26,6 +37,20 @@ edges
2637
| tainted_path.rb:59:40:59:45 | call to params : | tainted_path.rb:59:40:59:52 | ...[...] : |
2738
| tainted_path.rb:59:40:59:52 | ...[...] : | tainted_path.rb:59:12:59:53 | call to new : |
2839
nodes
40+
| ArchiveApiPathTraversal.rb:5:26:5:31 | call to params : | semmle.label | call to params : |
41+
| ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] : | semmle.label | ...[...] : |
42+
| ArchiveApiPathTraversal.rb:10:11:10:16 | call to params : | semmle.label | call to params : |
43+
| ArchiveApiPathTraversal.rb:10:11:10:23 | ...[...] : | semmle.label | ...[...] : |
44+
| ArchiveApiPathTraversal.rb:15:9:15:14 | call to params : | semmle.label | call to params : |
45+
| ArchiveApiPathTraversal.rb:15:9:15:25 | ...[...] : | semmle.label | ...[...] : |
46+
| ArchiveApiPathTraversal.rb:49:17:49:27 | destination : | semmle.label | destination : |
47+
| ArchiveApiPathTraversal.rb:52:28:52:67 | call to join : | semmle.label | call to join : |
48+
| ArchiveApiPathTraversal.rb:52:38:52:48 | destination : | semmle.label | destination : |
49+
| ArchiveApiPathTraversal.rb:59:21:59:36 | destination_file | semmle.label | destination_file |
50+
| ArchiveApiPathTraversal.rb:67:13:67:16 | file : | semmle.label | file : |
51+
| ArchiveApiPathTraversal.rb:68:20:68:23 | file | semmle.label | file |
52+
| ArchiveApiPathTraversal.rb:75:11:75:18 | filename : | semmle.label | filename : |
53+
| ArchiveApiPathTraversal.rb:76:19:76:26 | filename | semmle.label | filename |
2954
| tainted_path.rb:4:12:4:17 | call to params : | semmle.label | call to params : |
3055
| tainted_path.rb:4:12:4:24 | ...[...] : | semmle.label | ...[...] : |
3156
| tainted_path.rb:5:26:5:29 | path | semmle.label | path |
@@ -63,6 +88,9 @@ nodes
6388
| tainted_path.rb:60:26:60:29 | path | semmle.label | path |
6489
subpaths
6590
#select
91+
| 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 $@. | ArchiveApiPathTraversal.rb:5:26:5:31 | call to params | a user-provided value |
92+
| ArchiveApiPathTraversal.rb:68:20:68:23 | file | ArchiveApiPathTraversal.rb:10:11:10:16 | call to params : | ArchiveApiPathTraversal.rb:68:20:68:23 | file | This path depends on $@. | ArchiveApiPathTraversal.rb:10:11:10:16 | call to params | a user-provided value |
93+
| ArchiveApiPathTraversal.rb:76:19:76:26 | filename | ArchiveApiPathTraversal.rb:15:9:15:14 | call to params : | ArchiveApiPathTraversal.rb:76:19:76:26 | filename | This path depends on $@. | ArchiveApiPathTraversal.rb:15:9:15:14 | call to params | a user-provided value |
6694
| tainted_path.rb:5:26:5:29 | path | tainted_path.rb:4:12:4:17 | call to params : | tainted_path.rb:5:26:5:29 | path | This path depends on $@. | tainted_path.rb:4:12:4:17 | call to params | a user-provided value |
6795
| tainted_path.rb:11:26:11:29 | path | tainted_path.rb:10:31:10:36 | call to params : | tainted_path.rb:11:26:11:29 | path | This path depends on $@. | tainted_path.rb:10:31:10:36 | call to params | a user-provided value |
6896
| tainted_path.rb:17:26:17:29 | path | tainted_path.rb:16:28:16:33 | call to params : | tainted_path.rb:17:26:17:29 | path | This path depends on $@. | tainted_path.rb:16:28:16:33 | call to params | a user-provided value |

0 commit comments

Comments
 (0)