Skip to content
New issue

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

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

Already on GitHub? # 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 woefe force-pushed the test-relative-symlink branch from c440890 to b558c47 Compare February 2, 2021 23:33
@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.

@mrook mrook force-pushed the test-relative-symlink branch from b558c47 to c5dbd9c Compare February 4, 2021 08:45
woefe added 2 commits February 4, 2021 09:47
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 mrook force-pushed the test-relative-symlink branch from c5dbd9c to b6da5c3 Compare February 4, 2021 08:47
@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants