From 7a924a8d98b127308144c32fc728cc877e2d293f Mon Sep 17 00:00:00 2001 From: James Date: Fri, 28 Aug 2026 09:29:54 +0100 Subject: [PATCH 1/6] Check the declared payload length against the bytes present before allocating Owid::fromReader read the payload through Io::readByteArray, which takes the sender's declared count and slices that many bytes from the buffer. Io::readBytes already refused a count beyond the end of the buffer before slicing, so this port never allocated by the declared number, but nothing checked that the count left exactly the 64 byte signature after the payload. A count short of the payload let payload bytes be read as the signature, and bytes after the signature were ignored, so malformed OWIDs that the reference fix in owid-dotnet now refuses still parsed here. The count is now checked before anything is sized by it. A valid OWID is the declared payload followed by the 64 byte signature and nothing else, so the new Io::readPayload requires the count to equal the bytes remaining less the signature length, and any other count, short or long, is refused with the existing exception type through the new OwidException::payloadLengthMismatch, which names the declared length and the bytes present. Owid::fromReader reads the payload through readPayload, so an envelope with a byte after the signature, previously ignored, or a signature shorter than 64 bytes is refused as malformed. Io::readByteArray is unchanged because it is already bounded by readBytes and is not tied to the signature, and its comment now says so. The domain terminator scan uses strpos, which stops at the end of the buffer, and the date reads go through the bounded readBytes, so no other count driven read needed a change. tests/PayloadLengthTest.php covers a matching envelope, the library's own signed output, off by one counts, a trailing byte, a short signature, declared lengths of 64 MiB, 2 GiB and 0xFFFFFFFF each refused 1,000 times inside a second and with under 64 KiB of peak memory where PHP can measure it, and an empty payload. tests/run.php, the dependency free fallback runner, carries the same checks. --- src/Io.php | 28 ++++- src/Owid.php | 20 ++-- src/OwidException.php | 17 +++ tests/PayloadLengthTest.php | 230 ++++++++++++++++++++++++++++++++++++ tests/run.php | 52 ++++++++ 5 files changed, 339 insertions(+), 8 deletions(-) create mode 100644 tests/PayloadLengthTest.php diff --git a/src/Io.php b/src/Io.php index c66e939..8de88e9 100644 --- a/src/Io.php +++ b/src/Io.php @@ -124,7 +124,10 @@ public function readUint32(): int /** * Reads a byte array prefixed with its length as an unsigned 32 bit - * integer. + * integer. The count is bounded by the bytes present in readBytes, so + * nothing is sized by the declared number alone. The OWID payload is + * read with readPayload instead, because the payload must also be + * followed by exactly the signature. * * @throws OwidException when the buffer is too short. */ @@ -134,6 +137,29 @@ public function readByteArray(): string return $this->readBytes($count); } + /** + * Reads the length prefixed payload of an OWID, which must be followed + * by the signature and nothing else. The count is whatever the sender + * declared, so it is checked against the bytes actually present before + * anything is sized by it. The count must equal the bytes remaining + * less the signature length, and any other count, short or long, is + * refused. A byte after the signature or a signature shorter than 64 + * bytes therefore fails here rather than being ignored or failing + * later. + * + * @throws OwidException when the declared length does not leave exactly + * the signature after the payload. + */ + public function readPayload(): string + { + $count = $this->readUint32(); + $present = strlen($this->buffer) - $this->position; + if ($count + OwidException::SIGNATURE_LENGTH !== $present) { + throw OwidException::payloadLengthMismatch($count, $present); + } + return $this->readBytes($count); + } + /** * Reads the fixed length signature. * diff --git a/src/Owid.php b/src/Owid.php index 6e29f42..362b661 100644 --- a/src/Owid.php +++ b/src/Owid.php @@ -83,10 +83,13 @@ public static function fromBase64(string $value): self } /** - * Creates an OWID from its binary form. + * Creates an OWID from its binary form. The declared payload length must + * leave exactly the signature after the payload, so a buffer with bytes + * missing or bytes after the signature is refused. * - * @throws OwidException when the version is unknown or the buffer is too - * short for the remaining fields. + * @throws OwidException when the version is unknown, the buffer is too + * short for the remaining fields, or the declared + * payload length does not match the bytes present. */ public static function fromByteArray(string $buffer): self { @@ -94,10 +97,13 @@ public static function fromByteArray(string $buffer): self } /** - * Creates an OWID by reading the next fields from the reader. + * Creates an OWID by reading the next fields from the reader. The reader + * must end with the signature, because the declared payload length is + * checked against the bytes remaining before the payload is read. * - * @throws OwidException when the version is unknown or the buffer is too - * short. + * @throws OwidException when the version is unknown, the buffer is too + * short, or the declared payload length does not + * match the bytes present. */ public static function fromReader(Io $reader): self { @@ -109,7 +115,7 @@ public static function fromReader(Io $reader): self } $owid->domain = $reader->readString(); $owid->date = $reader->readDate($version); - $owid->payload = $reader->readByteArray(); + $owid->payload = $reader->readPayload(); $owid->signature = $reader->readSignature(); return $owid; } diff --git a/src/OwidException.php b/src/OwidException.php index 326585b..6d14192 100644 --- a/src/OwidException.php +++ b/src/OwidException.php @@ -89,6 +89,23 @@ public static function dateOutOfRange(): self ); } + /** + * The declared payload length does not leave exactly the signature after + * the payload. The declared value is whatever the sender wrote, so it is + * named alongside the bytes that were actually present. + */ + public static function payloadLengthMismatch( + int $declared, + int $present + ): self { + $signature = self::SIGNATURE_LENGTH; + return new self( + "OWID payload length '$declared' does not match the '$present' " . + "bytes present, of which the final '$signature' must be the " . + "signature" + ); + } + /** * The payload is larger than the unsigned 32 bit length prefix allows. */ diff --git a/tests/PayloadLengthTest.php b/tests/PayloadLengthTest.php new file mode 100644 index 0000000..bdd69c9 --- /dev/null +++ b/tests/PayloadLengthTest.php @@ -0,0 +1,230 @@ +asByte()); + Io::writeString($buffer, '51d.es'); + Io::writeUint32($buffer, 1000); + Io::writeUint32($buffer, $declaredLength); + return $buffer . $payload . $signature; + } + + private static function payload(): string + { + return str_repeat("\x5A", self::PAYLOAD_LENGTH); + } + + private static function signature( + int $length = self::SIGNATURE_LENGTH + ): string { + return str_repeat("\x99", $length); + } + + /** + * Parses the bytes expecting a refusal and returns the message, so a + * test can check what the message names. Every refusal must use the + * library's own exception type, and a parse that is accepted fails the + * test. + */ + private function refusal(string $bytes, string $label): string + { + try { + Owid::fromByteArray($bytes); + } catch (OwidException $e) { + return $e->getMessage(); + } + $this->fail("$label should have been refused"); + } + + /** + * The declared length matches the bytes present, the signature is the + * last 64 bytes, and the envelope parses to the same payload. + */ + public function testDeclaredLengthMatchesParses(): void + { + $owid = Owid::fromByteArray(self::envelope( + self::PAYLOAD_LENGTH, + self::payload(), + self::signature() + )); + $this->assertSame(self::payload(), $owid->payload); + $this->assertSame(self::signature(), $owid->signature); + $this->assertSame('51d.es', $owid->domain); + } + + /** + * A round trip through the library's own signing path still parses and + * verifies, so the check agrees with what the library itself produces. + */ + public function testLibraryOutputParses(): void + { + $crypto = Crypto::new(); + $creator = new Creator('51d.es', $crypto); + $original = $creator->signBytes(self::payload()); + $parsed = Owid::fromByteArray($original->asByteArray()); + $this->assertSame(self::payload(), $parsed->payload); + $this->assertTrue( + $parsed->verifyWithCrypto($crypto), + 'parsed copy should verify' + ); + } + + /** + * One more or one fewer than the bytes present is refused, because + * either leaves something other than exactly the signature at the end. + * The message names the declared length and the bytes present so the + * caller can see what the sender got wrong. + */ + public function testDeclaredLengthOffByOneIsRefused(): void + { + $present = self::PAYLOAD_LENGTH + self::SIGNATURE_LENGTH; + $declaredLengths = [self::PAYLOAD_LENGTH - 1, self::PAYLOAD_LENGTH + 1]; + foreach ($declaredLengths as $declared) { + $message = $this->refusal( + self::envelope($declared, self::payload(), self::signature()), + "declared $declared" + ); + $this->assertStringContainsString("'$declared'", $message); + $this->assertStringContainsString("'$present'", $message); + } + } + + /** + * A byte after the signature is refused, because the signature must be + * the end of the envelope. Before the check the extra byte was ignored. + * The message names the bytes present, which include the extra byte. + */ + public function testTrailingByteAfterSignatureIsRefused(): void + { + $bytes = self::envelope( + self::PAYLOAD_LENGTH, + self::payload(), + self::signature() + ); + $message = $this->refusal($bytes . "\x00", 'trailing byte'); + $present = self::PAYLOAD_LENGTH + self::SIGNATURE_LENGTH + 1; + $this->assertStringContainsString("'$present'", $message); + } + + /** + * A short signature is refused. The declared payload length is right for + * the payload, but the bytes after it are fewer than a signature. The + * message names the bytes present, one short of payload and signature. + */ + public function testShortSignatureIsRefused(): void + { + $bytes = self::envelope( + self::PAYLOAD_LENGTH, + self::payload(), + self::signature(self::SIGNATURE_LENGTH - 1) + ); + $message = $this->refusal($bytes, '63 byte signature'); + $present = self::PAYLOAD_LENGTH + self::SIGNATURE_LENGTH - 1; + $this->assertStringContainsString("'$present'", $message); + } + + /** + * A declared length far beyond the bytes present is refused without + * anything sized by the declared number. PHP cannot count allocations + * per call, so the envelope of a few dozen bytes that declares 64 MiB, + * then 2 GiB, then the largest unsigned 32 bit value is parsed 1,000 + * times each and must finish well inside a second, which a parse that + * sized a buffer by the declared length could not do. Where the runtime + * can reset its peak memory figure (PHP 8.2 and later) the peak during + * one refusal must also stay under 64 KiB above the level before it. + */ + public function testHugeDeclaredLengthIsRefusedQuickly(): void + { + $declaredLengths = [64 * 1024 * 1024, 0x7FFFFFFF, 0xFFFFFFFF]; + foreach ($declaredLengths as $declared) { + $bytes = self::envelope($declared, '', ''); + $message = ''; + $start = hrtime(true); + for ($attempt = 0; $attempt < 1000; $attempt++) { + $message = $this->refusal($bytes, "declared $declared"); + } + $elapsed = (hrtime(true) - $start) / 1e9; + $this->assertStringContainsString("'$declared'", $message); + $this->assertLessThan( + 1.0, + $elapsed, + "declared $declared took {$elapsed}s for 1,000 attempts" + ); + if (function_exists('memory_reset_peak_usage')) { + $before = memory_get_usage(); + memory_reset_peak_usage(); + $this->refusal($bytes, "declared $declared"); + $peak = memory_get_peak_usage() - $before; + $this->assertLessThan( + 64 * 1024, + $peak, + "declared $declared raised peak memory by $peak bytes" + ); + } + } + } + + /** + * An empty payload, declared length zero, followed by the signature is a + * valid OWID and parses. + */ + public function testEmptyPayloadParses(): void + { + $owid = Owid::fromByteArray(self::envelope(0, '', self::signature())); + $this->assertSame('', $owid->payload); + $this->assertSame(self::signature(), $owid->signature); + } +} diff --git a/tests/run.php b/tests/run.php index 65f7043..2146502 100644 --- a/tests/run.php +++ b/tests/run.php @@ -329,4 +329,56 @@ function () { Endpoints::publicKeyPath(Version::Version3) === '/owid/api/v3/public-key' ); +// Payload length. The declared length is checked against the bytes present +// before anything is sized by it, and exactly the signature must follow. +function payloadEnvelope(int $declared, string $payload, string $signature): string +{ + $buffer = ''; + Io::writeByte($buffer, Version::Version3->asByte()); + Io::writeString($buffer, '51d.es'); + Io::writeUint32($buffer, 1000); + Io::writeUint32($buffer, $declared); + return $buffer . $payload . $signature; +} +$lengthPayload = str_repeat("\x5A", 37); +$lengthSignature = str_repeat("\x99", 64); +$runner->check( + 'matching payload length parses', + Owid::fromByteArray(payloadEnvelope(37, $lengthPayload, $lengthSignature))->payload === $lengthPayload +); +$runner->check( + 'empty payload with signature parses', + Owid::fromByteArray(payloadEnvelope(0, '', $lengthSignature))->payload === '' +); +foreach ([36, 38] as $declared) { + $runner->checkThrows( + "payload length $declared off by one refused", + fn () => Owid::fromByteArray(payloadEnvelope($declared, $lengthPayload, $lengthSignature)) + ); +} +$runner->checkThrows( + 'trailing byte after signature refused', + fn () => Owid::fromByteArray(payloadEnvelope(37, $lengthPayload, $lengthSignature) . "\x00") +); +$runner->checkThrows( + '63 byte signature refused', + fn () => Owid::fromByteArray(payloadEnvelope(37, $lengthPayload, str_repeat("\x99", 63))) +); +foreach ([64 * 1024 * 1024, 0x7FFFFFFF, 0xFFFFFFFF] as $declared) { + $refused = true; + $start = hrtime(true); + for ($attempt = 0; $attempt < 1000; $attempt++) { + try { + Owid::fromByteArray(payloadEnvelope($declared, '', '')); + $refused = false; + } catch (OwidException $e) { + } + } + $elapsed = (hrtime(true) - $start) / 1e9; + $runner->check( + "declared length $declared refused 1000 times in under a second", + $refused && $elapsed < 1.0 + ); +} + exit($runner->summary()); From 84e648f98bd729438b9e177c57b3e11c09f553e1 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 28 Aug 2026 17:48:02 +0100 Subject: [PATCH 2/6] Clarify and optimize large payload handling --- README.md | 26 ++++++++++++++++++++++++++ src/Io.php | 15 ++++++++++----- tests/PayloadLengthTest.php | 30 +++++++++++++++++++++++++----- tests/run.php | 10 ++++++++++ 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 095b79d..7f373e0 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,32 @@ Fetching a creator public key over HTTP is out of scope. The `verifyWithPublicKey` method accepts a public key PEM that the caller has already obtained, so any HTTP client can supply it. +## Payload size and application limits + +The OWID wire format stores the payload length as an unsigned 32 bit value, +so a payload from zero through 4,294,967,295 bytes is structurally valid. The +format defines no smaller payload limit. The null-terminated domain has no +separate encoded maximum, so the protocol alone is not an application input +limit for the complete envelope. + +This library validates that the declared payload length agrees with the bytes +present before it extracts the payload. A large declaration without the +corresponding bytes is malformed and is rejected without allocating the +declared size. A matching large payload is not malformed merely because it is +large, and parsing work and memory use scale with the bytes actually present. + +The in-memory APIs remain subject to PHP string, platform, address-space and +available-memory limits. Applications accepting untrusted OWIDs must choose +limits suitable for their use case and enforce them before buffering the +binary form or decoding Base64. An implementation capacity failure or an +application policy rejection is distinct from an invalid OWID. + +For transport input, limit the complete HTTP body or encoded envelope; allow +for the domain and other OWID fields as well as the payload. After parsing, +`strlen($owid->payload)` reports the actual payload size without another copy +and can be used for downstream policy. The parser cannot choose either limit +on behalf of the application. + ## Installation Require the package with Composer. diff --git a/src/Io.php b/src/Io.php index 8de88e9..4b37f58 100644 --- a/src/Io.php +++ b/src/Io.php @@ -50,11 +50,13 @@ public static function baseDate(): DateTimeImmutable * buffer because each element accessed by offset is a single byte. */ private string $buffer; + private int $length; private int $position; public function __construct(string $buffer) { $this->buffer = $buffer; + $this->length = strlen($buffer); $this->position = 0; } @@ -65,7 +67,7 @@ public function __construct(string $buffer) */ public function readByte(): int { - if ($this->position >= strlen($this->buffer)) { + if ($this->position >= $this->length) { throw OwidException::unexpectedEndOfBuffer(); } $value = ord($this->buffer[$this->position]); @@ -80,7 +82,7 @@ public function readByte(): int */ public function readBytes(int $count): string { - if ($count < 0 || $this->position + $count > strlen($this->buffer)) { + if ($count < 0 || $this->position + $count > $this->length) { throw OwidException::unexpectedEndOfBuffer(); } $value = substr($this->buffer, $this->position, $count); @@ -116,9 +118,12 @@ public function readString(): string */ public function readUint32(): int { - $bytes = $this->readBytes(4); + if ($this->position + 4 > $this->length) { + throw OwidException::unexpectedEndOfBuffer(); + } /** @var array{1: int} $unpacked */ - $unpacked = unpack('V', $bytes); + $unpacked = unpack('V', $this->buffer, $this->position); + $this->position += 4; return $unpacked[1]; } @@ -153,7 +158,7 @@ public function readByteArray(): string public function readPayload(): string { $count = $this->readUint32(); - $present = strlen($this->buffer) - $this->position; + $present = $this->length - $this->position; if ($count + OwidException::SIGNATURE_LENGTH !== $present) { throw OwidException::payloadLengthMismatch($count, $present); } diff --git a/tests/PayloadLengthTest.php b/tests/PayloadLengthTest.php index bdd69c9..fad41d0 100644 --- a/tests/PayloadLengthTest.php +++ b/tests/PayloadLengthTest.php @@ -105,6 +105,24 @@ public function testDeclaredLengthMatchesParses(): void $this->assertSame('51d.es', $owid->domain); } + /** + * A payload materially larger than an ordinary identifier remains valid + * when its declaration and bytes agree. Application policy is separate + * from format validity. + */ + public function testMatchingOneMebibytePayloadParses(): void + { + $payload = str_repeat("\x5A", 1024 * 1024); + + $owid = Owid::fromByteArray(self::envelope( + strlen($payload), + $payload, + self::signature() + )); + + $this->assertSame($payload, $owid->payload); + } + /** * A round trip through the library's own signing path still parses and * verifies, so the check agrees with what the library itself produces. @@ -177,16 +195,18 @@ public function testShortSignatureIsRefused(): void } /** - * A declared length far beyond the bytes present is refused without + * A large declaration whose payload bytes are absent is refused without * anything sized by the declared number. PHP cannot count allocations * per call, so the envelope of a few dozen bytes that declares 64 MiB, - * then 2 GiB, then the largest unsigned 32 bit value is parsed 1,000 - * times each and must finish well inside a second, which a parse that - * sized a buffer by the declared length could not do. Where the runtime + * then 2 GiB, then the largest unsigned 32 bit value while carrying none + * of those bytes is parsed 1,000 times each. The numeric values remain + * valid when the matching payload is present. The attempts must finish + * well inside a second, which a parse that sized a buffer by the + * declaration could not do. Where the runtime * can reset its peak memory figure (PHP 8.2 and later) the peak during * one refusal must also stay under 64 KiB above the level before it. */ - public function testHugeDeclaredLengthIsRefusedQuickly(): void + public function testMismatchedLargeDeclarationIsRefusedQuickly(): void { $declaredLengths = [64 * 1024 * 1024, 0x7FFFFFFF, 0xFFFFFFFF]; foreach ($declaredLengths as $declared) { diff --git a/tests/run.php b/tests/run.php index 2146502..4657d12 100644 --- a/tests/run.php +++ b/tests/run.php @@ -346,6 +346,16 @@ function payloadEnvelope(int $declared, string $payload, string $signature): str 'matching payload length parses', Owid::fromByteArray(payloadEnvelope(37, $lengthPayload, $lengthSignature))->payload === $lengthPayload ); +$largeLengthPayload = str_repeat("\x5A", 1024 * 1024); +$runner->check( + 'matching one mebibyte payload parses', + Owid::fromByteArray(payloadEnvelope( + strlen($largeLengthPayload), + $largeLengthPayload, + $lengthSignature + ))->payload === $largeLengthPayload +); +unset($largeLengthPayload); $runner->check( 'empty payload with signature parses', Owid::fromByteArray(payloadEnvelope(0, '', $lengthSignature))->payload === '' From c5085d56991d76d75a05e134667c4f1f4fcb4aac Mon Sep 17 00:00:00 2001 From: James Date: Fri, 28 Aug 2026 18:13:18 +0100 Subject: [PATCH 3/6] Preserve framed OWID reader behavior --- src/Io.php | 27 +++++++++++++--------- src/Owid.php | 20 +++++++++++------ src/OwidException.php | 7 +++--- tests/PayloadLengthTest.php | 45 ++++++++++++++++++++++++------------- 4 files changed, 63 insertions(+), 36 deletions(-) diff --git a/src/Io.php b/src/Io.php index 4b37f58..98e2446 100644 --- a/src/Io.php +++ b/src/Io.php @@ -90,6 +90,15 @@ public function readBytes(int $count): string return $value; } + /** + * Returns the number of unread bytes, so a top-level decoder can require + * EOF while a framed reader can deliberately leave following data. + */ + public function remaining(): int + { + return $this->length - $this->position; + } + /** * Reads bytes until the null terminator and returns them as a string. The * terminator is consumed but not returned. @@ -132,7 +141,7 @@ public function readUint32(): int * integer. The count is bounded by the bytes present in readBytes, so * nothing is sized by the declared number alone. The OWID payload is * read with readPayload instead, because the payload must also be - * followed by exactly the signature. + * followed by the fixed-length signature. * * @throws OwidException when the buffer is too short. */ @@ -144,22 +153,20 @@ public function readByteArray(): string /** * Reads the length prefixed payload of an OWID, which must be followed - * by the signature and nothing else. The count is whatever the sender + * by the signature. The count is whatever the sender * declared, so it is checked against the bytes actually present before - * anything is sized by it. The count must equal the bytes remaining - * less the signature length, and any other count, short or long, is - * refused. A byte after the signature or a signature shorter than 64 - * bytes therefore fails here rather than being ignored or failing - * later. + * anything is sized by it. The count must leave at least the signature; + * a public reader consumes one OWID and leaves following framed bytes, + * while top-level byte-array parsing separately requires EOF. * - * @throws OwidException when the declared length does not leave exactly - * the signature after the payload. + * @throws OwidException when the declared length does not leave a + * complete signature after the payload. */ public function readPayload(): string { $count = $this->readUint32(); $present = $this->length - $this->position; - if ($count + OwidException::SIGNATURE_LENGTH !== $present) { + if ($count + OwidException::SIGNATURE_LENGTH > $present) { throw OwidException::payloadLengthMismatch($count, $present); } return $this->readBytes($count); diff --git a/src/Owid.php b/src/Owid.php index 362b661..6b9a9ae 100644 --- a/src/Owid.php +++ b/src/Owid.php @@ -83,9 +83,8 @@ public static function fromBase64(string $value): self } /** - * Creates an OWID from its binary form. The declared payload length must - * leave exactly the signature after the payload, so a buffer with bytes - * missing or bytes after the signature is refused. + * Creates an OWID from its complete binary form. Bytes missing from or + * following the envelope are refused. * * @throws OwidException when the version is unknown, the buffer is too * short for the remaining fields, or the declared @@ -93,13 +92,20 @@ public static function fromBase64(string $value): self */ public static function fromByteArray(string $buffer): self { - return self::fromReader(new Io($buffer)); + $reader = new Io($buffer); + $owid = self::fromReader($reader); + if ($reader->remaining() !== 0) { + throw new OwidException( + "OWID contains '" . $reader->remaining() . + "' bytes after the envelope" + ); + } + return $owid; } /** - * Creates an OWID by reading the next fields from the reader. The reader - * must end with the signature, because the declared payload length is - * checked against the bytes remaining before the payload is read. + * Creates an OWID by reading its next fields from the reader. Bytes after + * the signature are left for a caller that frames multiple values. * * @throws OwidException when the version is unknown, the buffer is too * short, or the declared payload length does not diff --git a/src/OwidException.php b/src/OwidException.php index 6d14192..0212503 100644 --- a/src/OwidException.php +++ b/src/OwidException.php @@ -90,7 +90,7 @@ public static function dateOutOfRange(): self } /** - * The declared payload length does not leave exactly the signature after + * The declared payload length does not leave a complete signature after * the payload. The declared value is whatever the sender wrote, so it is * named alongside the bytes that were actually present. */ @@ -100,9 +100,8 @@ public static function payloadLengthMismatch( ): self { $signature = self::SIGNATURE_LENGTH; return new self( - "OWID payload length '$declared' does not match the '$present' " . - "bytes present, of which the final '$signature' must be the " . - "signature" + "OWID payload length '$declared' exceeds the '$present' bytes " . + "present, which must also contain the '$signature' byte signature" ); } diff --git a/tests/PayloadLengthTest.php b/tests/PayloadLengthTest.php index fad41d0..d2018b6 100644 --- a/tests/PayloadLengthTest.php +++ b/tests/PayloadLengthTest.php @@ -31,8 +31,8 @@ /** * The payload length field of an OWID is whatever the sender declared, so * parsing must check it against the bytes present before sizing anything by - * it. These tests prove that a declared length that does not leave exactly - * the signature after the payload is refused, that refusing it costs nothing + * it. These tests prove that a declared length that does not leave a complete + * signature after the payload is refused, that refusing it costs nothing * sized by the declared number, and that a correctly sized envelope still * parses. The 64 byte signature is the fixed tail every valid OWID ends with. */ @@ -142,28 +142,22 @@ public function testLibraryOutputParses(): void /** * One more or one fewer than the bytes present is refused, because - * either leaves something other than exactly the signature at the end. - * The message names the declared length and the bytes present so the - * caller can see what the sender got wrong. + * either overruns the payload or leaves bytes after the top-level value. */ public function testDeclaredLengthOffByOneIsRefused(): void { - $present = self::PAYLOAD_LENGTH + self::SIGNATURE_LENGTH; $declaredLengths = [self::PAYLOAD_LENGTH - 1, self::PAYLOAD_LENGTH + 1]; foreach ($declaredLengths as $declared) { - $message = $this->refusal( + $this->refusal( self::envelope($declared, self::payload(), self::signature()), "declared $declared" ); - $this->assertStringContainsString("'$declared'", $message); - $this->assertStringContainsString("'$present'", $message); } } /** - * A byte after the signature is refused, because the signature must be - * the end of the envelope. Before the check the extra byte was ignored. - * The message names the bytes present, which include the extra byte. + * A byte after the signature is refused because a top-level decoder + * requires the signature to end the envelope. */ public function testTrailingByteAfterSignatureIsRefused(): void { @@ -172,9 +166,7 @@ public function testTrailingByteAfterSignatureIsRefused(): void self::payload(), self::signature() ); - $message = $this->refusal($bytes . "\x00", 'trailing byte'); - $present = self::PAYLOAD_LENGTH + self::SIGNATURE_LENGTH + 1; - $this->assertStringContainsString("'$present'", $message); + $this->refusal($bytes . "\x00", 'trailing byte'); } /** @@ -247,4 +239,27 @@ public function testEmptyPayloadParses(): void $this->assertSame('', $owid->payload); $this->assertSame(self::signature(), $owid->signature); } + + /** + * The public reader consumes one OWID and leaves following framed bytes; + * the byte-array entry point remains strict about EOF. + */ + public function testFromReaderLeavesFollowingEnvelopeUnread(): void + { + $firstBytes = self::envelope( + self::PAYLOAD_LENGTH, + self::payload(), + self::signature() + ); + $secondBytes = self::envelope(0, '', self::signature()); + $reader = new Io($firstBytes . $secondBytes); + + $first = Owid::fromReader($reader); + $this->assertSame(self::payload(), $first->payload); + $this->assertSame(strlen($secondBytes), $reader->remaining()); + + $second = Owid::fromReader($reader); + $this->assertSame('', $second->payload); + $this->assertSame(0, $reader->remaining()); + } } From 7c7ffd2ae654331cdb7ae40b56a38c0294421446 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 28 Aug 2026 20:38:20 +0100 Subject: [PATCH 4/6] Count expected parser refusals as assertions --- tests/PayloadLengthTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/PayloadLengthTest.php b/tests/PayloadLengthTest.php index d2018b6..1b302b2 100644 --- a/tests/PayloadLengthTest.php +++ b/tests/PayloadLengthTest.php @@ -84,6 +84,7 @@ private function refusal(string $bytes, string $label): string try { Owid::fromByteArray($bytes); } catch (OwidException $e) { + $this->addToAssertionCount(1); return $e->getMessage(); } $this->fail("$label should have been refused"); From c536c4f8955760269f7c2ae04ecaabb936b4960d Mon Sep 17 00:00:00 2001 From: James Date: Sun, 30 Aug 2026 13:24:33 +0100 Subject: [PATCH 5/6] Bound the domain read at the published maximum The domain in an OWID envelope is stored as text followed by a zero terminator, and the parse found the end of it by walking forward to that terminator. Nothing stopped the walk, so a buffer whose terminator was missing or corrupted was read all the way to its end. That is work an attacker controls, in the same class as the declared payload length this branch already checks, and it was the last unbounded read left in the envelope parse. The search now stops after 253 characters and the envelope is refused when no terminator has been found by then, which also refuses a domain longer than a domain name may be. Refusing costs the bound rather than the length of the input, because the search takes the window as an argument instead of running to the end of the buffer. RFC 1035 section 2.3.4, "Size limits", restricts the total length of a domain name, counting label octets and label length octets, to 255 octets or less. That figure is the wire format, which spends one length octet on every label and one zero octet on the root, whereas OWID stores the presentation form, the text example.com, where the dots stand in for the label length octets and the root has no text at all. Exactly two of those 255 octets therefore have no character here, so the maximum is 253. The number is held in one named constant beside the signature length, and the comment on the constant carries this reasoning so it can be checked without leaving the file. Refusals use the library exception type. A new named constructor sits alongside the existing ones because the message has to name the maximum and must not repeat the offending bytes back, of which there may be no end. Tests cover a domain of the greatest length, one character more, a buffer with no terminator at all, a run of characters filling the bound exactly, and what the library itself signs. The cost of the unterminated case is timed over two buffers sixteen times apart, so the check rests on the cost not growing with the buffer rather than on how fast the machine is. The dependency free runner in tests/run.php gains the same checks. The README no longer says the domain has no maximum. --- README.md | 11 +- src/Io.php | 28 +++-- src/OwidException.php | 28 +++++ tests/DomainLengthTest.php | 238 +++++++++++++++++++++++++++++++++++++ tests/run.php | 89 ++++++++++++++ 5 files changed, 383 insertions(+), 11 deletions(-) create mode 100644 tests/DomainLengthTest.php diff --git a/README.md b/README.md index 7f373e0..c185652 100644 --- a/README.md +++ b/README.md @@ -40,8 +40,8 @@ already obtained, so any HTTP client can supply it. The OWID wire format stores the payload length as an unsigned 32 bit value, so a payload from zero through 4,294,967,295 bytes is structurally valid. The -format defines no smaller payload limit. The null-terminated domain has no -separate encoded maximum, so the protocol alone is not an application input +format defines no smaller payload limit. The null-terminated domain carries no +length before it either, so the protocol alone is not an application input limit for the complete envelope. This library validates that the declared payload length agrees with the bytes @@ -50,6 +50,13 @@ corresponding bytes is malformed and is rejected without allocating the declared size. A matching large payload is not malformed merely because it is large, and parsing work and memory use scale with the bytes actually present. +The domain is read the same way. Because nothing declares its length, the +search for its terminator stops at the greatest number of characters a domain +name can hold, which RFC 1035 section 2.3.4 fixes for the presentation form +this library stores. A domain with no terminator, or one longer than a domain +name may be, is rejected for a cost set by that maximum rather than by the +length of the buffer. + The in-memory APIs remain subject to PHP string, platform, address-space and available-memory limits. Applications accepting untrusted OWIDs must choose limits suitable for their use case and enforce them before buffering the diff --git a/src/Io.php b/src/Io.php index 98e2446..7ce480e 100644 --- a/src/Io.php +++ b/src/Io.php @@ -101,21 +101,31 @@ public function remaining(): int /** * Reads bytes until the null terminator and returns them as a string. The - * terminator is consumed but not returned. + * terminator is consumed but not returned. The only such string in an + * OWID is the creator domain, and the terminator is whatever the sender + * wrote, so the search for it stops after the greatest number of + * characters a domain name can hold rather than running to the end of the + * buffer. A buffer with no terminator therefore costs the bound and not + * its own length. strcspn is used because it takes the window as an + * argument and so examines no more bytes than the window, whereas strpos + * would search the rest of the buffer. * - * @throws OwidException when no terminator is found. + * @throws OwidException when the domain has no terminator within the + * characters a domain name can hold, or the buffer + * ends before the terminator. */ public function readString(): string { - $terminator = strpos($this->buffer, "\0", $this->position); - if ($terminator === false) { + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $count = strcspn($this->buffer, "\0", $this->position, $maximum + 1); + if ($count > $maximum) { + throw OwidException::domainTooLong(); + } + $terminator = $this->position + $count; + if ($terminator >= $this->length) { throw OwidException::unexpectedEndOfBuffer(); } - $value = substr( - $this->buffer, - $this->position, - $terminator - $this->position - ); + $value = substr($this->buffer, $this->position, $count); $this->position = $terminator + 1; return $value; } diff --git a/src/OwidException.php b/src/OwidException.php index 0212503..511b076 100644 --- a/src/OwidException.php +++ b/src/OwidException.php @@ -34,6 +34,18 @@ final class OwidException extends Exception */ public const SIGNATURE_LENGTH = 64; + /** + * The greatest number of characters an OWID domain can hold. RFC 1035 + * section 2.3.4, "Size limits", restricts the total length of a domain + * name, counting label octets and label length octets, to 255 octets or + * less. That 255 is the wire format, which spends one length octet on + * every label and one zero octet on the root, whereas OWID stores the + * presentation form, the text "example.com", where the dots stand in for + * the label length octets and the root has no text at all, so exactly + * two of those 255 octets have no character here and the limit is 253. + */ + public const MAXIMUM_DOMAIN_LENGTH = 253; + /** * The version byte is not one supported by this implementation. */ @@ -79,6 +91,22 @@ public static function invalidDomain(string $domain): self return new self("domain '$domain' is not valid"); } + /** + * The domain field has no terminator within the greatest number of + * characters a domain name can hold, so either the terminator is missing + * or the domain is longer than a domain name may be. The bytes are not + * named because they are whatever the sender wrote and there may be no + * end to them. + */ + public static function domainTooLong(): self + { + $maximum = self::MAXIMUM_DOMAIN_LENGTH; + return new self( + "OWID domain has no terminator within the '$maximum' characters " . + "a domain name can hold" + ); + } + /** * The date can not be represented in the encoding used by the version. */ diff --git a/tests/DomainLengthTest.php b/tests/DomainLengthTest.php new file mode 100644 index 0000000..e64c360 --- /dev/null +++ b/tests/DomainLengthTest.php @@ -0,0 +1,238 @@ + 64) { + $labels[] = str_repeat('a', 63); + $remaining -= 64; + } + $labels[] = str_repeat('a', $remaining); + $domain = implode('.', $labels); + if (strlen($domain) !== $length) { + throw new OwidException('test domain built to the wrong length'); + } + return $domain; + } + + /** + * A version 3 envelope carrying the domain given, followed by a date, an + * empty payload and the signature. + */ + private static function envelope(string $domain): string + { + $buffer = ''; + Io::writeByte($buffer, Version::Version3->asByte()); + Io::writeString($buffer, $domain); + Io::writeUint32($buffer, 1000); + Io::writeUint32($buffer, 0); + return $buffer . str_repeat("\x99", self::SIGNATURE_LENGTH); + } + + /** + * Parses the bytes expecting a refusal and returns the message, so a test + * can check what the message names. Every refusal must use the library's + * own exception type, and a parse that is accepted fails the test. + */ + private function refusal(string $bytes, string $label): string + { + try { + Owid::fromByteArray($bytes); + } catch (OwidException $e) { + $this->addToAssertionCount(1); + return $e->getMessage(); + } + $this->fail("$label should have been refused"); + } + + /** + * A domain of the greatest length a domain name can hold parses, and the + * envelope round trips back to the same bytes. + */ + public function testMaximumLengthDomainParses(): void + { + $domain = self::domain(OwidException::MAXIMUM_DOMAIN_LENGTH); + $bytes = self::envelope($domain); + + $owid = Owid::fromByteArray($bytes); + + $this->assertSame($domain, $owid->domain); + $this->assertSame( + OwidException::MAXIMUM_DOMAIN_LENGTH, + strlen($owid->domain) + ); + $this->assertSame($bytes, $owid->asByteArray()); + } + + /** + * One character more than a domain name can hold is refused, even though + * the terminator is present, because the parse stops at the bound. + */ + public function testDomainOneOverMaximumIsRefused(): void + { + $domain = self::domain(OwidException::MAXIMUM_DOMAIN_LENGTH + 1); + + $message = $this->refusal(self::envelope($domain), 'over long domain'); + + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $this->assertStringContainsString("'$maximum'", $message); + } + + /** + * Returns the seconds taken to refuse the bytes the number of times + * given, and puts the last refusal message into the reference given. + */ + private function timeRefusals( + string $bytes, + int $attempts, + string &$message + ): float { + $start = hrtime(true); + for ($attempt = 0; $attempt < $attempts; $attempt++) { + $message = $this->refusal($bytes, 'unterminated domain'); + } + return (hrtime(true) - $start) / 1e9; + } + + /** + * A buffer whose domain field has no terminator at all is refused for a + * cost that does not grow with the buffer. PHP cannot count the bytes a + * single call examines, so the proof is the time taken, and it is taken + * twice over buffers sixteen times apart so the result does not depend on + * how fast the machine is. A search running to the end of the buffer + * costs sixteen times as much on the larger one, whereas a search + * stopping at the bound costs the same on both, so the larger is required + * to stay inside four times the smaller. The small allowance added to + * that limit absorbs timer noise, because at the bound both runs take + * only a few thousandths of a second. A plain ceiling on the larger run + * is kept as well, in the style of the payload length checks. The peak + * memory during one refusal is held to 64 KiB above the level before it, + * where the runtime can reset its peak figure, which is PHP 8.2 and + * later, so nothing is sized by the run of characters either. + */ + public function testUnterminatedDomainIsRefusedWithoutReadingTheBuffer(): void + { + $prefix = chr(Version::Version3->asByte()); + $small = $prefix . str_repeat('a', 1024 * 1024); + $large = $prefix . str_repeat('a', 16 * 1024 * 1024); + $message = ''; + + $smallSeconds = $this->timeRefusals($small, 1000, $message); + $largeSeconds = $this->timeRefusals($large, 1000, $message); + + $this->assertLessThan( + 4 * $smallSeconds + 0.05, + $largeSeconds, + "sixteen times the buffer took {$largeSeconds}s against " . + "{$smallSeconds}s, so the cost grows with the buffer" + ); + $this->assertLessThan( + 1.0, + $largeSeconds, + "unterminated domain took {$largeSeconds}s for 1,000 attempts" + ); + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $this->assertStringContainsString("'$maximum'", $message); + if (function_exists('memory_reset_peak_usage')) { + $before = memory_get_usage(); + memory_reset_peak_usage(); + $this->refusal($large, 'unterminated domain'); + $peak = memory_get_peak_usage() - $before; + $this->assertLessThan( + 64 * 1024, + $peak, + "unterminated domain raised peak memory by $peak bytes" + ); + } + } + + /** + * A buffer holding only the version byte and characters filling the bound + * exactly, with no terminator after them, is refused rather than read as + * a domain, because the terminator has to be present within the bound and + * not merely absent from it. + */ + public function testDomainFillingTheBoundWithNoTerminatorIsRefused(): void + { + $bytes = chr(Version::Version3->asByte()) . + str_repeat('a', OwidException::MAXIMUM_DOMAIN_LENGTH); + + $this->refusal($bytes, 'domain filling the bound'); + } + + /** + * What the library itself signs still parses and still verifies, with an + * ordinary domain and with one of the greatest length, so the bound is + * not retrospective on anything real. + */ + public function testLibraryOutputParses(): void + { + $domains = [ + '51d.es', + self::domain(OwidException::MAXIMUM_DOMAIN_LENGTH), + ]; + foreach ($domains as $domain) { + $crypto = Crypto::new(); + $creator = new Creator($domain, $crypto); + $original = $creator->signString('value'); + + $parsed = Owid::fromByteArray($original->asByteArray()); + + $this->assertSame($domain, $parsed->domain); + $this->assertTrue( + $parsed->verifyWithCrypto($crypto), + "parsed copy of '$domain' should verify" + ); + } + } +} diff --git a/tests/run.php b/tests/run.php index 4657d12..8a52207 100644 --- a/tests/run.php +++ b/tests/run.php @@ -391,4 +391,93 @@ function payloadEnvelope(int $declared, string $payload, string $signature): str ); } +// Domain length. The zero terminator is whatever the sender wrote, so the +// search for it stops at the greatest number of characters a domain name can +// hold rather than running to the end of the buffer. +function domainOfLength(int $length): string +{ + $labels = []; + $remaining = $length; + while ($remaining > 64) { + $labels[] = str_repeat('a', 63); + $remaining -= 64; + } + $labels[] = str_repeat('a', $remaining); + return implode('.', $labels); +} +function domainEnvelope(string $domain): string +{ + $buffer = ''; + Io::writeByte($buffer, Version::Version3->asByte()); + Io::writeString($buffer, $domain); + Io::writeUint32($buffer, 1000); + Io::writeUint32($buffer, 0); + return $buffer . str_repeat("\x99", 64); +} +$maximumDomain = domainOfLength(OwidException::MAXIMUM_DOMAIN_LENGTH); +$maximumBytes = domainEnvelope($maximumDomain); +$maximumOwid = Owid::fromByteArray($maximumBytes); +$runner->check( + 'domain of the greatest length parses', + $maximumOwid->domain === $maximumDomain +); +$runner->check( + 'domain of the greatest length round trips byte exact', + $maximumOwid->asByteArray() === $maximumBytes +); +$runner->checkThrows( + 'domain one character over the greatest length refused', + fn () => Owid::fromByteArray( + domainEnvelope(domainOfLength(OwidException::MAXIMUM_DOMAIN_LENGTH + 1)) + ) +); +$runner->checkThrows( + 'domain filling the bound with no terminator refused', + fn () => Owid::fromByteArray( + chr(Version::Version3->asByte()) . + str_repeat('a', OwidException::MAXIMUM_DOMAIN_LENGTH) + ) +); +// The cost of a buffer with no terminator is timed over two buffers sixteen +// times apart, so the result does not depend on how fast the machine is. A +// search running to the end of the buffer costs sixteen times as much on the +// larger one, whereas a search stopping at the bound costs the same on both. +// The small allowance absorbs timer noise, because at the bound both runs +// take only a few thousandths of a second. +function timeDomainRefusals(string $bytes, int $attempts, bool &$refused): float +{ + $start = hrtime(true); + for ($attempt = 0; $attempt < $attempts; $attempt++) { + try { + Owid::fromByteArray($bytes); + $refused = false; + } catch (OwidException $e) { + } + } + return (hrtime(true) - $start) / 1e9; +} +$versionPrefix = chr(Version::Version3->asByte()); +$smallUnterminated = $versionPrefix . str_repeat('a', 1024 * 1024); +$largeUnterminated = $versionPrefix . str_repeat('a', 16 * 1024 * 1024); +$refusedUnterminated = true; +$smallSeconds = timeDomainRefusals($smallUnterminated, 1000, $refusedUnterminated); +$largeSeconds = timeDomainRefusals($largeUnterminated, 1000, $refusedUnterminated); +$runner->check( + 'unterminated domain refused for a cost that does not grow with the buffer', + $refusedUnterminated && $largeSeconds < 4 * $smallSeconds + 0.05 +); +$runner->check( + 'unterminated domain refused 1000 times in under a second', + $refusedUnterminated && $largeSeconds < 1.0 +); +unset($smallUnterminated, $largeUnterminated); +$maximumCrypto = Crypto::new(); +$maximumSigned = (new Creator($maximumDomain, $maximumCrypto))->signString('value'); +$maximumParsed = Owid::fromByteArray($maximumSigned->asByteArray()); +$runner->check( + 'signed OWID with the greatest length domain parses and verifies', + $maximumParsed->domain === $maximumDomain && + $maximumParsed->verifyWithCrypto($maximumCrypto) +); + exit($runner->summary()); From b78ab126b6c01859bfe474ae258169f62a8930e2 Mon Sep 17 00:00:00 2001 From: James Date: Sun, 30 Aug 2026 13:45:29 +0100 Subject: [PATCH 6/6] Refuse to write a domain longer than the maximum The read was bounded at the greatest number of characters a domain name can hold but the write was not, so a creator configured with a longer domain still produced an OWID that this same library refused to parse, and the fault landed on whoever read it rather than on the creator that caused it. The domain is now refused at two points. The Creator constructor checks it first, which is the earliest point the caller can be told and is before the crypto instance is looked at, so nothing is ever signed with a domain that could not be read back. Io::writeString checks it again, so a domain that reached the public Owid domain field by some other route is still refused, and that refusal happens while the data to sign is being built rather than after a signature has been calculated. Both points raise OwidException::domainTooLong and reuse OwidException::MAXIMUM_DOMAIN_LENGTH, so the two halves report the one condition the one way. The message of that named constructor is reworded to be true from either side and it still names the maximum. It does not name the domain, because writeString cannot tell whether the value came from the caller's own configuration or from bytes that some other route filled in. An empty domain, and any domain at or under the maximum, behave exactly as before. The two test files that deliberately build an over long domain for the read side now append the terminator themselves rather than going through writeString, because the write side would otherwise refuse the bytes those read tests are built from. The PHPUnit suite goes from 74 tests to 77 and the dependency free runner from 105 checks to 113. With the two checks removed and the new tests kept, all three new PHPUnit tests fail and seven of the eight new runner checks fail, the eighth being the control at exactly the maximum which is meant to pass either way. --- README.md | 7 +++ src/Creator.php | 16 +++++- src/Io.php | 19 ++++++- src/OwidException.php | 18 +++--- tests/DomainLengthTest.php | 114 ++++++++++++++++++++++++++++++++++++- tests/run.php | 92 +++++++++++++++++++++++++++++- 6 files changed, 250 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index c185652..182c02a 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,13 @@ this library stores. A domain with no terminator, or one longer than a domain name may be, is rejected for a cost set by that maximum rather than by the length of the buffer. +The same maximum binds the write, so this library cannot produce an OWID it +would then refuse to read. A `Creator` refuses a domain longer than the +maximum when the domain is supplied, which is the earliest point the caller +can be told, and the serialization refuses one as well, so a domain that +reaches the `Owid` domain field by any other route is caught before the +signature is calculated. + The in-memory APIs remain subject to PHP string, platform, address-space and available-memory limits. Applications accepting untrusted OWIDs must choose limits suitable for their use case and enforce them before buffering the diff --git a/src/Creator.php b/src/Creator.php index 5fa39da..f77d820 100644 --- a/src/Creator.php +++ b/src/Creator.php @@ -35,9 +35,15 @@ final class Creator /** * Creates a new creator for the domain using the crypto instance for - * signing. + * signing. The domain is bounded here, at the earliest point the caller + * can be told, so a creator configured with a domain longer than a + * domain name can hold is refused when the domain is supplied rather + * than when an OWID is later serialized. The check comes before the + * crypto instance is looked at, so nothing is signed with a domain this + * same library would then refuse to read. * - * @throws OwidException when the domain is empty or whitespace, or the + * @throws OwidException when the domain is empty or whitespace, is + * longer than a domain name can hold, or the * crypto instance can not sign. */ public function __construct(string $domain, Crypto $crypto) @@ -45,6 +51,9 @@ public function __construct(string $domain, Crypto $crypto) if (trim($domain) === '') { throw OwidException::invalidDomain($domain); } + if (strlen($domain) > OwidException::MAXIMUM_DOMAIN_LENGTH) { + throw OwidException::domainTooLong(); + } if (!$crypto->canSign()) { throw OwidException::keyMissing('generate a signature'); } @@ -55,7 +64,8 @@ public function __construct(string $domain, Crypto $crypto) /** * Creates a new creator from the domain and the private key PEM provided. * - * @throws OwidException when the domain is empty or whitespace, or the + * @throws OwidException when the domain is empty or whitespace, is + * longer than a domain name can hold, or the * private key PEM is not valid. */ public static function fromConfiguration(string $domain, string $privatePem): self diff --git a/src/Io.php b/src/Io.php index 7ce480e..531b9cf 100644 --- a/src/Io.php +++ b/src/Io.php @@ -225,16 +225,29 @@ public static function writeByte(string &$buffer, int $value): void } /** - * Writes the string followed by the null terminator. The string must not - * contain a null character as that would conflict with the terminator. + * Writes the string followed by the null terminator. The only such string + * in an OWID is the creator domain. The value must not contain a null + * character as that would conflict with the terminator, and must not be + * longer than the greatest number of characters a domain name can hold, + * because readString stops looking for the terminator at that bound and + * would refuse anything longer. Without this the library could write an + * OWID it then refused to read, and the fault would land on whoever read + * it rather than on the creator that caused it. This is the later of the + * two write side checks, and it catches a domain that reached the OWID + * by some route other than the creator, such as the public domain field + * being assigned directly. * - * @throws OwidException when the value contains a null byte. + * @throws OwidException when the value contains a null byte, or is + * longer than a domain name can hold. */ public static function writeString(string &$buffer, string $value): void { if (strpos($value, "\0") !== false) { throw OwidException::invalidDomain($value); } + if (strlen($value) > OwidException::MAXIMUM_DOMAIN_LENGTH) { + throw OwidException::domainTooLong(); + } $buffer .= $value . "\0"; } diff --git a/src/OwidException.php b/src/OwidException.php index 511b076..49a0d65 100644 --- a/src/OwidException.php +++ b/src/OwidException.php @@ -92,18 +92,22 @@ public static function invalidDomain(string $domain): self } /** - * The domain field has no terminator within the greatest number of - * characters a domain name can hold, so either the terminator is missing - * or the domain is longer than a domain name may be. The bytes are not - * named because they are whatever the sender wrote and there may be no - * end to them. + * The domain is longer than the greatest number of characters a domain + * name can hold. Both halves of the library raise this, so both report + * the one condition the one way. On a read the domain field has no + * terminator within that many characters, so whatever the field holds + * runs past the bound, and on a write the value handed in is longer + * than the bound. The domain is not named because on a read the bytes + * are whatever the sender wrote and there may be no end to them, and + * because writeString cannot tell which of the two routes a value + * arrived by. */ public static function domainTooLong(): self { $maximum = self::MAXIMUM_DOMAIN_LENGTH; return new self( - "OWID domain has no terminator within the '$maximum' characters " . - "a domain name can hold" + "OWID domain is longer than the '$maximum' characters a domain " . + "name can hold" ); } diff --git a/tests/DomainLengthTest.php b/tests/DomainLengthTest.php index e64c360..e7fbeb3 100644 --- a/tests/DomainLengthTest.php +++ b/tests/DomainLengthTest.php @@ -67,13 +67,17 @@ private static function domain(int $length): string /** * A version 3 envelope carrying the domain given, followed by a date, an - * empty payload and the signature. + * empty payload and the signature. The domain and its terminator are + * appended here rather than through Io::writeString because these tests + * build domains the write side now refuses, and the point of them is + * what the read side does with such bytes when they arrive from + * somewhere else. */ private static function envelope(string $domain): string { $buffer = ''; Io::writeByte($buffer, Version::Version3->asByte()); - Io::writeString($buffer, $domain); + $buffer .= $domain . chr(0); Io::writeUint32($buffer, 1000); Io::writeUint32($buffer, 0); return $buffer . str_repeat("\x99", self::SIGNATURE_LENGTH); @@ -210,6 +214,112 @@ public function testDomainFillingTheBoundWithNoTerminatorIsRefused(): void $this->refusal($bytes, 'domain filling the bound'); } + /** + * A creator is refused a domain one character longer than a domain name + * can hold, at the point the domain is supplied, so the caller is told + * when the configuration is wrong rather than when an OWID is later + * serialized. Both ways of making a creator are covered, and the message + * names the maximum. + */ + public function testCreatorRefusesDomainOverMaximum(): void + { + $domain = self::domain(OwidException::MAXIMUM_DOMAIN_LENGTH + 1); + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $crypto = Crypto::new(); + + try { + new Creator($domain, $crypto); + $this->fail('over long creator domain should have been refused'); + } catch (OwidException $e) { + $this->assertStringContainsString("'$maximum'", $e->getMessage()); + } + + try { + Creator::fromConfiguration($domain, $crypto->privateKeyPem()); + $this->fail('over long configured domain should have been refused'); + } catch (OwidException $e) { + $this->assertStringContainsString("'$maximum'", $e->getMessage()); + } + } + + /** + * The serialization refuses the same domain as well, so a value that + * reached the public domain field by a route other than the creator is + * still refused. The data the signature is calculated over is built the + * same way, so the refusal reaches that too. A domain of exactly the + * greatest length is written and parses back unchanged, so this is a + * refusal at the top of the range and nothing else. + */ + public function testWriteRefusesDomainOverMaximum(): void + { + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $owid = new Owid(); + $owid->payload = 'value'; + $owid->signature = str_repeat(chr(0x99), self::SIGNATURE_LENGTH); + + $owid->domain = self::domain($maximum + 1); + foreach (['asByteArray', 'dataForCrypto'] as $method) { + try { + $owid->$method(); + $this->fail("$method should have refused the long domain"); + } catch (OwidException $e) { + $this->assertStringContainsString( + "'$maximum'", + $e->getMessage() + ); + } + } + + $owid->domain = self::domain($maximum); + $parsed = Owid::fromByteArray($owid->asByteArray()); + $this->assertSame($owid->domain, $parsed->domain); + } + + /** + * The refusal happens before any signature is calculated. A creator is + * refused before its crypto instance is looked at, which is shown by + * handing the constructor an instance that can only verify, because a + * message naming the key rather than the maximum would mean the two + * checks ran the other way round. A domain that arrives on another OWID + * covered by the signature is refused while the data to sign is being + * built, which is before the signing key is used, so the signature field + * is left holding exactly what it held before. + */ + public function testRefusalHappensBeforeAnySignature(): void + { + $domain = self::domain(OwidException::MAXIMUM_DOMAIN_LENGTH + 1); + $maximum = OwidException::MAXIMUM_DOMAIN_LENGTH; + $crypto = Crypto::new(); + $verifyOnly = Crypto::newVerifyOnly($crypto->publicKeyPem()); + + try { + new Creator($domain, $verifyOnly); + $this->fail('over long creator domain should have been refused'); + } catch (OwidException $e) { + $this->assertStringContainsString("'$maximum'", $e->getMessage()); + } + + $other = new Owid(); + $other->domain = $domain; + $other->payload = 'other'; + $other->signature = str_repeat(chr(0x11), self::SIGNATURE_LENGTH); + $owid = new Owid(); + $owid->payload = 'value'; + $owid->signature = str_repeat(chr(0x22), self::SIGNATURE_LENGTH); + + try { + (new Creator('51d.es', $crypto))->signWithOthers($owid, [$other]); + $this->fail('over long domain on another OWID should be refused'); + } catch (OwidException $e) { + $this->assertStringContainsString("'$maximum'", $e->getMessage()); + } + $this->assertSame( + str_repeat(chr(0x22), self::SIGNATURE_LENGTH), + $owid->signature, + 'the signature should not have been calculated' + ); + } + /** * What the library itself signs still parses and still verifies, with an * ordinary domain and with one of the greatest length, so the bound is diff --git a/tests/run.php b/tests/run.php index 8a52207..e622f71 100644 --- a/tests/run.php +++ b/tests/run.php @@ -405,11 +405,15 @@ function domainOfLength(int $length): string $labels[] = str_repeat('a', $remaining); return implode('.', $labels); } +// The domain and its terminator are appended here rather than through +// Io::writeString because these checks build domains the write side now +// refuses, and the point of them is what the read side does with such bytes +// when they arrive from somewhere else. function domainEnvelope(string $domain): string { $buffer = ''; Io::writeByte($buffer, Version::Version3->asByte()); - Io::writeString($buffer, $domain); + $buffer .= $domain . chr(0); Io::writeUint32($buffer, 1000); Io::writeUint32($buffer, 0); return $buffer . str_repeat("\x99", 64); @@ -480,4 +484,90 @@ function timeDomainRefusals(string $bytes, int $attempts, bool &$refused): float $maximumParsed->verifyWithCrypto($maximumCrypto) ); +// The write is bounded as well, at the creator where the domain is supplied +// and again in the serialization, so this library cannot produce an OWID it +// would then refuse to read. +function domainRefusalNamesMaximum(callable $action): bool +{ + try { + $action(); + } catch (OwidException $e) { + return str_contains( + $e->getMessage(), + "'" . OwidException::MAXIMUM_DOMAIN_LENGTH . "'" + ); + } + return false; +} +$overLongDomain = domainOfLength(OwidException::MAXIMUM_DOMAIN_LENGTH + 1); +$runner->check( + 'creator refuses a domain over the greatest length, naming the maximum', + domainRefusalNamesMaximum( + fn () => new Creator($overLongDomain, $maximumCrypto) + ) +); +$runner->check( + 'creator from configuration refuses a domain over the greatest length', + domainRefusalNamesMaximum( + fn () => Creator::fromConfiguration( + $overLongDomain, + $maximumCrypto->privateKeyPem() + ) + ) +); +$overLongOwid = new Owid(); +$overLongOwid->domain = $overLongDomain; +$overLongOwid->payload = 'value'; +$overLongOwid->signature = str_repeat(chr(0x99), 64); +$runner->check( + 'serializing a domain over the greatest length is refused', + domainRefusalNamesMaximum(fn () => $overLongOwid->asByteArray()) +); +$runner->check( + 'building signing data for a domain over the greatest length is refused', + domainRefusalNamesMaximum(fn () => $overLongOwid->dataForCrypto()) +); +$atBoundOwid = new Owid(); +$atBoundOwid->domain = $maximumDomain; +$atBoundOwid->payload = 'value'; +$atBoundOwid->signature = str_repeat(chr(0x99), 64); +$runner->check( + 'serializing a domain of the greatest length parses back unchanged', + Owid::fromByteArray($atBoundOwid->asByteArray())->domain === $maximumDomain +); +// The creator refuses the domain before it looks at the crypto instance, so +// an instance that can only verify still gives the domain message and not +// the key one, and nothing is ever signed with a domain that could not be +// read back. +$runner->check( + 'creator refuses the domain before looking at the crypto instance', + domainRefusalNamesMaximum( + fn () => new Creator( + $overLongDomain, + Crypto::newVerifyOnly($maximumCrypto->publicKeyPem()) + ) + ) +); +// A domain arriving on another OWID covered by the signature is refused +// while the data to sign is being built, which is before the signing key is +// used, so the signature field is left holding what it held before. +$otherOwid = new Owid(); +$otherOwid->domain = $overLongDomain; +$otherOwid->payload = 'other'; +$otherOwid->signature = str_repeat(chr(0x11), 64); +$targetOwid = new Owid(); +$targetOwid->payload = 'value'; +$targetOwid->signature = str_repeat(chr(0x22), 64); +$runner->check( + 'over long domain on another OWID is refused when signing', + domainRefusalNamesMaximum( + fn () => (new Creator('51d.es', $maximumCrypto)) + ->signWithOthers($targetOwid, [$otherOwid]) + ) +); +$runner->check( + 'signature is not calculated when the domain is refused', + $targetOwid->signature === str_repeat(chr(0x22), 64) +); + exit($runner->summary());