Fix GH-21682: Add NOT_SERIALIZABLE to ZipArchive

[iliaal]

Part of #21682, split from #21694 after review feedback.

ZipArchive wraps a libzip archive handle that can't survive serialization. serialize() produces a string, but unserializing it returns an object where numFiles reports 0 and the archive contents are gone.

Adds @not-serializable so serialize() throws instead of producing a broken object.

The UAF exploit test bug72434.phpt instantiated ZipArchive via unserialize(). With NOT_SERIALIZABLE, unserialize() rejects the class and closes the UAF vector. The test also moves to ext/zip/tests/ since it exercises ZipArchive.

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.

[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().