Skip to content

Resolve open_basedir paths on ini update #10987

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Resolve open_basedir paths on ini update #10987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ PHP NEWS
. Expose time spent collecting cycles in gc_status(). (Arnaud)
. Remove WeakMap entries whose key is only reachable through the entry value.
(Arnaud)
. Resolve open_basedir paths on INI update. (ilutov)

- Curl:
. Added Curl options and constants up to (including) version 7.87.
Expand Down
14 changes: 10 additions & 4 deletions Zend/tests/gh10469.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,29 @@
--TEST--
GH-10469: Disallow open_basedir() with parent dir components (..)
--EXTENSIONS--
zend_test
--FILE--
<?php
ini_set('open_basedir', __DIR__);

$pathSeparator = PHP_OS_FAMILY !== 'Windows' ? ':' : ';';
$originalDir = __DIR__;
$tmpDir = $originalDir . '/gh10469_tmp';
@mkdir($tmpDir, 0777, true);
chdir($tmpDir);
ini_set('open_basedir', ini_get('open_basedir') . ':./..');
ini_set('open_basedir', ini_get('open_basedir') . ':./../');
ini_set('open_basedir', ini_get('open_basedir') . ':/a/');
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..');
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR);
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a' . DIRECTORY_SEPARATOR);
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a');

chdir($originalDir);
var_dump(ini_get('open_basedir'));
var_dump(get_open_basedir());
?>
--CLEAN--
<?php
@rmdir(__DIR__ . '/gh10469_tmp');
?>
--EXPECTF--
string(%d) "%stests"
string(%d) "%stests%c.%e..%c.%e..%e%c.%ea%e%c.%ea"
string(%d) "%stests%c%stests%c%stests%e%c%stests%egh10469_tmp%ea"
23 changes: 23 additions & 0 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@
#include "zend_max_execution_timer.h"
#include "zend_hrtime.h"
#include "Optimizer/zend_optimizer.h"
#include "php.h"
#include "php_globals.h"

// FIXME: Breaks the declaration of the function below
#undef zenderror

static size_t global_map_ptr_last = 0;
static bool startup_done = false;
Expand Down Expand Up @@ -1266,11 +1271,29 @@ void zend_call_destructors(void) /* {{{ */
}
/* }}} */

static void zend_release_open_basedir(void)
{
/* Release custom open_basedir config, this needs to happen before ini shutdown */
if (PG(open_basedir)) {
zend_ini_entry *ini_entry = zend_hash_str_find_ptr(EG(ini_directives), "open_basedir", strlen("open_basedir"));
/* ini_entry->modified is unreliable, it might also be set when on_update has failed. */
if (ini_entry
&& ini_entry->modified
&& ini_entry->value != ini_entry->orig_value) {
efree(PG(open_basedir));
PG(open_basedir) = NULL;
}
}
}

ZEND_API void zend_deactivate(void) /* {{{ */
{
/* we're no longer executing anything */
EG(current_execute_data) = NULL;

/* Needs to run before zend_ini_deactivate(). */
zend_release_open_basedir();

zend_try {
shutdown_scanner();
} zend_end_try();
Expand Down
3 changes: 2 additions & 1 deletion ext/session/tests/session_save_path_variation5.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ if (file_exists($sessions) === TRUE) {

var_dump(mkdir($sessions));
var_dump(chdir($sessions));
ini_set('open_basedir', '.');
ini_set("session.save_path", $directory);
var_dump(session_save_path());

Expand All @@ -45,6 +46,6 @@ rmdir($sessions);
bool(true)
bool(true)

Warning: ini_set(): open_basedir restriction in effect. File(%s) is not within the allowed path(s): (.) in %s on line %d
Warning: ini_set(): open_basedir restriction in effect. File(%s) is not within the allowed path(s): (%ssession_save_path_variation5) in %s on line %d
string(0) ""
Done
6 changes: 6 additions & 0 deletions ext/zend_test/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,12 @@ static ZEND_FUNCTION(zend_test_fill_packed_array)
} ZEND_HASH_FILL_END();
}

static ZEND_FUNCTION(get_open_basedir)
{
ZEND_PARSE_PARAMETERS_NONE();
RETURN_STRING(PG(open_basedir));
}

static zend_object *zend_test_class_new(zend_class_entry *class_type)
{
zend_object *obj = zend_objects_new(class_type);
Expand Down
2 changes: 2 additions & 0 deletions ext/zend_test/test.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,8 @@ function zend_test_fill_packed_array(array &$array): void {}

/** @return resource */
function zend_test_create_throwing_resource() {}

function get_open_basedir(): ?string {}
}

namespace ZendTestNS {
Expand Down
7 changes: 6 additions & 1 deletion ext/zend_test/test_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 23 additions & 31 deletions main/fopen_wrappers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "ext/standard/php_standard.h"
#include "zend_compile.h"
#include "php_network.h"
#include "zend_smart_str.h"

#if HAVE_PWD_H
#include <pwd.h>
Expand Down Expand Up @@ -81,60 +82,51 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir)
return SUCCESS;
}

/* Otherwise we're in runtime */
if (!*p || !**p) {
/* open_basedir not set yet, go ahead and give it a value */
*p = ZSTR_VAL(new_value);
return SUCCESS;
}

/* Shortcut: When we have a open_basedir and someone tries to unset, we know it'll fail */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not pointing to this comment.

But the comment above: PHPAPI ZEND_INI_MH(OnUpdateBaseDir) says:

Allows any change to open_basedir setting in during Startup and Shutdown events,
or a tightening during activation/runtime/deactivation

However, we are updating the INI setting without any checks in:

if (stage == PHP_INI_STAGE_STARTUP || stage == PHP_INI_STAGE_SHUTDOWN || stage == PHP_INI_STAGE_ACTIVATE || stage == PHP_INI_STAGE_DEACTIVATE)

So this is confusing, wondering what the actual php.net docs say as they may also be misleading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more just a shortcut in the code as it is clear this will fail the check so we don't bother to do the actual check. Pretty much what it says so not sure what's confusing about this?

if (!new_value || !*ZSTR_VAL(new_value)) {
return FAILURE;
}

/* Is the proposed open_basedir at least as restrictive as the current setting? */
smart_str buf = {0};
ptr = pathbuf = estrdup(ZSTR_VAL(new_value));
while (ptr && *ptr) {
end = strchr(ptr, DEFAULT_DIR_SEPARATOR);
if (end != NULL) {
*end = '\0';
end++;
}
/* Don't allow paths with a parent dir component (..) to be set at runtime */
char *substr_pos = ptr;
while (*substr_pos) {
// Check if we have a .. path component
if (substr_pos[0] == '.'
&& substr_pos[1] == '.'
&& (substr_pos[2] == '\0' || IS_SLASH(substr_pos[2]))) {
efree(pathbuf);
return FAILURE;
}
// Skip to the next path component
while (true) {
substr_pos++;
if (*substr_pos == '\0' || *substr_pos == DEFAULT_DIR_SEPARATOR) {
goto no_parent_dir_component;
} else if (IS_SLASH(*substr_pos)) {
// Also skip the slash
substr_pos++;
break;
}
}
char resolved_name[MAXPATHLEN + 1];
if (expand_filepath(ptr, resolved_name) == NULL) {
efree(pathbuf);
smart_str_free(&buf);
return FAILURE;
}
no_parent_dir_component:
if (php_check_open_basedir_ex(ptr, 0) != 0) {
if (php_check_open_basedir_ex(resolved_name, 0) != 0) {
/* At least one portion of this open_basedir is less restrictive than the prior one, FAIL */
efree(pathbuf);
smart_str_free(&buf);
return FAILURE;
}
if (smart_str_get_len(&buf) != 0) {
smart_str_appendc(&buf, DEFAULT_DIR_SEPARATOR);
}
smart_str_appends(&buf, resolved_name);
ptr = end;
}
efree(pathbuf);

/* Everything checks out, set it */
*p = ZSTR_VAL(new_value);
if (*p) {
/* Unfortunately entry->modified has already been set to true so we compare entry->value
* against entry->orig_value. */
if (entry->modified && entry->value != entry->orig_value) {
efree(*p);
}
}
zend_string *tmp = smart_str_extract(&buf);
*p = estrdup(ZSTR_VAL(tmp));
zend_string_release(tmp);

return SUCCESS;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/security/bug76359.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ chdir("..");
chdir("..");
?>
--EXPECTF--
bool(false)
string(%d) "%ssecurity"

Warning: chdir(): open_basedir restriction in effect. File(..) is not within the allowed path(s): (%s) in %s on line %d
--CLEAN--
Expand Down
15 changes: 8 additions & 7 deletions tests/security/open_basedir_readlink.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ $symlink = ($initdir."/test/ok/symlink.txt");
var_dump(symlink($target, $symlink));

chdir($initdir."/test/ok");
ini_set("open_basedir", ".");

var_dump(readlink("symlink.txt"));
var_dump(readlink("../ok/symlink.txt"));
Expand Down Expand Up @@ -49,24 +50,24 @@ bool(true)
bool(true)
bool(true)

Warning: readlink(): open_basedir restriction in effect. File(symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: readlink(): open_basedir restriction in effect. File(../ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(../ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: readlink(): open_basedir restriction in effect. File(../ok/./symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(../ok/./symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: readlink(): open_basedir restriction in effect. File(./symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(./symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: symlink(): open_basedir restriction in effect. File(%s/test/bad/bad.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: symlink(): open_basedir restriction in effect. File(%s/test/bad/bad.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)

Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (.) in %s on line %d
Warning: readlink(): open_basedir restriction in effect. File(%s/test/ok/symlink.txt) is not within the allowed path(s): (%sok) in %s on line %d
bool(false)
*** Finished testing open_basedir configuration [readlink] ***