update the QHelp for rb/path-injection

Author: erik-krogh

ruby/ql/src/queries/security/cwe-022/PathInjection.qhelp

@@ -2,60 +2,67 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
-
 <overview>
-<p>
-Accessing files using paths constructed from user-controlled data can allow an
-attacker to access unexpected resources. This can result in sensitive
-information being revealed or deleted, or an attacker being able to influence
-behavior by modifying unexpected files.
-</p>
-</overview>
+<p>Accessing paths controlled by users can allow an attacker to access unexpected resources. This 
+can result in sensitive information being revealed or deleted, or an attacker being able to influence
+behavior by modifying unexpected files.</p>
+
+<p>Paths that are naively constructed from data controlled by a user may be absolute paths, or may contain
+unexpected special characters such as "..". Such a path could point anywhere on the file system.</p>
 
+</overview>
 <recommendation>
-<p>
-Validate user input before using it to construct a file path, either using an
-off-the-shelf library like <code>ActiveStorage::Filename#sanitized</code> in
-Rails, or by performing custom validation.
+
+<p>Validate user input before using it to construct a file path.</p>
+
+<p>Common validation methods include checking that the normalized path is relative and does not contain
+any ".." components, or checking that the path is contained within a safe folder. The method you should use depends 
+on how the path is used in the application, and whether the path should be a single path component.
+</p>
+
+<p>If the path should be a single path component (such as a file name), you can check for the existence 
+of any path separators ("/" or "\"), or ".." sequences in the input, and reject the input if any are found.
 </p>
 
 <p>
-Ideally, follow these rules:
+Note that removing "../" sequences is <i>not</i> sufficient, since the input could still contain a path separator
+followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences
+are removed.
 </p>
 
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the file
-system).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For
-example, after applying this filter to ".../...//", the resulting string would
-still be "../".</li>
-<li>Use a whitelist of known good patterns.</li>
-</ul>
-</recommendation>
+<p>Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that
+the user input matches one of these patterns.</p>
 
+</recommendation>
 <example>
+
+<p>In this example, a file name is read from a query parameter and then used to access a file
+and send it back over the socket. However, a malicious user could enter a file name anywhere on the file system,
+such as "/etc/passwd" or "../../../etc/passwd".</p>
+
+<sample src="examples/tainted_path.rb" />
+
 <p>
-In the first example, a file name is read from an HTTP request and then used to
-access a file.  However, a malicious user could enter a file name which is an
-absolute path, such as <code>"/etc/passwd"</code>.
+If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
 </p>
 
+<sample src="examples/tainted_path_normalize.rb" />
+
 <p>
-In the second example, it appears that the user is restricted to opening a file
-within the <code>"user"</code> home directory. However, a malicious user could
-enter a file name containing special characters. For example, the string
-<code>"../../etc/passwd"</code> will result in the code reading the file located
-at <code>"/home/user/../../etc/passwd"</code>, which is the system's password
-file. This file would then be sent back to the user, giving them access to all
-the system's passwords.
+If the input should be within a specific directory, you can check that the resolved path 
+is still contained within that directory.
 </p>
 
-<sample src="examples/tainted_path.rb" />
-</example>
+<sample src="examples/tainted_path_folder.rb" />
 
+</example>
 <references>
-<li>OWASP: <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.</li>
+
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.
 <li>Rails: <a href="https://api.rubyonrails.org/classes/ActiveStorage/Filename.html#method-i-sanitized">ActiveStorage::Filename#sanitized</a>.</li>
+</li>
+
 </references>
 </qhelp>

ruby/ql/src/queries/security/cwe-022/examples/tainted_path_folder.rb

@@ -0,0 +1,18 @@
+require 'pathname'
+
+class FilesController < ActionController::Base
+  def safe_example
+    filename = params[:path]
+    user_directory = "/home/#{current_user}/public" # Assuming `current_user` method returns the user's name
+
+    public_folder = Pathname.new(user_directory).cleanpath.to_s
+    file_path = Pathname.new(File.join(user_directory, filename)).cleanpath.to_s
+
+    # GOOD: Ensure that the path stays within the public folder
+    if !file_path.start_with?(public_folder + File::SEPARATOR)
+      raise ArgumentError, "Invalid filename"
+    else
+      @content = File.read(file_path)
+    end
+  end
+end

ruby/ql/src/queries/security/cwe-022/examples/tainted_path_normalize.rb

@@ -0,0 +1,12 @@
+class FilesController < ActionController::Base
+  def safe_example
+    filename = params[:path]
+    # GOOD: Ensure that the filename has no path separators or parent directory references
+    if filename.include?("..") || filename.include?("/") || filename.include?("\\")
+      raise ArgumentError, "Invalid filename"
+    else
+      # Assuming files are stored in a specific directory, e.g., /home/user/files/
+      @content = File.read("/home/user/files/#{filename}")
+    end
+  end
+end

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

@@ -66,6 +66,18 @@ edges
 | tainted_path.rb:90:12:90:53 | call to new | tainted_path.rb:90:5:90:8 | path | provenance |  |
 | tainted_path.rb:90:40:90:45 | call to params | tainted_path.rb:90:40:90:52 | ...[...] | provenance |  |
 | tainted_path.rb:90:40:90:52 | ...[...] | tainted_path.rb:90:12:90:53 | call to new | provenance |  |
+| tainted_path.rb:98:5:98:12 | filename | tainted_path.rb:102:56:102:63 | filename | provenance |  |
+| tainted_path.rb:98:16:98:21 | call to params | tainted_path.rb:98:16:98:28 | ...[...] | provenance |  |
+| tainted_path.rb:98:16:98:28 | ...[...] | tainted_path.rb:98:5:98:12 | filename | provenance |  |
+| tainted_path.rb:102:5:102:13 | file_path | tainted_path.rb:108:28:108:36 | file_path | provenance |  |
+| tainted_path.rb:102:17:102:65 | call to new | tainted_path.rb:102:17:102:75 | call to cleanpath | provenance |  |
+| tainted_path.rb:102:17:102:75 | call to cleanpath | tainted_path.rb:102:17:102:80 | call to to_s | provenance |  |
+| tainted_path.rb:102:17:102:80 | call to to_s | tainted_path.rb:102:5:102:13 | file_path | provenance |  |
+| tainted_path.rb:102:30:102:64 | call to join | tainted_path.rb:102:17:102:65 | call to new | provenance |  |
+| tainted_path.rb:102:56:102:63 | filename | tainted_path.rb:102:30:102:64 | call to join | provenance |  |
+| tainted_path.rb:113:5:113:12 | filename | tainted_path.rb:119:28:119:57 | "/home/user/files/#{...}" | provenance |  |
+| tainted_path.rb:113:16:113:21 | call to params | tainted_path.rb:113:16:113:28 | ...[...] | provenance |  |
+| tainted_path.rb:113:16:113:28 | ...[...] | tainted_path.rb:113:5:113:12 | filename | provenance |  |
 nodes
 | ArchiveApiPathTraversal.rb:5:26:5:31 | call to params | semmle.label | call to params |
 | ArchiveApiPathTraversal.rb:5:26:5:42 | ...[...] | semmle.label | ...[...] |
@@ -150,6 +162,20 @@ nodes
 | tainted_path.rb:90:40:90:45 | call to params | semmle.label | call to params |
 | tainted_path.rb:90:40:90:52 | ...[...] | semmle.label | ...[...] |
 | tainted_path.rb:92:11:92:14 | path | semmle.label | path |
+| tainted_path.rb:98:5:98:12 | filename | semmle.label | filename |
+| tainted_path.rb:98:16:98:21 | call to params | semmle.label | call to params |
+| tainted_path.rb:98:16:98:28 | ...[...] | semmle.label | ...[...] |
+| tainted_path.rb:102:5:102:13 | file_path | semmle.label | file_path |
+| tainted_path.rb:102:17:102:65 | call to new | semmle.label | call to new |
+| tainted_path.rb:102:17:102:75 | call to cleanpath | semmle.label | call to cleanpath |
+| tainted_path.rb:102:17:102:80 | call to to_s | semmle.label | call to to_s |
+| tainted_path.rb:102:30:102:64 | call to join | semmle.label | call to join |
+| tainted_path.rb:102:56:102:63 | filename | semmle.label | filename |
+| tainted_path.rb:108:28:108:36 | file_path | semmle.label | file_path |
+| tainted_path.rb:113:5:113:12 | filename | semmle.label | filename |
+| tainted_path.rb:113:16:113:21 | call to params | semmle.label | call to params |
+| tainted_path.rb:113:16:113:28 | ...[...] | semmle.label | ...[...] |
+| tainted_path.rb:119:28:119:57 | "/home/user/files/#{...}" | semmle.label | "/home/user/files/#{...}" |
 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 |
@@ -170,3 +196,5 @@ subpaths
 | tainted_path.rb:85:10:85:13 | path | tainted_path.rb:84:40:84:45 | call to params | tainted_path.rb:85:10:85:13 | path | This path depends on a $@. | tainted_path.rb:84:40:84:45 | call to params | user-provided value |
 | tainted_path.rb:86:25:86:28 | path | tainted_path.rb:84:40:84:45 | call to params | tainted_path.rb:86:25:86:28 | path | This path depends on a $@. | tainted_path.rb:84:40:84:45 | call to params | user-provided value |
 | tainted_path.rb:92:11:92:14 | path | tainted_path.rb:90:40:90:45 | call to params | tainted_path.rb:92:11:92:14 | path | This path depends on a $@. | tainted_path.rb:90:40:90:45 | call to params | user-provided value |
+| tainted_path.rb:108:28:108:36 | file_path | tainted_path.rb:98:16:98:21 | call to params | tainted_path.rb:108:28:108:36 | file_path | This path depends on a $@. | tainted_path.rb:98:16:98:21 | call to params | user-provided value |
+| tainted_path.rb:119:28:119:57 | "/home/user/files/#{...}" | tainted_path.rb:113:16:113:21 | call to params | tainted_path.rb:119:28:119:57 | "/home/user/files/#{...}" | This path depends on a $@. | tainted_path.rb:113:16:113:21 | call to params | user-provided value |

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

@@ -91,4 +91,32 @@ def require_relative()
     puts "Debug: require_relative(#{path})"
     super(path)
   end
+
+  require 'pathname'
+
+  def safe_example_folder()
+    filename = params[:path]
+    user_directory = "/home/#{current_user}/public" # Assuming `current_user` method returns the user's name
+
+    public_folder = Pathname.new(user_directory).cleanpath.to_s
+    file_path = Pathname.new(File.join(user_directory, filename)).cleanpath.to_s
+
+    # GOOD: Ensure that the path stays within the public folder
+    if !file_path.start_with?(public_folder + File::SEPARATOR)
+      raise ArgumentError, "Invalid filename"
+    else
+      @content = File.read(file_path) # GOOD, but we falsely report a vulnerability
+    end
+  end
+
+  def safe_example_normalize()
+    filename = params[:path]
+    # GOOD: Ensure that the filename has no path separators or parent directory references
+    if filename.include?("..") || filename.include?("/") || filename.include?("\\")
+      raise ArgumentError, "Invalid filename"
+    else
+      # Assuming files are stored in a specific directory, e.g., /home/user/files/
+      @content = File.read("/home/user/files/#{filename}") # GOOD, but we falsely report a vulnerability
+    end
+  end
 end