Merge pull request #3833 from vitorpamplona/claude/crypto-security-review-f13uy2

Fix SharedKeyCache hash collision vulnerability & add constant-time MAC comparison

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 {
     /**

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()}"
         }
     }

quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip44Encryption/SharedKeyCache.kt

@@ -23,32 +23,56 @@ package com.vitorpamplona.quartz.nip44Encryption
 import androidx.collection.LruCache
 
 class SharedKeyCache {
-    private val sharedKeyCache = LruCache<Int, ByteArray>(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<CacheKey, ByteArray>(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
-    }
-
     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)
+    }
+
+    /**
+     * 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)
+        }
     }
 }

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()}"
             }
         }

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
+}

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)!!))
+    }
+}