From bf6cba4a8d31d0b4b65181fc3c19b780ef570c0f Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 4 Nov 2024 21:18:37 +0100 Subject: [PATCH] Fix GH-16695: phar:// tar parser and zero-length file header blocks There are two issues: 1) There's an off-by-one in the check for the minimum file size for a tar (i.e. `>` instead of `>=`). 2) The loop in the tar parsing parses a header, and then unconditionally reads the next one. However, that doesn't necessarily exist. Instead, we remove the loop condition and check for the end of the file before reading the next header. Note that we can't use php_stream_eof as the flag may not be set yet when we're already at the end. --- ext/phar/phar.c | 2 +- ext/phar/tar.c | 10 +++++++--- ext/phar/tests/tar/gh16695_1.phpt | 28 ++++++++++++++++++++++++++++ ext/phar/tests/tar/gh16695_2.phpt | 26 ++++++++++++++++++++++++++ ext/phar/tests/tar/gh16695_3.phpt | 26 ++++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 ext/phar/tests/tar/gh16695_1.phpt create mode 100644 ext/phar/tests/tar/gh16695_2.phpt create mode 100644 ext/phar/tests/tar/gh16695_3.phpt diff --git a/ext/phar/phar.c b/ext/phar/phar.c index e3d6ea74c618..01a3d5462547 100644 --- a/ext/phar/phar.c +++ b/ext/phar/phar.c @@ -1774,7 +1774,7 @@ static int phar_open_from_fp(php_stream* fp, char *fname, size_t fname_len, char return phar_parse_zipfile(fp, fname, fname_len, alias, alias_len, pphar, error); } - if (got > 512) { + if (got >= 512) { if (phar_is_tar(pos, fname)) { php_stream_rewind(fp); return phar_parse_tarfile(fp, fname, fname_len, alias, alias_len, pphar, is_data, compression, error); diff --git a/ext/phar/tar.c b/ext/phar/tar.c index 60e248c78df5..652062679d70 100644 --- a/ext/phar/tar.c +++ b/ext/phar/tar.c @@ -254,9 +254,8 @@ int phar_parse_tarfile(php_stream* fp, char *fname, size_t fname_len, char *alia entry.is_tar = 1; entry.is_crc_checked = 1; entry.phar = myphar; - pos += sizeof(buf); - do { + while (true) { phar_entry_info *newentry; pos = php_stream_tell(fp); @@ -597,6 +596,11 @@ int phar_parse_tarfile(php_stream* fp, char *fname, size_t fname_len, char *alia } } + /* Only read next header if we're not yet at the end */ + if (php_stream_tell(fp) == totalsize) { + break; + } + read = php_stream_read(fp, buf, sizeof(buf)); if (read != sizeof(buf)) { @@ -607,7 +611,7 @@ int phar_parse_tarfile(php_stream* fp, char *fname, size_t fname_len, char *alia phar_destroy_phar_data(myphar); return FAILURE; } - } while (!php_stream_eof(fp)); + } if (zend_hash_str_exists(&(myphar->manifest), ".phar/stub.php", sizeof(".phar/stub.php")-1)) { myphar->is_data = 0; diff --git a/ext/phar/tests/tar/gh16695_1.phpt b/ext/phar/tests/tar/gh16695_1.phpt new file mode 100644 index 000000000000..8ce82bcf28dd --- /dev/null +++ b/ext/phar/tests/tar/gh16695_1.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-16695 (phar:// tar parser and zero-length file header blocks) +--CREDITS-- +hakre +--EXTENSIONS-- +phar +--INI-- +phar.require_hash=0 +--FILE-- + +--CLEAN-- + +--EXPECTF-- +int(512) + +Warning: file_get_contents(%stls): Failed to open stream: phar error: path "tls" is a directory in %s on line %d +bool(false) diff --git a/ext/phar/tests/tar/gh16695_2.phpt b/ext/phar/tests/tar/gh16695_2.phpt new file mode 100644 index 000000000000..5b7200398c49 --- /dev/null +++ b/ext/phar/tests/tar/gh16695_2.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-16695 (phar:// tar parser and zero-length file header blocks) +--CREDITS-- +hakre +--EXTENSIONS-- +phar +--INI-- +phar.require_hash=0 +--FILE-- + +--CLEAN-- + +--EXPECT-- +int(1024) +string(122) "{"Name":"default","Metadata":{},"Endpoints":{"docker":{"Host":"unix:///run/user/1000/docker.sock","SkipTLSVerify":false}}}" diff --git a/ext/phar/tests/tar/gh16695_3.phpt b/ext/phar/tests/tar/gh16695_3.phpt new file mode 100644 index 000000000000..eddf697b0137 --- /dev/null +++ b/ext/phar/tests/tar/gh16695_3.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-16695 (phar:// tar parser and zero-length file header blocks) +--CREDITS-- +hakre +--EXTENSIONS-- +phar +--INI-- +phar.require_hash=0 +--FILE-- + +--CLEAN-- + +--EXPECT-- +int(512) +string(0) ""