Skip to content

Fix #76359: open_basedir bypass through adding ".."#7024

Closed
cmb69 wants to merge 3 commits intophp:PHP-7.4from
cmb69:cmb/76359
Closed

Fix #76359: open_basedir bypass through adding ".."#7024
cmb69 wants to merge 3 commits intophp:PHP-7.4from
cmb69:cmb/76359

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented May 21, 2021

We explicitly forbid adding .. to open_basedirat runtime.

Note this is only a minimal fix for the reported issue. There are still problems with .. somewhere in the path, e.g. consider:

+---foo
|       oh.no
|
\---wwwroot
    |   index.php
    |
    \---foo
            no.problem

with index.php:

<?php
chdir(__DIR__);
ini_set('open_basedir', __DIR__);
chdir("./foo");
ini_set('open_basedir', ini_get('open_basedir') . PATH_SEPARATOR . "../foo");
chdir("..");
chdir("../foo");
var_dump(scandir(".")[2]);

outputs:

string(5) "oh.no"

Generally disallowing .. in the path might by too much of a BC break, though.

@cmb69
Fix #76359: open_basedir bypass through adding ".."
a8422d7 
We explicitly forbid adding `..` to `open_basedir`at runtime.
@cmb69 cmb69 added the Bug label May 21, 2021
nikic
nikic reviewed May 21, 2021
View reviewed changes
nikic
nikic reviewed May 21, 2021
View reviewed changes
@cmb69 cmb69 changed the base branch from master to PHP-7.4 May 21, 2021 14:22
@cmb69
Copy link
Member Author

cmb69 commented May 21, 2021

It was actually my intention to target PHP-7.4.

@mvorisek
Copy link
Contributor

mvorisek commented May 21, 2021

ini_set('open_basedir', ini_get('open_basedir') . PATH_SEPARATOR . "../foo");

What is the impl. issue with this?

It should be possible to normalize the set value before it is used in the check and then when the fixed value contains .. the path must be rejected.

@cmb69
Copy link
Member Author

cmb69 commented May 21, 2021

It should be possible to normalize the set value before it is used in the check and then when the fixed value contains .. the path must be rejected.

That would be a behavioral change. As it is, relative paths, and absolute paths containing . or .. are added as is to the setting. Especially, . is even documented to work this way.

@mvorisek
Copy link
Contributor

mvorisek commented May 21, 2021

Then simply store the unnormalized path (if check passed of course) :)

@cmb69
Copy link
Member Author

cmb69 commented May 21, 2021

Then simply store the unnormalized path (if check passed of course) :)

That's what we're doing. But relative paths depend on the CWD, which might change later.

@cmb69 cmb69 closed this in ee9e075 May 25, 2021
@cmb69 cmb69 deleted the cmb/76359 branch May 25, 2021 11:49
@phil-davis phil-davis mentioned this pull request Jul 20, 2021
Closed
@1265578519
Copy link

1265578519 commented Nov 5, 2021

https://bugs.php.net/bug.php?id=76359

看了一眼居然是三年前就捅出来的问题。。。目前才修复吗

@BorelEnzo BorelEnzo mentioned this pull request Jan 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cmb69 @mvorisek @1265578519 @nikic