Skip to content

Commit 9336bdb

Browse files
fix(sdk): reject unpaired surrogates before signing, and correct the length-prefix docs
Review findings 2 and 5 on #4259. The Kotlin and Swift docs described the digest's length prefix in each language's own string units — `message.length` (UTF-16 code units) and `message.count` (grapheme clusters). The format prefixes the UTF-8 BYTE count; the three agree only for ASCII. Corrected to `message.toByteArray(Charsets.UTF_8).size` and `message.utf8.count`. That doc bug has a real counterpart: a Kotlin `String` is an unvalidated UTF-16 sequence and may hold an unpaired surrogate, which has no UTF-8 encoding. Every conversion below is lenient and they do not even agree — the JNI bridge's string read substitutes U+FFFD, while `toByteArray(Charsets.UTF_8)` substitutes '?' (verified on JDK 17). So the wallet would sign bytes the caller never wrote and return a signature that verifies for a different message, silently, and "verbatim" in the docs would be false. Rejected at the Kotlin entry point, the last layer that still holds the exact UTF-16 and can explain why. Marshalling a UTF-8 ByteArray across the JNI instead — the other option raised in review — would NOT fix this: Kotlin's own encoder is equally lossy, so it would relocate the silent substitution and change it from U+FFFD to '?' while widening the FFI surface. Well-formed strings are unaffected. The lossy `get_string` read is the shared `read_cstring_required` behaviour on every JNI string parameter in the crate, so it is a codebase-wide convention rather than something this path introduced; flagged rather than changed here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent bf3a588 commit 9336bdb

2 files changed

Lines changed: 52 additions & 6 deletions

File tree

packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,10 @@ class ManagedPlatformWallet internal constructor(
180180
* CrowdNode's server-side check.
181181
*
182182
* **The format.** The signed digest is
183-
* `SHA256d(prefix ‖ varint(message.length) ‖ message)`, where the prefix is
183+
* `SHA256d(prefix ‖ varint(bytes.size) ‖ bytes)` over
184+
* `bytes = message.toByteArray(Charsets.UTF_8)`: the length prefix counts
185+
* **UTF-8 bytes**, not `String.length`, which counts UTF-16 code units and
186+
* diverges for any non-ASCII text. The prefix is
184187
* the historical `"\x19DarkCoin Signed Message:\n"` — *not* `"Dash"`. Dash
185188
* inherited that string from before the rename and every existing verifier
186189
* depends on it, so it can never change. The returned signature is the
@@ -223,9 +226,13 @@ class ManagedPlatformWallet internal constructor(
223226
*
224227
* @param address the P2PKH address whose key signs; must be one this wallet
225228
* owns and has derived.
226-
* @param message the string to sign, **verbatim**. It is length-prefixed
227-
* into the digest, so trailing whitespace and newlines are significant and
228-
* the verifier must receive the identical bytes. An empty string is valid.
229+
* @param message the string to sign, **verbatim** as UTF-8. It is
230+
* length-prefixed into the digest, so trailing whitespace and newlines are
231+
* significant and the verifier must receive the identical bytes. An empty
232+
* string is valid. Must be well-formed text: a string holding an unpaired
233+
* UTF-16 surrogate has no UTF-8 encoding and is rejected with
234+
* [IllegalArgumentException] rather than signed after a silent
235+
* substitution.
229236
* @param coreSignerHandle the manager's `MnemonicResolverHandle`
230237
* (`PlatformWalletManager.mnemonicResolverHandle`); no private key crosses
231238
* the boundary.
@@ -237,6 +244,18 @@ class ManagedPlatformWallet internal constructor(
237244
coreSignerHandle: Long,
238245
): String = gate.op {
239246
require(address.isNotEmpty()) { "address must not be empty" }
247+
// The digest commits to the message's UTF-8 bytes, but a Kotlin String
248+
// is an unvalidated UTF-16 sequence and may hold an unpaired surrogate,
249+
// which has no UTF-8 encoding at all. Every conversion below is LENIENT
250+
// and they do not even agree: the JNI bridge's String read substitutes
251+
// U+FFFD, while `toByteArray(Charsets.UTF_8)` substitutes '?'. Either
252+
// way the wallet would sign bytes the caller never wrote and hand back a
253+
// signature that verifies for a different message — silently. Rejected
254+
// here, the one layer that still has the exact UTF-16 and can say why.
255+
require(message.hasNoUnpairedSurrogate()) {
256+
"message must be well-formed text: it contains an unpaired UTF-16 surrogate, " +
257+
"which has no UTF-8 encoding and would be silently substituted before signing"
258+
}
240259

241260
mapNativeErrors {
242261
coreWallet().use { core ->
@@ -245,6 +264,30 @@ class ManagedPlatformWallet internal constructor(
245264
}
246265
}
247266

267+
/**
268+
* Whether every UTF-16 surrogate in this string is part of a well-formed
269+
* high/low pair — i.e. whether the string has an exact UTF-8 encoding.
270+
*
271+
* Scanned directly rather than via a strict `CharsetEncoder` to keep the
272+
* check allocation-free on the hot path; the two agree on exactly which
273+
* strings are encodable.
274+
*/
275+
private fun String.hasNoUnpairedSurrogate(): Boolean {
276+
var i = 0
277+
while (i < length) {
278+
val c = this[i]
279+
when {
280+
c.isHighSurrogate() -> {
281+
if (i + 1 >= length || !this[i + 1].isLowSurrogate()) return false
282+
i += 2
283+
}
284+
c.isLowSurrogate() -> return false
285+
else -> i++
286+
}
287+
}
288+
return true
289+
}
290+
248291
/**
249292
* The wallet's Platform-payment addresses that currently hold credits,
250293
* each as a [FundingInput] whose `credits` is the full cached balance —

packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,11 @@ public class ManagedCoreWallet {
142142
/// RPC, and verifiable by `verifymessage`, `ECKey.verifyMessage`, and
143143
/// CrowdNode's server-side check.
144144
///
145-
/// The signed digest is `SHA256d(prefix ‖ varint(message.count) ‖ message)`,
146-
/// where the prefix is the historical `"\u{19}DarkCoin Signed Message:\n"` —
145+
/// The signed digest is
146+
/// `SHA256d(prefix ‖ varint(message.utf8.count) ‖ message.utf8)`: the length
147+
/// prefix counts **UTF-8 bytes**, not `String.count`, which counts grapheme
148+
/// clusters and diverges for any non-ASCII text.
149+
/// The prefix is the historical `"\u{19}DarkCoin Signed Message:\n"` —
147150
/// *not* `"Dash"`. Dash inherited that string from before the rename and
148151
/// every existing verifier depends on it, so it can never change. The
149152
/// returned signature is the 65-byte BIP-137-style recoverable form

0 commit comments

Comments
 (0)