Skip to content

Commit 657f0d6

Browse files
LamentXU123LamentXU123
committed
ext/phar: Fix ZIP extra field length underflow (#22330)
Validate each ZIP extra field header before consuming its payload. The old parser kept the remaining extra field length in a uint16_t and subtracted the declared payload size plus the header size without first checking that the field fit inside the remaining extra data. A malformed ZIP central directory entry could therefore underflow the counter and make the parser continue into following bytes, such as the file comment. That allowed comment bytes to be interpreted as another extra field and update metadata like the entry mtime. Reject truncated extra headers and oversized payloads, keep the remaining length in size_t while parsing, and check seeks that skip unknown or unused field data. Add a regression test that builds a malformed ZIP and expects PharData to reject it. Closes #22330
1 parent 100938b commit 657f0d6

3 files changed

Lines changed: 129 additions & 15 deletions

File tree

‎NEWS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ PHP NEWS
6565
. Fixed a bypass of the magic ".phar" directory protection in
6666
Phar::addEmptyDir() for paths starting with "/.phar", while allowing
6767
non-magic directory names that merely share the ".phar" prefix. (Weilin Du)
68+
. Fixed an integer underflow when parsing ZIP extra fields. (Weilin Du)
6869

6970
- Reflection:
7071
. Preserve class-name case in ReflectionClass::getProperty() error messages

‎ext/phar/tests/zip/zip_extra_underflow.phpt‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
--TEST--
2+
Phar: ZIP extra field length must not underflow
3+
--EXTENSIONS--
4+
phar
5+
--FILE--
6+
<?php
7+
function uint16($value) {
8+
return pack('v', $value);
9+
}
10+
11+
function uint32($value) {
12+
return pack('V', $value);
13+
}
14+
15+
$filename = __DIR__ . '/zip_extra_underflow.zip';
16+
$entry = 'test.txt';
17+
$contents = 'hello';
18+
$crc = crc32($contents);
19+
20+
$local = uint32(0x04034b50)
21+
. uint16(20)
22+
. uint16(0)
23+
. uint16(0)
24+
. uint16(0)
25+
. uint16(0)
26+
. uint32($crc)
27+
. uint32(strlen($contents))
28+
. uint32(strlen($contents))
29+
. uint16(strlen($entry))
30+
. uint16(0)
31+
. $entry
32+
. $contents;
33+
34+
$extra = 'XX' . uint16(1);
35+
36+
/* Old code seeks one byte past the extra field and parses this as another extra header. */
37+
$commentPrefix = 'A'
38+
. 'UT'
39+
. uint16(5)
40+
. "\x01"
41+
. uint32(946684800)
42+
. 'ZZ'
43+
. uint16(65522);
44+
$comment = $commentPrefix . str_repeat('B', 65535 - strlen($commentPrefix));
45+
46+
$central = uint32(0x02014b50)
47+
. uint16(20)
48+
. uint16(20)
49+
. uint16(0)
50+
. uint16(0)
51+
. uint16(0)
52+
. uint16(0)
53+
. uint32($crc)
54+
. uint32(strlen($contents))
55+
. uint32(strlen($contents))
56+
. uint16(strlen($entry))
57+
. uint16(strlen($extra))
58+
. uint16(strlen($comment))
59+
. uint16(0)
60+
. uint16(0)
61+
. uint32(0)
62+
. uint32(0)
63+
. $entry
64+
. $extra
65+
. $comment;
66+
67+
$eocd = uint32(0x06054b50)
68+
. uint16(0)
69+
. uint16(0)
70+
. uint16(1)
71+
. uint16(1)
72+
. uint32(strlen($central))
73+
. uint32(strlen($local))
74+
. uint16(0);
75+
76+
file_put_contents($filename, $local . $central . $eocd);
77+
78+
try {
79+
$phar = new PharData($filename);
80+
echo "Loaded corrupt ZIP\n";
81+
echo $phar[$entry]->getMTime(), "\n";
82+
} catch (Exception $e) {
83+
echo $e->getMessage(), "\n";
84+
}
85+
?>
86+
--CLEAN--
87+
<?php
88+
@unlink(__DIR__ . '/zip_extra_underflow.zip');
89+
?>
90+
--EXPECTF--
91+
phar error: Unable to process extra field header for file in central directory in zip-based phar "%szip_extra_underflow.zip"

‎ext/phar/zip.c‎

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -41,19 +41,30 @@ static inline void phar_write_16(char buffer[2], uint32_t value)
4141
# define PHAR_SET_32(var, value) phar_write_32(var, (uint32_t) (value));
4242
# define PHAR_SET_16(var, value) phar_write_16(var, (uint16_t) (value));
4343

44-
static int phar_zip_process_extra(php_stream *fp, phar_entry_info *entry, uint16_t len) /* {{{ */
44+
static int phar_zip_process_extra(php_stream *fp, phar_entry_info *entry, uint16_t extra_len) /* {{{ */
4545
{
4646
union {
4747
phar_zip_extra_field_header header;
4848
phar_zip_unix3 unix3;
4949
phar_zip_unix_time time;
5050
} h;
51+
size_t len = extra_len;
5152
size_t read;
5253

53-
do {
54+
while (len) {
55+
size_t header_size;
56+
57+
if (len < sizeof(h.header)) {
58+
return FAILURE;
59+
}
5460
if (sizeof(h.header) != php_stream_read(fp, (char *) &h.header, sizeof(h.header))) {
5561
return FAILURE;
5662
}
63+
len -= sizeof(h.header);
64+
header_size = PHAR_GET_16(h.header.size);
65+
if (header_size > len) {
66+
return FAILURE;
67+
}
5768

5869
if (h.header.tag[0] == 'U' && h.header.tag[1] == 'T') {
5970
/* Unix timestamp header found.
@@ -62,7 +73,6 @@ static int phar_zip_process_extra(php_stream *fp, phar_entry_info *entry, uint16
6273
* We only store the modification time in the entry, so only read that.
6374
*/
6475
const size_t min_size = 5;
65-
uint16_t header_size = PHAR_GET_16(h.header.size);
6676
if (header_size >= min_size) {
6777
read = php_stream_read(fp, &h.time.flags, min_size);
6878
if (read != min_size) {
@@ -73,36 +83,48 @@ static int phar_zip_process_extra(php_stream *fp, phar_entry_info *entry, uint16
7383
entry->timestamp = PHAR_GET_32(h.time.time);
7484
}
7585

76-
len -= header_size + 4;
77-
7886
/* Consume remaining bytes */
79-
if (header_size != read) {
80-
php_stream_seek(fp, header_size - read, SEEK_CUR);
87+
if (header_size != read && -1 == php_stream_seek(fp, header_size - read, SEEK_CUR)) {
88+
return FAILURE;
8189
}
90+
len -= header_size;
8291
continue;
8392
}
8493
/* Fallthrough to next if to skip header */
8594
}
8695

8796
if (h.header.tag[0] != 'n' || h.header.tag[1] != 'u') {
8897
/* skip to next header */
89-
php_stream_seek(fp, PHAR_GET_16(h.header.size), SEEK_CUR);
90-
len -= PHAR_GET_16(h.header.size) + 4;
98+
if (header_size && -1 == php_stream_seek(fp, header_size, SEEK_CUR)) {
99+
return FAILURE;
100+
}
101+
len -= header_size;
91102
continue;
92103
}
93104

94105
/* unix3 header found */
95-
read = php_stream_read(fp, (char *) &(h.unix3.crc32), sizeof(h.unix3) - sizeof(h.header));
96-
len -= read + 4;
106+
size_t unix3_size = sizeof(h.unix3) - sizeof(h.header);
107+
size_t field_size = header_size;
108+
if (field_size == unix3_size - sizeof(h.unix3.crc32)) {
109+
/* Some archives omit the CRC32 from the unix3 size field. */
110+
field_size = unix3_size;
111+
}
112+
if (field_size < unix3_size || field_size > len) {
113+
return FAILURE;
114+
}
97115

98-
if (sizeof(h.unix3) - sizeof(h.header) != read) {
116+
read = php_stream_read(fp, (char *) &(h.unix3.crc32), unix3_size);
117+
if (unix3_size != read) {
99118
return FAILURE;
100119
}
101120

102-
if (PHAR_GET_16(h.unix3.size) > sizeof(h.unix3) - 4) {
121+
if (field_size > unix3_size) {
103122
/* skip symlink filename - we may add this support in later */
104-
php_stream_seek(fp, PHAR_GET_16(h.unix3.size) - sizeof(h.unix3.size), SEEK_CUR);
123+
if (-1 == php_stream_seek(fp, field_size - unix3_size, SEEK_CUR)) {
124+
return FAILURE;
125+
}
105126
}
127+
len -= field_size;
106128

107129
/* set permissions */
108130
entry->flags &= PHAR_ENT_COMPRESSION_MASK;
@@ -113,7 +135,7 @@ static int phar_zip_process_extra(php_stream *fp, phar_entry_info *entry, uint16
113135
entry->flags |= PHAR_GET_16(h.unix3.perms) & PHAR_ENT_PERM_MASK;
114136
}
115137

116-
} while (len);
138+
}
117139

118140
return SUCCESS;
119141
}

0 commit comments

Comments
 (0)