From e94b8eaf3b62bc25ada2cdda6c7b1da0c80d5128 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 20:00:07 +0000 Subject: [PATCH 1/2] fix(nip44): content-address shared-key cache and use constant-time MAC checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes in the NIP-44/NIP-04 encryption primitives: 1. SharedKeyCache keyed conversation/shared secrets by a 32-bit polynomial hashCode of (privateKey || pubKey). Distinct peers whose bytes hash to the same Int collided on one LRU slot, so get() could return one peer's key for a message meant for another — a silent wrong-key encrypt/decrypt. The hash collides independently of the private key, so a pubkey aliasing a victim's contact is grindable. Now keyed on the full (priv || pub) byte content. Added SharedKeyCacheTest, which fails on the old hashCode key and passes now. 2. NIP-44 MAC verification (Hkdf.fastExpand and Nip44v2.checkHMacAad) used ByteArray.contentEquals, which short-circuits on the first mismatching byte and leaks a timing side channel on the HMAC tag. Switched to a shared equalsConstantTime helper (utils/ConstantTime.kt), matching the constant-time tag checks already used in ChaCha20Poly1305/XChaCha20Poly1305/MLS. Also corrects the X25519 KDoc, which claimed the JVM/Android actual delegates to java.security XDH; it is in fact the same pure-Kotlin Montgomery ladder as the other targets. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PY7kpiTcFiDshYqBsQ5KVC --- .../quartz/marmot/mls/crypto/X25519.kt | 11 ++- .../quartz/nip44Encryption/Nip44v2.kt | 3 +- .../quartz/nip44Encryption/SharedKeyCache.kt | 27 +++--- .../quartz/nip44Encryption/crypto/Hkdf.kt | 3 +- .../quartz/utils/ConstantTime.kt | 43 ++++++++++ .../nip44Encryption/SharedKeyCacheTest.kt | 86 +++++++++++++++++++ 6 files changed, 156 insertions(+), 17 deletions(-) create mode 100644 quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/ConstantTime.kt create mode 100644 quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCacheTest.kt diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.kt index 702fb3d8b4..ea6f18ae98 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/crypto/X25519.kt @@ -25,9 +25,14 @@ package com.vitorpamplona.quartz.marmot.mls.crypto * * Used in TreeKEM for path secret encryption via HPKE. * - * Platform-specific implementations: - * - JVM/Android: java.security XDH (Java 11+, Android API 31+) - * - Native: expect/actual with platform crypto + * All platform actuals (jvmAndroid, apple, linux) are the SAME pure-Kotlin + * Montgomery-ladder implementation over [Curve25519Field] (a TweetNaCl port) — + * they intentionally do NOT delegate to a platform provider such as + * `java.security` XDH, because Android's KeyStore/JCA integration for raw X25519 + * keys is unreliable across API levels. `dh()` rejects an all-zero shared secret + * per RFC 7748 §6.1; note the ladder relies on branch-uniform (mask-based) + * selection but, being pure Kotlin on the JVM/ART, carries no hardware + * constant-time guarantee. */ expect object X25519 { /** diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/Nip44v2.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/Nip44v2.kt index 7dffa7b13a..98baa80452 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/Nip44v2.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/Nip44v2.kt @@ -25,6 +25,7 @@ import com.vitorpamplona.quartz.nip44Encryption.crypto.ChaCha20 import com.vitorpamplona.quartz.nip44Encryption.crypto.Hkdf import com.vitorpamplona.quartz.utils.RandomInstance import com.vitorpamplona.quartz.utils.Secp256k1Instance +import com.vitorpamplona.quartz.utils.equalsConstantTime import kotlinx.coroutines.CancellationException import kotlin.io.encoding.Base64 import kotlin.math.floor @@ -104,7 +105,7 @@ class Nip44v2 { ) { val calculatedMac = hmacAad(messageKey.hmacKey, decoded.ciphertext, decoded.nonce) - check(calculatedMac.contentEquals(decoded.mac)) { + check(calculatedMac.equalsConstantTime(decoded.mac)) { "Invalid Mac: Calculated ${calculatedMac.toHexKey()}, decoded: ${decoded.mac.toHexKey()}" } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt index 3d510ca11e..d613695448 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt @@ -21,34 +21,37 @@ package com.vitorpamplona.quartz.nip44Encryption import androidx.collection.LruCache +import com.vitorpamplona.quartz.nip01Core.core.toHexKey class SharedKeyCache { - private val sharedKeyCache = LruCache(200) + // Keyed by the full (privateKey || pubKey) content, NOT by a 32-bit hashCode. + // A hashCode key silently collides: two distinct peers whose (priv, pub) bytes + // hash to the same Int would share a cache slot, so `get` could return one + // peer's conversation key for a message meant for the other — a silent + // wrong-key encrypt/decrypt. The polynomial hash that was used here collides + // independently of the private key, so an attacker could grind a pubkey that + // aliases a victim's contact. Keying on the raw bytes removes the collision. + private val sharedKeyCache = LruCache(200) fun clearCache() { sharedKeyCache.evictAll() } - fun combinedHashCode( - a: ByteArray, - b: ByteArray, - ): Int { - var result = 1 - for (element in a) result = 31 * result + element - for (element in b) result = 31 * result + element - return result - } + private fun cacheKey( + privateKey: ByteArray, + pubKey: ByteArray, + ): String = privateKey.toHexKey() + pubKey.toHexKey() fun get( privateKey: ByteArray, pubKey: ByteArray, - ): ByteArray? = sharedKeyCache[combinedHashCode(privateKey, pubKey)] + ): ByteArray? = sharedKeyCache[cacheKey(privateKey, pubKey)] fun add( privateKey: ByteArray, pubKey: ByteArray, secret: ByteArray, ) { - sharedKeyCache.put(combinedHashCode(privateKey, pubKey), secret) + sharedKeyCache.put(cacheKey(privateKey, pubKey), secret) } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/crypto/Hkdf.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/crypto/Hkdf.kt index 9ede033861..e117245b2f 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/crypto/Hkdf.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/crypto/Hkdf.kt @@ -21,6 +21,7 @@ package com.vitorpamplona.quartz.nip44Encryption.crypto import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.utils.equalsConstantTime import com.vitorpamplona.quartz.utils.mac.MacInstance class Hkdf( @@ -194,7 +195,7 @@ class Hkdf( mac.update(ciphertext) val calculatedMac = mac.doFinal() - check(calculatedMac.contentEquals(ciphertextMac)) { + check(calculatedMac.equalsConstantTime(ciphertextMac)) { "Invalid Mac: Calculated ${calculatedMac.toHexKey()}, decoded: ${ciphertextMac.toHexKey()}" } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/ConstantTime.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/ConstantTime.kt new file mode 100644 index 0000000000..b9b0ccb757 --- /dev/null +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/utils/ConstantTime.kt @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.utils + +/** + * Constant-time byte-array equality for MAC / authentication-tag comparison. + * + * Unlike [ByteArray.contentEquals], this does not short-circuit on the first + * differing byte, so its running time does not depend on how many leading bytes + * of a candidate MAC/tag are correct. Comparing MACs with an early-exit check + * exposes a timing side channel that can, in principle, let an attacker recover a + * valid tag byte-by-byte and forge messages (the classic Keyczar/XBox-360 class + * of bug). Use this for every secret-dependent equality check (HMAC tags, Poly1305 + * tags, reset tokens); ordinary non-secret comparisons can keep `contentEquals`. + * + * The length check leaks only the (public, fixed) tag length, not its contents. + */ +fun ByteArray.equalsConstantTime(other: ByteArray): Boolean { + if (this.size != other.size) return false + var diff = 0 + for (i in indices) { + diff = diff or (this[i].toInt() xor other[i].toInt()) + } + return diff == 0 +} diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCacheTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCacheTest.kt new file mode 100644 index 0000000000..e0e97e64e4 --- /dev/null +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCacheTest.kt @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.quartz.nip44Encryption + +import kotlin.test.Test +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class SharedKeyCacheTest { + /** + * The cache maps (privateKey, pubKey) to a shared/conversation key. If two + * DISTINCT peer pubkeys can map to the same cache slot, the cache returns one + * peer's secret for a message intended for the other — a silent wrong-key + * encryption/decryption (confidentiality break). + * + * `pubA` and `pubB` below are engineered to collide under a 31-multiplier + * polynomial hash of their bytes: bumping byte[31] by +31 and byte[30] by -1 + * leaves `31 * acc + byte` unchanged (31^1 * (-1) + 31^0 * (+31) == 0). They + * are otherwise different keys, so a correct cache MUST treat them as distinct. + */ + @Test + fun collidingPubKeysMustNotShareCacheEntry() { + val cache = SharedKeyCache() + val priv = ByteArray(32) { 1 } + + val pubA = + ByteArray(32).also { + it[30] = 0x10 + it[31] = 0x00 + } + val pubB = + ByteArray(32).also { + it[30] = 0x0F + it[31] = 0x1F + } + + // Sanity: these are genuinely different peer public keys. + assertTrue(!pubA.contentEquals(pubB), "test setup: pubkeys must differ") + + val secretForA = ByteArray(32) { 0xAA.toByte() } + cache.add(priv, pubA, secretForA) + + // The secret cached for peer A must never be handed back for peer B. + assertNull( + cache.get(priv, pubB), + "cache returned peer A's shared secret when asked for peer B (hash collision leaks the wrong key)", + ) + + // The legitimate lookup must still work. + assertTrue(secretForA.contentEquals(cache.get(priv, pubA)!!)) + } + + @Test + fun distinctPeersGetDistinctEntries() { + val cache = SharedKeyCache() + val priv = ByteArray(32) { 7 } + val pub1 = ByteArray(32) { 2 } + val pub2 = ByteArray(32) { 3 } + val s1 = ByteArray(32) { 0x11 } + val s2 = ByteArray(32) { 0x22 } + + cache.add(priv, pub1, s1) + cache.add(priv, pub2, s2) + + assertTrue(s1.contentEquals(cache.get(priv, pub1)!!)) + assertTrue(s2.contentEquals(cache.get(priv, pub2)!!)) + } +} From a59bc6d82d4b495945aaca25dba9c8ce3f654be4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 20:13:59 +0000 Subject: [PATCH 2/2] refactor(nip44): use a content-addressed key object for the shared-key cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the hex-string cache key with a small CacheKey holding the raw (privateKey, pubKey) references — no byte copy and no ~400-byte hex String per lookup. The precomputed Int hash is only a bucket selector; equals() does the authoritative full-content comparison, so collisions share a bucket and are disambiguated rather than returning the wrong peer's secret. Same value-type immutability contract already relied on by X25519KeyPair as a map key. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PY7kpiTcFiDshYqBsQ5KVC --- .../quartz/nip44Encryption/SharedKeyCache.kt | 53 +++++++++++++------ 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt index d613695448..a74614dda6 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt @@ -21,37 +21,58 @@ package com.vitorpamplona.quartz.nip44Encryption import androidx.collection.LruCache -import com.vitorpamplona.quartz.nip01Core.core.toHexKey class SharedKeyCache { - // Keyed by the full (privateKey || pubKey) content, NOT by a 32-bit hashCode. - // A hashCode key silently collides: two distinct peers whose (priv, pub) bytes - // hash to the same Int would share a cache slot, so `get` could return one - // peer's conversation key for a message meant for the other — a silent - // wrong-key encrypt/decrypt. The polynomial hash that was used here collides - // independently of the private key, so an attacker could grind a pubkey that - // aliases a victim's contact. Keying on the raw bytes removes the collision. - private val sharedKeyCache = LruCache(200) + // Keyed by the full (privateKey, pubKey) content via [CacheKey], NOT by a bare + // 32-bit hashCode. Using a hashCode as the whole map key silently collides: + // two distinct peers whose (priv, pub) bytes hash to the same Int would share a + // slot, so `get` could return one peer's conversation key for a message meant + // for the other — a silent wrong-key encrypt/decrypt (and the polynomial hash + // that was used collides independently of the private key, so a pubkey aliasing + // a victim's contact is grindable). [CacheKey] keeps the cheap Int hash only as + // a bucket selector and disambiguates collisions with a full contentEquals, so + // it stays correct while avoiding the per-lookup allocation of a hex String key. + private val sharedKeyCache = LruCache(200) fun clearCache() { sharedKeyCache.evictAll() } - private fun cacheKey( - privateKey: ByteArray, - pubKey: ByteArray, - ): String = privateKey.toHexKey() + pubKey.toHexKey() - fun get( privateKey: ByteArray, pubKey: ByteArray, - ): ByteArray? = sharedKeyCache[cacheKey(privateKey, pubKey)] + ): ByteArray? = sharedKeyCache[CacheKey(privateKey, pubKey)] fun add( privateKey: ByteArray, pubKey: ByteArray, secret: ByteArray, ) { - sharedKeyCache.put(cacheKey(privateKey, pubKey), secret) + sharedKeyCache.put(CacheKey(privateKey, pubKey), secret) + } + + /** + * Content-addressed cache key holding the raw key references (no byte copy, no + * hex string). The precomputed [hash] is only a bucket selector — [equals] does + * the authoritative full-content comparison, so hash collisions can never return + * the wrong peer's secret. Callers must treat the passed arrays as immutable + * (the same value-type contract [com.vitorpamplona.quartz.marmot.mls.crypto.X25519KeyPair] + * relies on when used as a map key). + */ + private class CacheKey( + val privateKey: ByteArray, + val pubKey: ByteArray, + ) { + private val hash = privateKey.contentHashCode() * 31 + pubKey.contentHashCode() + + override fun hashCode(): Int = hash + + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is CacheKey) return false + return hash == other.hash && + privateKey.contentEquals(other.privateKey) && + pubKey.contentEquals(other.pubKey) + } } }