Skip to content

Conversation

nielsdos
Copy link
Member

And fix a memleak while here.

@nielsdos nielsdos requested a review from adoy as a code owner November 14, 2024 21:29
@nielsdos nielsdos linked an issue Nov 14, 2024 that may be closed by this pull request
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Thank you!

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

@nielsdos nielsdos closed this in 179ca2b Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

open_basedir bypass using curl extension

3 participants