fix: reject a signed byte pair in Data(hexString:) (#1687)

* fix: reject a signed byte pair in Data(hexString:)

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.

* test: cover hex parsing and the PeerID validity it backs

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 commit is contained in:
Taksh Kothari 2026-09-24 13:04:25 +05:30 committed by GitHub
parent 5a10ba6bf9
commit c82e6d5ab0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 79 additions and 1 deletions

View File

@ -45,7 +45,12 @@ public extension Data {
for _ in 0..<len {
let nextIndex = hex.index(index, offsetBy: 2)
guard let byte = UInt8(String(hex[index..<nextIndex]), radix: 16) else {
let pair = hex[index..<nextIndex]
// UInt8(_:radix:) accepts a leading sign, so "+f" parses as 0x0f.
// That gives an identity-bearing hex string a second spelling which
// decodes to the same bytes, so require two hex digits first.
guard pair.allSatisfy(\.isHexDigit),
let byte = UInt8(pair, radix: 16) else {
return nil
}
data.append(byte)

View File

@ -0,0 +1,62 @@
//
// DataHexTests.swift
// bitchatTests
//
// This is free and unencumbered software released into the public domain.
// For more information, see <https://unlicense.org>
//
import Testing
import Foundation
import BitFoundation
struct DataHexTests {
// MARK: - Accepted forms
@Test func decodes_plain_hex() {
#expect(Data(hexString: "0a1b2c")?.hexEncodedString() == "0a1b2c")
}
@Test func decodes_uppercase_hex() {
#expect(Data(hexString: "0A1B2C")?.hexEncodedString() == "0a1b2c")
}
@Test func decodes_prefixed_and_padded_hex() {
#expect(Data(hexString: "0x0a1b")?.hexEncodedString() == "0a1b")
#expect(Data(hexString: "0X0a1b")?.hexEncodedString() == "0a1b")
#expect(Data(hexString: " 0a1b ")?.hexEncodedString() == "0a1b")
}
@Test func round_trips_encoded_bytes() {
let bytes = Data((0...255).map { UInt8($0) })
#expect(Data(hexString: bytes.hexEncodedString()) == bytes)
}
// MARK: - Rejected forms
@Test func rejects_odd_length() {
#expect(Data(hexString: "0a1") == nil)
}
@Test func rejects_non_hex_characters() {
#expect(Data(hexString: "zz") == nil)
#expect(Data(hexString: "0a zz") == nil)
}
/// UInt8(_:radix:) accepts a leading sign, so "+f" parses as 0x0f unless the
/// digits are checked first. Two spellings of one identity-bearing hex
/// string decoding to the same bytes is the whole problem.
@Test func rejects_signed_byte_pairs() {
#expect(Data(hexString: "+a") == nil)
#expect(Data(hexString: "-a") == nil)
#expect(Data(hexString: "0a+b") == nil)
#expect(Data(hexString: "+a+b") == nil)
}
@Test func a_signed_pair_does_not_alias_a_genuine_key() {
let genuine = String(repeating: "0b", count: 32)
let signed = String(repeating: "+b", count: 32)
#expect(Data(hexString: genuine) != nil)
#expect(Data(hexString: signed) == nil)
}
}

View File

@ -362,6 +362,10 @@ struct PeerIDTests {
@Test func rejects_invalid_characters() {
#expect(!PeerID(str: "peer!@#").isValid)
#expect(!PeerID(str: "gggggggggggggggg").isValid) // not hex for short form
// A 64-character id is only valid because it decodes to a Noise key, so
// it has to be rejected on the same grounds a short one is.
#expect(!PeerID(str: String(repeating: "+b", count: 32)).isValid)
#expect(!PeerID(str: String(repeating: "gg", count: 32)).isValid)
}
@Test func rejects_too_long() {
@ -388,6 +392,13 @@ struct PeerIDTests {
let badPeerID = PeerID(str: bad)
#expect(!badPeerID.isNoiseKeyHex)
#expect(badPeerID.noiseKey == nil)
// isHex and isNoiseKeyHex have to agree: a signed pair is not hex, so it
// must not decode to a Noise key either.
let signedPeerID = PeerID(str: String(repeating: "+b", count: 32))
#expect(!signedPeerID.isHex)
#expect(!signedPeerID.isNoiseKeyHex)
#expect(signedPeerID.noiseKey == nil)
}
@Test func prefixes() {