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

Remove expand home (`~`) by default in File.expand_path and Path#expand, now opt-in argument #7903

Merged
merged 2 commits into from Nov 12, 2019

Conversation

@didactic-drunk
Copy link
Contributor

didactic-drunk commented Jun 19, 2019

Path#absolute doesn't expand "~/" and may be used for secure path
resolution.


(final behavior)

home specifies the home directory which ~ will expand to.

  • "~" is expanded to the value passed to home.
  • If it is false (default), home is not expanded.
  • If true, it is expanded to the user's home directory (Path.home).
spec/std/path_spec.cr Outdated Show resolved Hide resolved
@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from ef4207d to 1436e26 Jun 20, 2019
@asterite

This comment has been minimized.

Copy link
Member

asterite commented Jun 20, 2019

I don't think I can relate absolute to what it does. In my mind, turning "foo" to absolute would mean it's "/foo".

Maybe we can add an optional argument to expand that is true by default and which specified whether to expand ~.

But it's always better to discuss this is an issue, not a PR.

@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jun 20, 2019

@asterite Absolute is pretty standard and the normal term.

  • Ruby File.absolute_path
    Converts a pathname to an absolute pathname. Relative paths are referenced from the current working directory of the process unless dir_string is given, in which case it will be used as the starting point. If the given pathname starts with a “~” it is NOT expanded, it is treated as a normal directory name.
  • Go func Abs(path string) (string, error)
    https://golang.org/pkg/path/filepath/#Abs
    Abs returns an absolute representation of path. If the path is not absolute it will be joined with the current working directory to turn it into an absolute path. The absolute path name for a given file is not guaranteed to be unique. Abs calls Clean on the result.
  • Java getAbsolutePath()
    This file path method returns the absolute path of the file. If File is created with absolute pathname, it simply returns the pathname.
    If the file object is created using a relative path, the absolute pathname is resolved in a system-dependent way. On UNIX systems, a relative pathname is made absolute by resolving it against the current user directory.
  • C++ std::filesystem::absolute
    Returns a path referencing the same file system location as p, for which is_absolute() is true
  • D absolutePath("some/file", "/foo/bar")
  • Nim had a PR to add absolutePath and already has isAbsolute.
  • Elixir absname(path)
    Converts the given path to an absolute one. Unlike expand/1, no attempt is made to resolve .., . or ~.
  • Python os.path.abspath(path)
    Return a normalized absolutized version of the pathname path. On most platforms, this is equivalent to calling the function normpath() as follows: normpath(join(os.getcwd(), path)).

C# is an outlier calling the function GetFullPath.

Almost all implementations have the 2nd arg as the root directory to allow for caching of Dir.current equivalents.

Absolute path is what people search for. Not fullpath.

And finally absolute(path) matches absolute? in Crystal.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Jun 20, 2019

I think the outlier is actually correct. Absolute seems like unix baggage. Just my opinion.

@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Jun 20, 2019

@asterite Seeing full_path makes me think that ~ should be expanded. absolute doesn't convey such notion, thus is closer to the given implementation. just my 2c.

@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jun 20, 2019

@asterite Perhaps I didn't write enough about the problems with expand.

Most application probably want absolute and NOT expand for most uses except finding user configuration files. See #7768 where misuse of expand accidentally broke an application. He didn't want expand. He wanted absolute but no API provided the necessary function.

There are also serious security implications for any application using expand if an attacker can choose file names or the application calls expand on any filename outside of it's own control.

Depending on the application expand can be used to escape out of a specified file structure to gain read or write access to other files.

mkdir -p expand/~/.ssh
touch expand/~/.ssh/authorized_keys
Dir.cd "expand"
Dir.glob("*/*/*") do |file| # Not sure why ** isn't working.
#Dir.new(".").children.each do |file|
        path = Path[file]
        puts "path #{path} expanded to #{path.expand}"
end

Output:

path ~/.ssh/authorized_keys expanded to /Users/test/.ssh/authorized_keys

If anything expand should work the other way around and not expand ~ by default. Or provide absolute along with expand letting the developer choose the appropriate method.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Jun 20, 2019

Okay, disregard my comments.

src/file.cr Outdated Show resolved Hide resolved
@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from 1436e26 to 871fd08 Jun 22, 2019
@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jun 29, 2019

All of the code reviews were addressed. Is there anything else needed to get this PR approved?

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Jun 29, 2019

Someone needs to approve it. I won't because I'm not too familiar with Path's API so you'll have to wait for someone else. But don't worry, I'm sure this will be merged.

src/path.cr Show resolved Hide resolved
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jul 23, 2019

The described behaviour is not different from expand as evidenced by the implementation. The only difference is an option to disable home path resolution in expand. This seems like a good addition for expand anyway.

But I'm not convinced we need a separate method for this when it's just a simple delegate.
We could however debate whether home argument should perhaps be nil by default.

The return value of Path#expand is an absolute path with the only exception when expand_base is intentionally set to false.

There are also serious security implications for any application using expand if an attacker can choose file names or the application calls expand on any filename outside of it's own control.
Depending on the application expand can be used to escape out of a specified file structure to gain read or write access to other files.

This is completely unrelated. #expand is the same as #absolute. Whether you expand ~ to a user's home directory doesn't add any more security implications than expanding a series of ../ to parent directories.

If you want to restrict the resulting path to a specific path prefix, you need a different solution anyway.

@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jul 26, 2019

But I'm not convinced we need a separate method for this when it's just a simple delegate.
We could however debate whether home argument should perhaps be nil by default.

Great. Tell me which will be accepted and I'll rework this PR. Home argument on or off by default?

Finding the absolute path is a more common operation than home directory lookups. Perhaps off should be the default?

This is completely unrelated. #expand is the same as #absolute. Whether you expand ~ to a user's home directory doesn't add any more security implications than expanding a series of ../ to parent directories.

Incorrect. You are describing sanitizing user input which is a different problem. I am describing taking data returned from the file system and turning a relative path in to an absolute path. A file can't be named .. for this trick to work with Dir.

See my post above on using glob with expand for an example. Dir.new gives similar results.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jul 26, 2019

I don't follow what you describe as a security issue regarding expand. Could you elaborate?

@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jul 27, 2019

Let's say you make a file watching application which watches an input dir and performs actions on any file saved there. Maybe it's a javascript minifier, compressor, decompressor, encryption etc service. It works as follows:

  • Save a file in an input directory.
  • A process chdir's to "input" and periodically polls "./" using Dir.glob or Dir.entries to get a recursive list of files.
  • The process uses File.expand on data returned from Dir.* expecting it will only return an absolute path. Instead it is redirected to another file completely.
  • The watching process may need the expanded path if another application takes part in job processing (when using a queue) or for notification purposes.
mkdir -p input/~/.ssh
touch input/~/.ssh/authorized_keys
Dir.cd "input"
Dir.glob("*/*/*") do |file| # Not sure why ** isn't working.
#Dir.new(".").children.each do |file|
        path = Path[file]
        puts "path #{path} expanded to #{path.expand}"
end

Output:

path ~/.ssh/authorized_keys expanded to /Users/test/.ssh/authorized_keys

Another example is #7768 which was almost hit by this misfeature. Depending on how he wrote his application the files he downloaded may have read or overwritten any files under his home directory including .bashrc or .ssh/authorized_keys. The files he downloaded were in the form of ~foo which was addressed in the PR, but this does not take in to account intentional malicious acts of creating file structures to exploit the current behavior. If crystal gains in popularity I would not expect the situation to improve if applications are already creating exploitable conditions. Wait until torrents start showing up with "~/.bashrc" in the file name.

The default is not safe and there's no current standardized way to get a safe absolute path to a file.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jul 27, 2019

Okay, I see. This is about the ambiguity of a ~ as first character of a path name.

I agree there needs to be a way to expand a path without ~ expansion.
A solution for that would be to make home parameter nilable. As said before, we could consider making it nil by default, which would be more strict and avoid surprises. You'd have to opt in for that.

There is unfortunately no reliable way to escape a ~ character. In a shell you could use a backslash to escape it, but such a path would not work outside a shell, so this should not be included in any return value.
Since the expansion only considers the very first character, adding a ./ prefix would sufficiently escape the ~ and make it unambiguously refer to a path in the working directory. But this would not be a single path component, but actually a path of two components. And #normalize would blow it away; there could be a special case for this, but this only adds layers of complexity and still carries some rough edges.

It seems fine to me when you can toggle ~ expansion:

  • When dealing with paths from user input, you can enable ~ to offer this convenience feature. In order to specify a path starting with ~, it should be fine to insert the path with a ./ prefix.
  • When dealing with paths read from the file system, you would disable ~ because it's not necessary anyway.

Alternatively, we could also remove ~ expansion entirely and leave that for a shard to implement (which could also cover more complex variants like ~foo).
It seems Go, Rust, Node.js don't have this in their stdlib. So it can work without it. However, given this is a pretty commonly used feature, I think it makes sense to have it available in stdlib.

@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from 871fd08 to ad578ad Jul 29, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jul 29, 2019

I think I wouldn't accept a Bool parameter for home, just Path | Nil should suffice. Calling it with home: Path.home is not much more effort than home: true and is more explicit about what it does.

But don't implement that right away. I'd like to hear some thoughts from others as well.

@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Jul 29, 2019

But don't implement that right away. I'd like to hear some thoughts from others as well.

Too late.

@straight-shoota straight-shoota requested a review from RX14 Aug 1, 2019
src/path.cr Outdated Show resolved Hide resolved
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Aug 2, 2019

Please add specs which pass in home as a String or a Path or nil, instead of true or false.

@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from ad578ad to ea8ea6e Aug 3, 2019
spec/std/path_spec.cr Outdated Show resolved Hide resolved
src/path.cr Show resolved Hide resolved
src/file.cr Outdated Show resolved Hide resolved
src/path.cr Outdated Show resolved Hide resolved
spec/std/path_spec.cr Outdated Show resolved Hide resolved
@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from ea8ea6e to 5512210 Aug 3, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Sep 18, 2019

Would be nice to have openat2 wrappers, when openat2 lands in the linux kernel, which allows sandboxing path lookups beneath a given dirfd correctly, ignoring magic links like /proc/, symlinks, paths which use .., etc. Obviously needs a more expensive fallback for older kernel versions to be useful.

Something like dir.open(path, :beneath) #=> File. This would solve all filesystem escapes automatically.

@RX14
RX14 approved these changes Sep 18, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Sep 18, 2019

I guess this can be merged for 0.32.0?

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Sep 18, 2019

Maybe. It's a breaking change and I'd like feedback from @waj before merging it.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Sep 18, 2019

0.32.0 is the release after next, i meant, wait until after the imminent release to merge this

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

@asterite The breaking change is probably not a huge issue because Path is relatively recent addition and not well integrated yet, anyway.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

Never mind. File.expand_path changes behaviour as well.

@straight-shoota straight-shoota added this to the 0.32.0 milestone Sep 19, 2019
@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Sep 19, 2019

If File.expand_path is scheduled for removal and Path is new why not keep home expansion on by default for expand_path and remove it later. Combatibility issues are minimized that way. The security issues will sort themselves when expand_path is removed.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

I'd avoid having different behaviour.

@straight-shoota straight-shoota requested a review from waj Oct 23, 2019
@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Nov 8, 2019

Is there anything for me to do?

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 8, 2019

@didactic-drunk May I ask for a rebase on master? After that, if we have a couple of confirmation of the reviews (since it's been a while) we are good to go.

I think the other needed thing is to update the PR title and opening comment to reflect the final state of the PR.

Copy link
Member

straight-shoota left a comment

This is good to go! Thanks four your patience ❤️

didactic-drunk and others added 2 commits Jun 19, 2019
Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
Co-Authored-By: Johannes Müller <johannes.mueller@smj-fulda.org>
@didactic-drunk didactic-drunk force-pushed the didactic-drunk:expand_path branch from 7f0a5e6 to cf2b5f9 Nov 8, 2019
@didactic-drunk

This comment has been minimized.

Copy link
Contributor Author

didactic-drunk commented Nov 8, 2019

@bcardiff Rebased.

@bcardiff bcardiff changed the title Add Path#absolute as companion to Path#expand. Add home named argument to File.expand_path and Path#expand Nov 11, 2019
Copy link
Member

asterite left a comment

This is a big breaking change that might make fail a lot of apps out there with no indication as to why it fails, other than looking at the changelog and hoping users will find a reference to the File.expand_path entry, and also hoping users will figure out why their code is suddenly failing on non-expanded paths. But we can give it a try.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 11, 2019

Actually, maybe not that many apps will fail. Only those with a hardcoded "~/" in their code will, because I guess if you want to pass that as a command line argument the shell will expand it for you first. The other possibility is that these paths are in a config file but I'm not sure how likely is that.

@straight-shoota straight-shoota changed the title Add home named argument to File.expand_path and Path#expand Remove expand home (`~`) by default in File.expand_path and Path#expand, now opt-in argument Nov 11, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 11, 2019

I think this title makes it more clear that this is a breaking change. The main change here is not adding an argument, it's changing a feature that was previously enabled by default.

@bcardiff bcardiff merged commit b98c0ff into crystal-lang:master Nov 12, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@didactic-drunk didactic-drunk deleted the didactic-drunk:expand_path branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.