fix: reject a signed byte pair in Data(hexString:) - #1687
Open
Chessing234 wants to merge 2 commits into
Open
Conversation
UInt8(_:radix:) accepts a leading sign, so the two-character chunks this parser feeds it let "+f" through as 0x0f. The doc comment already promises nil for a string containing invalid hex characters, and it did not deliver that. The cost is a second spelling for every identity-bearing hex string the app parses -- Noise keys, Nostr pubkeys, fingerprints, signatures, routing IDs. A 64-character peer ID of "+b" pairs passed PeerID.isValid, reported isHex false and isNoiseKeyHex true at the same time, and resolved to the same 32-byte key and the same short routing ID as the genuine peer it shadowed. Require two hex digits before parsing the pair.
DataHexTests covers what the parser accepts (plain, uppercase, 0x-prefixed, whitespace-padded, every byte value round-tripped) alongside what it must refuse, including the signed pairs that used to alias a genuine key. PeerIDTests gains the 64-character negatives it was missing -- the short form already had them -- and asserts isHex and isNoiseKeyHex agree.
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
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.
Data(hexString:)chunks the string into two-character pairs and hands each toUInt8(_:radix:). That initializer accepts a leading sign, so"+f"parses as0x0f. Its own doc comment promises nil for a string that "contains invalid hex characters", and+is one.Every identity-bearing hex string in the app goes through this parser — Noise keys, Nostr pubkeys, group member fingerprints, signatures, routing IDs — so each one had a second spelling that decodes to the same bytes.
PeerIDis where that shows up most sharply. A 64-character id built from"+b"pairs:isShortvalidates the 16-hex form withisHexDigitand rejects+correctly;isNoiseKeyHexvalidates the 64-hex form by asking whetherData(hexString:)returns non-nil, so the two halves of the same check disagreed.PeerIDequality and hashing are on the string, so the spoofed id is a distinct key in any dictionary while resolving to the same cryptographic identity.Fix is to require two hex digits before parsing the pair. Everything the parser is documented to accept still parses: plain, uppercase,
0x/0X-prefixed, whitespace-padded, and every byte value round-trips.I did not widen this beyond the parser.
PeerID.isValidneeds no change onceData(hexString:)keeps its contract, and I would rather not touch the validity rules in the same diff.Evidence
No Xcode on this machine, so I could not run
bitchatTestsor build the app. What I did instead: builtBitFoundationwithswift buildand linked its object files into a standalone harness, so the assertions below ran against the real compiled module rather than a stub.Before the fix, the harness reproduced the table above —
+b×32 and0b×32 are distinctPeerIDvalues that both passisValidand produce the identical 32-byte key and identicaltoShort(). After it, the spoofed id returns nil fromnoiseKey,isValidis false,isHexandisNoiseKeyHexagree, and the genuine id is untouched.I then ran all 22 assertions from the two test files through that harness; they pass. The tests themselves are swift-testing, and the
Testingmodule needs the Xcode toolchain, so I have not executedDataHexTestsorPeerIDTestsas tests — CI is the first thing that will. The assertions inside them are the ones I ran; the@Test/#expectscaffolding is copied from the sibling files.swift build --package-path localPackages/BitFoundationis clean.Related but not a duplicate: #911 hardened the same initializer for odd length,
0xprefix, whitespace and empty input. Those all landed and are still there; the sign case was not part of it.This was AI-assisted.