phar: reject extractTo() when destination parent escapes via symlink by iliaal · Pull Request #21808 · php/php-src

iliaal · 2026-04-18T19:10:23Z

Fixes destination-side symlink traversal in Phar::extractTo().

extractTo() builds the target path as dest + "/" + entry_path. CWD_EXPAND validates the entry path against .. traversal but doesn't resolve symlinks. A pre-existing symlink in the destination (e.g., /dest/subdir -> /etc) causes the extracted file to land outside the intended root.

After creating the parent directory, resolve it with VCWD_REALPATH and verify it stays under dest. Return FAILURE if it escapes so the caller throws a PharException.

The test skips on Windows only when SeCreateSymbolicLinkPrivilege is absent. Without it, symlink() returns false without error, so no symlink exists in the destination, extractTo() succeeds, and the test outputs EXTRACTED (unexpected) instead of the PharException that --EXPECTF-- requires.

LamentXU123

just my 2 cents,

LamentXU123 · 2026-04-19T11:18:30Z

+				return FAILURE;
+			}
+			size_t resolved_dest_len = strlen(resolved_dest);
+			if (strncmp(resolved, resolved_dest, resolved_dest_len) != 0 ||


nit: could use zend_binary_strncmp

Keeping strncmp. phar uses it throughout for path prefix checks (util.c:1296, stream.c:615, phar.c:509). zend_binary_strncmp carries three-way-compare length semantics for userland zend_string comparison, not a prefix check on NUL-terminated realpath output.

LamentXU123 · 2026-04-19T11:21:50Z

+			}
+			size_t resolved_dest_len = strlen(resolved_dest);
+			if (strncmp(resolved, resolved_dest, resolved_dest_len) != 0 ||
+					(resolved[resolved_dest_len] != '/' && resolved[resolved_dest_len] != '\\' && resolved[resolved_dest_len] != '\0')) {


nit: could use IS_SLASH

Good point, updated.

Girgias

Frankly struggling to understand the fix. Comments are useful to describe what the purpose of the code and why it is being done.

Girgias · 2026-04-20T15:23:50Z

 		return SUCCESS;
 	}

+	{


Why? Why are you creating a block scope, we use C99 (heck even C11...) you can just declare a new variable.

Girgias · 2026-04-20T15:25:50Z

 	}

+	{
+		const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', strlen(fullpath) - dest_len - 1);


We have the length of fullpath already.

Suggested change

const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', strlen(fullpath) - dest_len - 1);

const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', len - dest_len - 1);

Girgias · 2026-04-20T15:27:16Z

+			if (parent_len >= MAXPATHLEN) {
+				spprintf(error, 4096, "Cannot extract \"%s\", path too long", entry->filename);
+				efree(fullpath);
+				return FAILURE;
+			}


How would this ever happen? This is derived from fullpath and this was already checked to fit within the maxpathlen.

Convert this to an assertion.

Yup, you right, fixed.

Phar::extractTo() builds the target path as dest + "/" + entry_path. CWD_EXPAND validates entry_path against ".." traversal but doesn't resolve symlinks. A pre-existing symlink in the destination (e.g., /dest/subdir -> /etc) writes the extracted file outside the intended root. After creating the parent directory, resolve it with VCWD_REALPATH and verify it stays under dest. Return FAILURE if it escapes so the caller throws a PharException.

iliaal · 2026-04-20T16:08:17Z

Addressed the comments: dropped the block scope, reused len, and converted the dead parent_len >= MAXPATHLEN check to an assert.

For the overall approach: extractTo() builds the target as dest + "/" + entry_path. CWD_EXPAND sanitizes .. inside the entry path, but it doesn't resolve symlinks already sitting in dest. So if the caller's destination contains a pre-existing subdir -> /etc symlink, the write follows it out of the intended root.

The patch resolves the parent directory of the target with VCWD_REALPATH right before php_stream_open_wrapper and rejects extraction when the resolved parent doesn't stay under the resolved dest. ext/phar/tests/gh-ss008-extractto-symlink.phpt covers that case.

github-actions bot added the Extension: phar label Apr 18, 2026

iliaal mentioned this pull request Apr 18, 2026

phar: reject extractTo() when destination parent escapes via symlink iliaal/php-src#43

Closed

LamentXU123 reviewed Apr 19, 2026

View reviewed changes

iliaal force-pushed the fix/phar-extractto-symlink-traversal branch from 5029b6a to e2a8b21 Compare April 19, 2026 11:56

Girgias requested changes Apr 20, 2026

View reviewed changes

iliaal force-pushed the fix/phar-extractto-symlink-traversal branch from e2a8b21 to 0ff1ded Compare April 20, 2026 16:03

	const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', strlen(fullpath) - dest_len - 1);
	const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', len - dest_len - 1);

Conversation

iliaal commented Apr 18, 2026

Uh oh!

LamentXU123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliaal commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants