From 7c1aac5dc1697974c4013fb91ca71e5738cd0ff2 Mon Sep 17 00:00:00 2001 From: Eugene Kabanov Date: Sat, 8 Aug 2020 09:53:33 +0300 Subject: [PATCH] Use constant-time comparison for keys and signatures. (#299) --- libp2p/crypto/ecnist.nim | 37 +++++++----- libp2p/crypto/ed25519/ed25519.nim | 8 +-- libp2p/crypto/rsa.nim | 97 +++++++++++++++---------------- 3 files changed, 75 insertions(+), 67 deletions(-) diff --git a/libp2p/crypto/ecnist.nim b/libp2p/crypto/ecnist.nim index 30475ef4b..ad8312613 100644 --- a/libp2p/crypto/ecnist.nim +++ b/libp2p/crypto/ecnist.nim @@ -20,7 +20,7 @@ import bearssl import nimcrypto/utils import minasn1 export minasn1.Asn1Error -import stew/results +import stew/[results, ctops] export results const @@ -540,8 +540,8 @@ proc `==`*(pubkey1, pubkey2: EcPublicKey): bool = let op2 = pubkey2.getOffset() if op1 == -1 or op2 == -1: return false - result = equalMem(unsafeAddr pubkey1.buffer[op1], - unsafeAddr pubkey2.buffer[op2], pubkey1.key.qlen) + return CT.isEqual(pubkey1.buffer.toOpenArray(op1, pubkey1.key.qlen - 1), + pubkey2.buffer.toOpenArray(op2, pubkey2.key.qlen - 1)) proc `==`*(seckey1, seckey2: EcPrivateKey): bool = ## Returns ``true`` if both keys ``seckey1`` and ``seckey2`` are equal. @@ -560,19 +560,30 @@ proc `==`*(seckey1, seckey2: EcPrivateKey): bool = let op2 = seckey2.getOffset() if op1 == -1 or op2 == -1: return false - result = equalMem(unsafeAddr seckey1.buffer[op1], - unsafeAddr seckey2.buffer[op2], seckey1.key.xlen) + return CT.isEqual(seckey1.buffer.toOpenArray(op1, seckey1.key.xlen - 1), + seckey2.buffer.toOpenArray(op2, seckey2.key.xlen - 1)) -proc `==`*(sig1, sig2: EcSignature): bool = +proc `==`*(a, b: EcSignature): bool = ## Return ``true`` if both signatures ``sig1`` and ``sig2`` are equal. - if isNil(sig1) and isNil(sig2): - result = true - elif isNil(sig1) and (not isNil(sig2)): - result = false - elif isNil(sig2) and (not isNil(sig1)): - result = false + if isNil(a) and isNil(b): + true + elif isNil(a) and (not isNil(b)): + false + elif isNil(b) and (not isNil(a)): + false else: - result = (sig1.buffer == sig2.buffer) + # We need to cover all the cases because Signature initialization procedure + # do not perform any checks. + if len(a.buffer) == 0 and len(b.buffer) == 0: + true + elif len(a.buffer) == 0 and len(b.buffer) != 0: + false + elif len(b.buffer) == 0 and len(a.buffer) != 0: + false + elif len(a.buffer) != len(b.buffer): + false + else: + CT.isEqual(a.buffer, b.buffer) proc init*(key: var EcPrivateKey, data: openarray[byte]): Result[void, Asn1Error] = ## Initialize EC `private key` or `signature` ``key`` from ASN.1 DER binary diff --git a/libp2p/crypto/ed25519/ed25519.nim b/libp2p/crypto/ed25519/ed25519.nim index 3e69f4d1c..3c16b7cf6 100644 --- a/libp2p/crypto/ed25519/ed25519.nim +++ b/libp2p/crypto/ed25519/ed25519.nim @@ -15,7 +15,7 @@ import constants, bearssl import nimcrypto/[hash, sha2, utils] -import stew/results +import stew/[results, ctops] export results # This workaround needed because of some bugs in Nim Static[T]. @@ -1725,15 +1725,15 @@ proc getBytes*(sig: EdSignature): seq[byte] = @(sig.data) proc `==`*(eda, edb: EdPrivateKey): bool = ## Compare ED25519 `private key` objects for equality. - result = (eda.data == edb.data) + result = CT.isEqual(eda.data, edb.data) proc `==`*(eda, edb: EdPublicKey): bool = ## Compare ED25519 `public key` objects for equality. - result = (eda.data == edb.data) + result = CT.isEqual(eda.data, edb.data) proc `==`*(eda, edb: EdSignature): bool = ## Compare ED25519 `signature` objects for equality. - result = (eda.data == edb.data) + result = CT.isEqual(eda.data, edb.data) proc `$`*(key: EdPrivateKey): string = toHex(key.data) ## Return string representation of ED25519 `private key`. diff --git a/libp2p/crypto/rsa.nim b/libp2p/crypto/rsa.nim index f325bc45f..835c9370a 100644 --- a/libp2p/crypto/rsa.nim +++ b/libp2p/crypto/rsa.nim @@ -19,7 +19,7 @@ import nimcrypto/utils import bearssl import minasn1 export Asn1Error -import stew/results +import stew/[results, ctops] export results const @@ -662,81 +662,78 @@ proc `$`*(sig: RsaSignature): string = result.add(toHex(sig.buffer)) result.add(")") -proc cmp(a: openarray[byte], b: openarray[byte]): bool = - let alen = len(a) - let blen = len(b) - if alen == blen: - if alen == 0: - true - else: - var n = alen - var res = 0 - while n > 0: - dec(n) - res = res or int(a[n] xor b[n]) - (res == 0) - else: - false - proc `==`*(a, b: RsaPrivateKey): bool = ## Compare two RSA private keys for equality. ## ## Result is true if ``a`` and ``b`` are both ``nil`` or ``a`` and ``b`` are ## equal by value. if isNil(a) and isNil(b): - result = true + true elif isNil(a) and (not isNil(b)): - result = false + false elif isNil(b) and (not isNil(a)): - result = false + false else: if a.seck.nBitlen == b.seck.nBitlen: if cast[int](a.seck.nBitlen) > 0: - let r1 = cmp(getArray(a.buffer, a.seck.p, a.seck.plen), - getArray(b.buffer, b.seck.p, b.seck.plen)) - let r2 = cmp(getArray(a.buffer, a.seck.q, a.seck.qlen), - getArray(b.buffer, b.seck.q, b.seck.qlen)) - let r3 = cmp(getArray(a.buffer, a.seck.dp, a.seck.dplen), - getArray(b.buffer, b.seck.dp, b.seck.dplen)) - let r4 = cmp(getArray(a.buffer, a.seck.dq, a.seck.dqlen), - getArray(b.buffer, b.seck.dq, b.seck.dqlen)) - let r5 = cmp(getArray(a.buffer, a.seck.iq, a.seck.iqlen), - getArray(b.buffer, b.seck.iq, b.seck.iqlen)) - let r6 = cmp(getArray(a.buffer, a.pexp, a.pexplen), - getArray(b.buffer, b.pexp, b.pexplen)) - let r7 = cmp(getArray(a.buffer, a.pubk.n, a.pubk.nlen), - getArray(b.buffer, b.pubk.n, b.pubk.nlen)) - let r8 = cmp(getArray(a.buffer, a.pubk.e, a.pubk.elen), - getArray(b.buffer, b.pubk.e, b.pubk.elen)) - result = r1 and r2 and r3 and r4 and r5 and r6 and r7 and r8 + let r1 = CT.isEqual(getArray(a.buffer, a.seck.p, a.seck.plen), + getArray(b.buffer, b.seck.p, b.seck.plen)) + let r2 = CT.isEqual(getArray(a.buffer, a.seck.q, a.seck.qlen), + getArray(b.buffer, b.seck.q, b.seck.qlen)) + let r3 = CT.isEqual(getArray(a.buffer, a.seck.dp, a.seck.dplen), + getArray(b.buffer, b.seck.dp, b.seck.dplen)) + let r4 = CT.isEqual(getArray(a.buffer, a.seck.dq, a.seck.dqlen), + getArray(b.buffer, b.seck.dq, b.seck.dqlen)) + let r5 = CT.isEqual(getArray(a.buffer, a.seck.iq, a.seck.iqlen), + getArray(b.buffer, b.seck.iq, b.seck.iqlen)) + let r6 = CT.isEqual(getArray(a.buffer, a.pexp, a.pexplen), + getArray(b.buffer, b.pexp, b.pexplen)) + let r7 = CT.isEqual(getArray(a.buffer, a.pubk.n, a.pubk.nlen), + getArray(b.buffer, b.pubk.n, b.pubk.nlen)) + let r8 = CT.isEqual(getArray(a.buffer, a.pubk.e, a.pubk.elen), + getArray(b.buffer, b.pubk.e, b.pubk.elen)) + r1 and r2 and r3 and r4 and r5 and r6 and r7 and r8 else: - result = true + true + else: + false proc `==`*(a, b: RsaSignature): bool = ## Compare two RSA signatures for equality. if isNil(a) and isNil(b): - result = true + true elif isNil(a) and (not isNil(b)): - result = false + false elif isNil(b) and (not isNil(a)): - result = false + false else: - result = (a.buffer == b.buffer) + # We need to cover all the cases because Signature initialization procedure + # do not perform any checks. + if len(a.buffer) == 0 and len(b.buffer) == 0: + true + elif len(a.buffer) == 0 and len(b.buffer) != 0: + false + elif len(b.buffer) == 0 and len(a.buffer) != 0: + false + elif len(a.buffer) != len(b.buffer): + false + else: + CT.isEqual(a.buffer, b.buffer) proc `==`*(a, b: RsaPublicKey): bool = ## Compare two RSA public keys for equality. if isNil(a) and isNil(b): - result = true + true elif isNil(a) and (not isNil(b)): - result = false + false elif isNil(b) and (not isNil(a)): - result = false + false else: - let r1 = cmp(getArray(a.buffer, a.key.n, a.key.nlen), - getArray(b.buffer, b.key.n, b.key.nlen)) - let r2 = cmp(getArray(a.buffer, a.key.e, a.key.elen), - getArray(b.buffer, b.key.e, b.key.elen)) - result = r1 and r2 + let r1 = CT.isEqual(getArray(a.buffer, a.key.n, a.key.nlen), + getArray(b.buffer, b.key.n, b.key.nlen)) + let r2 = CT.isEqual(getArray(a.buffer, a.key.e, a.key.elen), + getArray(b.buffer, b.key.e, b.key.elen)) + (r1 and r2) proc sign*[T: byte|char](key: RsaPrivateKey, message: openarray[T]): RsaResult[RsaSignature] {.gcsafe.} =