Skip to content

Fix GH-16802: open_basedir bypass using curl extension #16804

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 3 commits into from
Closed

Fix GH-16802: open_basedir bypass using curl extension #16804

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
181a7d2
Fix GH-16802: open_basedir bypass using curl extension
nielsdos Nov 14, 2024
a00bb86
ver check
nielsdos Nov 14, 2024
8be0265
Don't skip win
nielsdos Nov 15, 2024
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
5 changes: 4 additions & 1 deletion ext/curl/interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,7 +1976,10 @@ static zend_result _php_curl_setopt(php_curl *ch, zend_long option, zval *zvalue
zend_string *str = zval_get_tmp_string(zvalue, &tmp_str);
#if LIBCURL_VERSION_NUM >= 0x075500 /* Available since 7.85.0 */
if ((option == CURLOPT_PROTOCOLS_STR || option == CURLOPT_REDIR_PROTOCOLS_STR) &&
(PG(open_basedir) && *PG(open_basedir)) && php_memnistr(ZSTR_VAL(str), "file", sizeof("file") - 1, ZSTR_VAL(str) + ZSTR_LEN(str)) != NULL) {
(PG(open_basedir) && *PG(open_basedir))
&& (php_memnistr(ZSTR_VAL(str), "file", sizeof("file") - 1, ZSTR_VAL(str) + ZSTR_LEN(str)) != NULL
|| php_memnistr(ZSTR_VAL(str), "all", sizeof("all") - 1, ZSTR_VAL(str) + ZSTR_LEN(str)) != NULL)) {
zend_tmp_string_release(tmp_str);
php_error_docref(NULL, E_WARNING, "The FILE protocol cannot be activated when an open_basedir is set");
return FAILURE;
}
Expand Down
32 changes: 32 additions & 0 deletions ext/curl/tests/gh16802.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--TEST--
GH-16802 (open_basedir bypass using curl extension)
--EXTENSIONS--
curl
--SKIPIF--
<?php
if (PHP_OS_FAMILY === "Windows") die("skip not for Windows");
Copy link
Member

Choose a reason for hiding this comment

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

Why not? The test passes for me (checked master only, though).

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the file paths are Linux file paths. It passes because the check prevents the access anyway, but I didn't want to deal with Windows and check what happens if the patch is not applied. I can remove this check if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Without the patch, only the fourth curl_setopt() would trigger a warning, and curl_exec() would return false (with CURLOPT_VERBOSE "Protocol "file" disabled" is reported). So the actual file:// URI wouldn't matter. Doing curl_exec() after each curl_setopt() would only fail due to "Couldn't open file /etc/passwd" for the first exec; the three following curl_exec() would fail due to "Protocol "file" disabled" (still without the patch).

Anyhow, since the open_basedir value doesn't matter, we could use "file://" . str_replace("\\", "/", __FILE__) instead of file://etc/passwd, but it seems to me checking for the warnings is good enough. Even the curl_exec() is not really relevant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even the curl_exec() is not really relevant.

Yes it is, because we want to check that the path is actually blocked, e.g. it could be broken in a way that warns but still allows the path.

Anyway I'll just drop the Windows skipif

$curl_version = curl_version();
if ($curl_version['version_number'] < 0x075500) {
die("skip: blob options not supported for curl < 7.85.0");
}
?>
--INI--
open_basedir=/nowhere
--FILE--
<?php
$ch = curl_init("file:///etc/passwd");
curl_setopt($ch, CURLOPT_PROTOCOLS_STR, "all");
curl_setopt($ch, CURLOPT_PROTOCOLS_STR, "ftp,all");
curl_setopt($ch, CURLOPT_PROTOCOLS_STR, "all,ftp");
curl_setopt($ch, CURLOPT_PROTOCOLS_STR, "all,file,ftp");
var_dump(curl_exec($ch));
?>
--EXPECTF--
Warning: curl_setopt(): The FILE protocol cannot be activated when an open_basedir is set in %s on line %d

Warning: curl_setopt(): The FILE protocol cannot be activated when an open_basedir is set in %s on line %d

Warning: curl_setopt(): The FILE protocol cannot be activated when an open_basedir is set in %s on line %d

Warning: curl_setopt(): The FILE protocol cannot be activated when an open_basedir is set in %s on line %d
bool(false)
Loading