Skip to content

Commit 804aa29

Browse files
iluuu1994iluuu1994
committed
Resolve open_basedir paths on ini update
Closes GH-10987
1 parent 1057cce commit 804aa29

File tree

10 files changed

+82
-45
lines changed

10 files changed

+82
-45
lines changed

‎NEWS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ PHP NEWS
1818
. Expose time spent collecting cycles in gc_status(). (Arnaud)
1919
. Remove WeakMap entries whose key is only reachable through the entry value.
2020
(Arnaud)
21+
. Resolve open_basedir paths on INI update. (ilutov)
2122

2223
- Curl:
2324
. Added Curl options and constants up to (including) version 7.87.

‎Zend/tests/gh10469.phpt

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,29 @@
11
--TEST--
22
GH-10469: Disallow open_basedir() with parent dir components (..)
3+
--EXTENSIONS--
4+
zend_test
35
--FILE--
46
<?php
57
ini_set('open_basedir', __DIR__);
68

9+
$pathSeparator = PHP_OS_FAMILY !== 'Windows' ? ':' : ';';
710
$originalDir = __DIR__;
811
$tmpDir = $originalDir . '/gh10469_tmp';
912
@mkdir($tmpDir, 0777, true);
1013
chdir($tmpDir);
11-
ini_set('open_basedir', ini_get('open_basedir') . ':./..');
12-
ini_set('open_basedir', ini_get('open_basedir') . ':./../');
13-
ini_set('open_basedir', ini_get('open_basedir') . ':/a/');
14+
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..');
15+
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR);
16+
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a' . DIRECTORY_SEPARATOR);
17+
ini_set('open_basedir', ini_get('open_basedir') . $pathSeparator . '.' . DIRECTORY_SEPARATOR . 'a');
1418

1519
chdir($originalDir);
1620
var_dump(ini_get('open_basedir'));
21+
var_dump(get_open_basedir());
1722
?>
1823
--CLEAN--
1924
<?php
2025
@rmdir(__DIR__ . '/gh10469_tmp');
2126
?>
2227
--EXPECTF--
23-
string(%d) "%stests"
28+
string(%d) "%stests%c.%e..%c.%e..%e%c.%ea%e%c.%ea"
29+
string(%d) "%stests%c%stests%c%stests%e%c%stests%egh10469_tmp%ea"

‎Zend/zend.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@
3939
#include "zend_max_execution_timer.h"
4040
#include "zend_hrtime.h"
4141
#include "Optimizer/zend_optimizer.h"
42+
#include "php.h"
43+
#include "php_globals.h"
44+
45+
// FIXME: Breaks the declaration of the function below
46+
#undef zenderror
4247

4348
static size_t global_map_ptr_last = 0;
4449
static bool startup_done = false;
@@ -1266,11 +1271,29 @@ void zend_call_destructors(void) /* {{{ */
12661271
}
12671272
/* }}} */
12681273

1274+
static void zend_release_open_basedir(void)
1275+
{
1276+
/* Release custom open_basedir config, this needs to happen before ini shutdown */
1277+
if (PG(open_basedir)) {
1278+
zend_ini_entry *ini_entry = zend_hash_str_find_ptr(EG(ini_directives), "open_basedir", strlen("open_basedir"));
1279+
/* ini_entry->modified is unreliable, it might also be set when on_update has failed. */
1280+
if (ini_entry
1281+
&& ini_entry->modified
1282+
&& ini_entry->value != ini_entry->orig_value) {
1283+
efree(PG(open_basedir));
1284+
PG(open_basedir) = NULL;
1285+
}
1286+
}
1287+
}
1288+
12691289
ZEND_API void zend_deactivate(void) /* {{{ */
12701290
{
12711291
/* we're no longer executing anything */
12721292
EG(current_execute_data) = NULL;
12731293

1294+
/* Needs to run before zend_ini_deactivate(). */
1295+
zend_release_open_basedir();
1296+
12741297
zend_try {
12751298
shutdown_scanner();
12761299
} zend_end_try();

‎ext/session/tests/session_save_path_variation5.phpt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ if (file_exists($sessions) === TRUE) {
2828

2929
var_dump(mkdir($sessions));
3030
var_dump(chdir($sessions));
31+
ini_set('open_basedir', '.');
3132
ini_set("session.save_path", $directory);
3233
var_dump(session_save_path());
3334

@@ -45,6 +46,6 @@ rmdir($sessions);
4546
bool(true)
4647
bool(true)
4748

48-
Warning: ini_set(): open_basedir restriction in effect. File(%s) is not within the allowed path(s): (.) in %s on line %d
49+
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
4950
string(0) ""
5051
Done

‎ext/zend_test/test.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,12 @@ static ZEND_FUNCTION(zend_test_fill_packed_array)
620620
} ZEND_HASH_FILL_END();
621621
}
622622

623+
static ZEND_FUNCTION(get_open_basedir)
624+
{
625+
ZEND_PARSE_PARAMETERS_NONE();
626+
RETURN_STRING(PG(open_basedir));
627+
}
628+
623629
static zend_object *zend_test_class_new(zend_class_entry *class_type)
624630
{
625631
zend_object *obj = zend_objects_new(class_type);

‎ext/zend_test/test.stub.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,8 @@ function zend_test_fill_packed_array(array &$array): void {}
224224

225225
/** @return resource */
226226
function zend_test_create_throwing_resource() {}
227+
228+
function get_open_basedir(): ?string {}
227229
}
228230

229231
namespace ZendTestNS {

‎ext/zend_test/test_arginfo.h

Lines changed: 6 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎main/fopen_wrappers.c

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "ext/standard/php_standard.h"
3939
#include "zend_compile.h"
4040
#include "php_network.h"
41+
#include "zend_smart_str.h"
4142

4243
#if HAVE_PWD_H
4344
#include <pwd.h>
@@ -81,60 +82,51 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir)
8182
return SUCCESS;
8283
}
8384

84-
/* Otherwise we're in runtime */
85-
if (!*p || !**p) {
86-
/* open_basedir not set yet, go ahead and give it a value */
87-
*p = ZSTR_VAL(new_value);
88-
return SUCCESS;
89-
}
90-
9185
/* Shortcut: When we have a open_basedir and someone tries to unset, we know it'll fail */
9286
if (!new_value || !*ZSTR_VAL(new_value)) {
9387
return FAILURE;
9488
}
9589

9690
/* Is the proposed open_basedir at least as restrictive as the current setting? */
91+
smart_str buf = {0};
9792
ptr = pathbuf = estrdup(ZSTR_VAL(new_value));
9893
while (ptr && *ptr) {
9994
end = strchr(ptr, DEFAULT_DIR_SEPARATOR);
10095
if (end != NULL) {
10196
*end = '\0';
10297
end++;
10398
}
104-
/* Don't allow paths with a parent dir component (..) to be set at runtime */
105-
char *substr_pos = ptr;
106-
while (*substr_pos) {
107-
// Check if we have a .. path component
108-
if (substr_pos[0] == '.'
109-
&& substr_pos[1] == '.'
110-
&& (substr_pos[2] == '\0' || IS_SLASH(substr_pos[2]))) {
111-
efree(pathbuf);
112-
return FAILURE;
113-
}
114-
// Skip to the next path component
115-
while (true) {
116-
substr_pos++;
117-
if (*substr_pos == '\0' || *substr_pos == DEFAULT_DIR_SEPARATOR) {
118-
goto no_parent_dir_component;
119-
} else if (IS_SLASH(*substr_pos)) {
120-
// Also skip the slash
121-
substr_pos++;
122-
break;
123-
}
124-
}
99+
char resolved_name[MAXPATHLEN + 1];
100+
if (expand_filepath(ptr, resolved_name) == NULL) {
101+
efree(pathbuf);
102+
smart_str_free(&buf);
103+
return FAILURE;
125104
}
126-
no_parent_dir_component:
127-
if (php_check_open_basedir_ex(ptr, 0) != 0) {
105+
if (php_check_open_basedir_ex(resolved_name, 0) != 0) {
128106
/* At least one portion of this open_basedir is less restrictive than the prior one, FAIL */
129107
efree(pathbuf);
108+
smart_str_free(&buf);
130109
return FAILURE;
131110
}
111+
if (smart_str_get_len(&buf) != 0) {
112+
smart_str_appendc(&buf, DEFAULT_DIR_SEPARATOR);
113+
}
114+
smart_str_appends(&buf, resolved_name);
132115
ptr = end;
133116
}
134117
efree(pathbuf);
135118

136119
/* Everything checks out, set it */
137-
*p = ZSTR_VAL(new_value);
120+
if (*p) {
121+
/* Unfortunately entry->modified has already been set to true so we compare entry->value
122+
* against entry->orig_value. */
123+
if (entry->modified && entry->value != entry->orig_value) {
124+
efree(*p);
125+
}
126+
}
127+
zend_string *tmp = smart_str_extract(&buf);
128+
*p = estrdup(ZSTR_VAL(tmp));
129+
zend_string_release(tmp);
138130

139131
return SUCCESS;
140132
}

‎tests/security/bug76359.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ chdir("..");
1010
chdir("..");
1111
?>
1212
--EXPECTF--
13-
bool(false)
13+
string(%d) "%ssecurity"
1414

1515
Warning: chdir(): open_basedir restriction in effect. File(..) is not within the allowed path(s): (%s) in %s on line %d
1616
--CLEAN--

‎tests/security/open_basedir_readlink.phpt

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ $symlink = ($initdir."/test/ok/symlink.txt");
2121
var_dump(symlink($target, $symlink));
2222

2323
chdir($initdir."/test/ok");
24+
ini_set("open_basedir", ".");
2425

2526
var_dump(readlink("symlink.txt"));
2627
var_dump(readlink("../ok/symlink.txt"));
@@ -49,24 +50,24 @@ bool(true)
4950
bool(true)
5051
bool(true)
5152

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

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

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

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

64-
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
65+
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
6566
bool(false)
6667

67-
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
68+
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
6869
bool(false)
6970

70-
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
71+
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
7172
bool(false)
7273
*** Finished testing open_basedir configuration [readlink] ***

0 commit comments

Comments
 (0)