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

Fix out-of-path check for benign symlinks #36

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

woefe
Copy link
Contributor

@woefe woefe commented Feb 2, 2021

As far as I understand the problem in #35, a symlink is "out-of-path" if it is an absolute path or goes "up" too many times. This proposed fix checks the amount of ".." vs. normal downward path elements (excluding "." and "").

@woefe woefe changed the title Add testcase for relative, in-path symlink Add testcase and fix for relative, in-path symlink Feb 2, 2021
@woefe
Copy link
Contributor Author

woefe commented Feb 2, 2021

I'm not 100% sure that this proposed fix also mitigates CVE-2020-36193. It's probably a good idea to consult whoever reported that vulnerability to you.

@woefe
Copy link
Contributor Author

woefe commented Feb 2, 2021

Nope, this does not fix CVE-2020-36193 🙈. ../../../../root/a/b/c/d would not be detected as out-of-path. I'll have another take at it

@woefe
Copy link
Contributor Author

woefe commented Feb 2, 2021

Okay, now I think its fine

@mrook
Copy link
Member

mrook commented Feb 3, 2021

Thanks for taking a look at this @woefe! I'll review it later this week.

A symlink is out-of-path if it is an absolute path or goes "up" too many
times. This checks how deep the filename is and whether the link points
more levels up than the depth of the filename.
@mrook
Copy link
Member

mrook commented Feb 4, 2021

@woefe str_starts_with is PHP 8+, so I replaced that. I'll do some more testing but it looks good.

@mrook mrook merged commit 40283fb into pear:master Feb 4, 2021
@mrook mrook changed the title Add testcase and fix for relative, in-path symlink Fix out-of-path check for benign symlinks Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants