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

Rule for Pathname.join with forward slash. #1721

Closed
tehryanx opened this issue Jul 8, 2022 · 4 comments · Fixed by #1733
Closed

Rule for Pathname.join with forward slash. #1721

tehryanx opened this issue Jul 8, 2022 · 4 comments · Fixed by #1733

Comments

@tehryanx
Copy link

tehryanx commented Jul 8, 2022

Is your feature request related to a problem? Please describe.
Pathname.join has a weird behavioral quirk where, if a string beginning with a / is joined to the end of a pathname object, the entire pathname is removed up to the /.

A developer might do something like this: Rails.root.join("safe_path", user_submitted_path) thinking that the user only has control over the end of the path.

Describe the solution you'd like
It would be nice to have a rule that detects this. Below is an example of what the issue looks like:

irb(main):012:0> Rails.root.join("safe_path", "a", "b", "c")
=> #<Pathname:/home/ryan/test/safe_path/a/b/c>
irb(main):013:0> Rails.root.join("safe_path", "a", "b", "/c")
=> #<Pathname:/c>
@presidentbeef
Copy link
Owner

Honestly this seems like a straight up vulnerability in Pathname. Have you considered reporting it to Ruby Core?

(Easy to add a Brakeman check for it, though.)

@presidentbeef
Copy link
Owner

To me it looks like an absolute path anywhere in Pathname#join is a problem:

> Pathname.new('a').join('/c', 'd')
 => #<Pathname:/c/d> 

compared to

> File.join("a", "/c", "d")
 => "a/c/d"

@mockdeep
Copy link

@presidentbeef is there a recommended way to satisfy this rule? I'm trying to sub out the initial / and it still reports an issue:

Confidence: High
Category: Path Traversal
Check: Pathname
Message: Absolute paths in `Pathname#join` cause the entire path to be relative to the absolute path, ignoring any prior values
Code: Rails.root.join("tmp", "#{params[:file_path].sub(/^\//, "")}.zip")
File: app/controllers/test_files_controller.rb
Line: 26

@Lasvad
Copy link

Lasvad commented Jan 9, 2023

This is discussed in more detail here: https://bugs.ruby-lang.org/issues/14891 and looks to be considered a feature, not a bug, based on this comment https://bugs.ruby-lang.org/issues/14891#note-7

Repository owner locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants