Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RB: update the QHelp for rb/path-injection #16109

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erik-krogh
Copy link
Contributor

Draft for now, because the new fixes produce lots of FPs.

Copy link
Contributor

github-actions bot commented Apr 3, 2024

QHelp previews:

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

Uncontrolled data used in path expression

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.

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.

Recommendation

Validate user input before using it to construct a file path.

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.

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.

Note that removing "../" sequences is not 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.

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.

Example

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".

class FilesController < ActionController::Base
  def first_example
    # BAD: This could read any file on the file system
    @content = File.read params[:path]
  end

  def second_example
    # BAD: This could still read any file on the file system
    @content = File.read "/home/user/#{ params[:path] }"
  end
end

If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.

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

If the input should be within a specific directory, you can check that the resolved path is still contained within that directory.

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

References

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant