Bound the declared payload length and the domain on read and write - #1
Merged
Conversation
…locating Reader.read_byte_array in owid/io.py passed the sender's declared payload count to read_bytes, which bounds every read by the buffer, so this port never allocated by the declared number. The count was not checked against what a valid OWID must contain though. A valid OWID is the declared payload followed by the 64 byte signature and nothing else, and the parser read 64 signature bytes from wherever the declared count left it and ignored whatever came after. A count one short of the payload therefore parsed with a misaligned signature, and an envelope with bytes after the signature parsed as if they were not there, so this port accepted malformed envelopes that the other ports now refuse. The count is now checked before anything is sized by it. It must equal the bytes remaining less the signature length, and any other count, short or long, is refused with the existing OwidError naming the declared length and the bytes present. That one check also refuses a signature shorter than 64 bytes and any byte after the signature. The other length driven reads were checked and need no change, as read_string stops at the end of the buffer when no terminator is found and read_bytes refuses any count beyond the bytes present. This port has no stream reader, so there is no non-seekable path to bound. tests/test_payload_length.py 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 in under a second across 1,000 parses and with a tracemalloc peak under 64 KiB, and an empty payload. tests/test_io.py now writes the signature after the byte array it round trips, because the reader requires it. The suite goes from 57 to 64 tests.
The creator domain is stored as text followed by a zero terminator, and the parse found the end of it by walking forward to that terminator with nothing stopping the walk short of the end of the buffer. A buffer whose terminator was missing or corrupted was therefore walked to its end, and a terminator placed far away made the reader decode everything before it into a string, so the work was sized by the sender rather than by the format. The reader now stops looking for the terminator after MAXIMUM_DOMAIN_LENGTH bytes and refuses the buffer there, so the cost of a hostile domain field is fixed by that constant. RFC 1035 section 2.3.4, "Size limits", restricts the total length of a domain name, being the label octets and the label length octets, to 255 octets or less. An OWID stores the presentation form, the text "example.com", where the dots stand in for the label length octets and the root label has no text at all, so the limit on the text is two fewer. Nothing changes for a valid envelope. A domain of the maximum length still parses, and the library's own output still parses and verifies. The new tests cover the maximum length, one character over, a buffer with no zero byte anywhere and a buffer with the terminator 64 MiB away, using tracemalloc and a wall clock to assert that refusing each hostile buffer stays inside the bound. The README no longer says the domain has no encoded maximum.
jwrosewell
changed the title
The parse was bounded at 253 characters but the write was not, so a creator configured with a longer domain still produced an OWID that this same library refused to read. A library that can emit something it cannot read is worse than one that does neither, because the fault then surfaces at the consumer rather than at the creator. The limit now binds both halves, reusing MAXIMUM_DOMAIN_LENGTH rather than repeating the number. A Creator refuses a longer domain when the caller supplies it, which is the earliest point a caller can be told and is before the key is checked and before anything is signed. The writer refuses one that reached an OWID by another route, such as a domain set on the structure directly, when the OWID is serialised. Both raise OwidError naming the maximum, as the parse does. An empty domain and any domain at or under the maximum behave exactly as before. The tests cover writing and reading back a domain of the maximum length, the refusal at each of the two points, and a counting crypto stand-in that proves no signature is computed before the refusal arrives.
jwrosewell
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect and format rule
The parser previously accepted a payload length that left bytes after the 64-byte signature. This PR requires the declared payload to be followed by exactly the signature and nothing else. A declaration that does not agree with the bytes present is rejected before the payload is copied.
The unsigned 32-bit field itself permits payloads from 0 through 4,294,967,295 bytes. A large value is not malformed when the matching bytes are present. Runtime capacity and application policy are separate from format validity.
The creator domain had the same shape of defect and is now bounded as well. It is stored as text followed by a zero terminator, and the parse found the end of it by walking forward to that terminator with nothing stopping the walk short of the end of the buffer, so a missing or corrupted terminator turned the domain field into work the sender chose the size of. RFC 1035 section 2.3.4, "Size limits", restricts the total length of a domain name, being the label octets and the label length octets, to 255 octets or less. An OWID stores the presentation form of that name, the text
example.com, where the dots stand in for the label length octets and the root label has no text at all, so the limit on the text is two fewer. The reader now stops looking for the terminator at that maximum and refuses the buffer there.The read was then bounded and the write was not, so a creator configured with a longer domain still produced an OWID that this same library refused to parse. A library that can emit something it cannot read is worse than one that does neither, because the fault surfaces at the consumer rather than at the creator. The same maximum now binds the write, and the specification is being changed to match in owid#6 so the limit binds a creator as well as a consumer.
Current changes
len(owid.payload)is the allocation-free structured size for downstream policy.struct.unpack_from, avoiding a temporary four-byte slice.MAXIMUM_DOMAIN_LENGTHinowid/io.py, whose comment carries the RFC 1035 derivation, so a domain field whose terminator is missing, corrupted or far away is refused after a fixed number of bytes instead of after the whole buffer has been walked or decoded into a string.tests/test_domain_length.pycovering a domain of the maximum length, one character over the maximum, a buffer holding no zero byte anywhere and a buffer with the terminator 64 MiB away.MAXIMUM_DOMAIN_LENGTHat both of the points a domain can enter the library, reusing the same constant rather than repeating the number.Creatorrefuses it where the caller supplies it, which is the earliest point a caller can be told and is before the crypto instance is checked and before anything is signed, andwrite_stringrefuses one that reached an OWID by another route, such as a domain set on the structure directly, when the OWID is serialised. Both raiseOwidErrornaming the maximum, as the parse does.tests/test_domain_length.pycovering a domain of the maximum length written and parsed back, an empty domain still written as the terminator on its own, the refusal at each of the two points, and a counting crypto stand-in proving no signature is computed before the refusal arrives.A malformed envelope declaring 4 GiB but carrying only a few bytes still fails with work and memory proportional to the bytes actually supplied. A matching payload is processed in proportion to its real size and remains subject to Python's object, address-space, and available-memory capacity. A hostile domain field now costs the bound rather than the length of the input, and nothing about a valid envelope changes, so a domain of the maximum length still parses and the library's own signed output still parses and verifies.
Validation
python -m unittest discover, which is what CI runs, on Python 3.14.5 on Windows. 66 passed before any of this work, 71 after the payload length check and the domain read bound, and 76 after the domain write bound, with no existing test modified.Reverting only the read bound and leaving its new tests in place fails three of the five read tests. The 254-character domain parsed with no error raised. The buffer with no terminator took 8.849 seconds for 1,000 parses against a limit of 1 second. The buffer with the terminator 64 MiB away reached a tracemalloc peak of 134,218,458 bytes against a limit of 65,536. The two tests that pass either way are the ones asserting that valid input is unaffected, which is what they are there for.
Reverting both halves of the write bound and leaving its new tests in place fails three of the five write tests. Creating a creator with a 254-character domain raised nothing, serialising an OWID whose domain had been set on the structure directly raised nothing, and the counting crypto test failed at the creator that should have been refused. Reverting each half on its own separates the two, because removing the check in
write_stringfails only the serialisation test and removing the check inCreatorfails only the creator test and the one about signing order. The two write tests that pass either way are the ones asserting that a domain of the maximum length and an empty domain are unaffected.References: the format-validation fix is owid-dotnet#5; the .NET size-policy follow-up is owid-dotnet#6. The domain bound follows RFC 1035 section 2.3.4, Size limits.
This change was produced with AI assistance under James Rosewell's direction and needs human review before merge.