Fix GH-21682: reject ZipArchive serialization via __serialize()

[iliaal]

ZipArchive wraps a libzip handle that can't survive serialization: it holds uncommitted in-memory changes and may be file-backed, so the implicit serialize()/unserialize() returned a broken object (numFiles 0) and was the GH-72434 use-after-free vector.

This adds throwing __serialize()/__unserialize() on the base, rejecting both paths while letting a subclass override them to round-trip through closeString()/openString(). The bug72434 reproducer moves to ext/zip/tests/ and now asserts the unserialize() rejection.

[DanielEScherzer]

Marking as "request changes" so that this doesn't merge until we can evaluate the impact of #21497

[DanielEScherzer]

Per the discussion on #21694, ZipArchive::closeString() (in flight as #21497) gives subclasses a way to capture archive state, so this decision is worth revisiting once that lands.

@iliaal does this change things? Specifically, you should now be able to round-trip by serializing with closeString() and unserializing with openString()

CC @tstarling

[iliaal]

closeString()/openString() are an explicit, manual round-trip; they don't touch serialize(). The flag blocks the implicit serialize(), which still returns a broken object (numFiles 0) and still feeds the bug72434 UAF. That's unchanged by closeString existing.

Until ZipArchive actually implements serialization (and handles file-backed archives, which have no string export), the broken object and the UAF are real and the serialize/unserialize block is still needed. The flag and __serialize are mutually exclusive, so whoever implements that later just drops the flag. I'd add it now.

[tstarling]

I think the change is still appropriate. ZipArchive holds uncommitted changes to the underlying file. If serialization was implemented then it would have to restore the state including uncommitted changes, as distinct from the archive file contents. For example ZipArchive::unchangeAll() makes this distinction visible to the user.

[DanielEScherzer]

My concern is that ZEND_ACC_NOT_SERIALIZABLE also blocks the serialization of any subclasses, e.g. https://3v4l.org/SVqho#v8.5.7

And, now that we have openString() and closeString(), it is possible to do a userland serialization with those:

<?php

$orig;

class MyArchive extends ZipArchive {
    public function __serialize(): array {
		global $orig;
		$orig = $this->closeString();
		return [ 'data' => $orig ];
    }
    
    public function __unserialize(array $data): void {
        $this->openString($data['data']);
    }
}

$zip = new MyArchive();
$zip->openString();
$zip->addFromString('test1', 'abc123');

var_dump($zip);
$serialized = serialize($zip);
$roundtrip = unserialize($serialized);

var_dump($roundtrip);

$new = $roundtrip->closeString();

var_dump($orig, $new, $orig === $new); // they are indeed the same

Maybe instead add __serialize() and __unserialize() handlers that always throw, saying they need to be overridden?

[iliaal]

Switched to throwing __serialize()/__unserialize(): the base still rejects serialize/unserialize and still closes the bug72434 UAF, since a throwing __unserialize means the payload is never injected as raw properties, while a subclass can override both to round-trip through closeString()/openString().

[DanielEScherzer]

Needs news, and PR title is no longer correct, but implementation looks good

please also add a test case for a subclass of ZipArchive that does not override the serialization functions

[iliaal]

Added the NEWS entry and a test for a non-overriding subclass (it inherits the base throw, with the subclass name in the message). Retitled and updated the description to match the __serialize()/__unserialize() approach.

[DanielEScherzer]

Looks good, though not sure if this should be in NEWS or UPGRADING (I didn't remember that this was for master when reviewing earlier). Either way the code and tests are good

[iliaal]

Since this is new capability I am inclined to say in belongs in NEWS as opposed to changing how something works per UPGRADING