Merge pull request #3999

d2e26c2 add and use constant time 32 byte equality function (moneromooo-monero)

9 files changed, 130 insertions, 14 deletions

diff --git a/README.md b/README.md
index f4e034d0c..ee7c112e7 100644
--- a/README.md
+++ b/README.md
@@ -139,7 +139,7 @@ library archives (`.a`).
 | libzmq       | 3.0.0         | NO       | `libzmq3-dev`      | `zeromq`     | `cppzmq-devel`    | NO       | ZeroMQ library |
 | OpenPGM      | ?             | NO       | `libpgm-dev`       | `libpgm`     | `openpgm-devel`   | NO       | For ZeroMQ     |
 | libunbound   | 1.4.16        | YES      | `libunbound-dev`   | `unbound`    | `unbound-devel`   | NO       | DNS resolver   |
-| libsodium    | ?             | NO       | `libsodium-dev`    | ?            | `libsodium-devel` | NO       | libsodium      |
+| libsodium    | ?             | NO       | `libsodium-dev`    | ?            | `libsodium-devel` | NO       | cryptography   |
 | libunwind    | any           | NO       | `libunwind8-dev`   | `libunwind`  | `libunwind-devel` | YES      | Stack traces   |
 | liblzma      | any           | NO       | `liblzma-dev`      | `xz`         | `xz-devel`        | YES      | For libunwind  |
 | libreadline  | 6.3.0         | NO       | `libreadline6-dev` | `readline`   | `readline-devel`  | YES      | Input editing  |
diff --git a/src/crypto/CMakeLists.txt b/src/crypto/CMakeLists.txt
index 71dcedcab..0c635e7cb 100644
--- a/src/crypto/CMakeLists.txt
+++ b/src/crypto/CMakeLists.txt
@@ -78,6 +78,7 @@ target_link_libraries(cncrypto
   PUBLIC
     epee
     ${Boost_SYSTEM_LIBRARY}
+    ${SODIUM_LIBRARY}
   PRIVATE
     ${EXTRA_LIBRARIES})
 
diff --git a/src/crypto/crypto.h b/src/crypto/crypto.h
index c1576a218..33cc0a25a 100644
--- a/src/crypto/crypto.h
+++ b/src/crypto/crypto.h
@@ -283,6 +283,6 @@ namespace crypto {
 }
 
 CRYPTO_MAKE_HASHABLE(public_key)
-CRYPTO_MAKE_HASHABLE(secret_key)
+CRYPTO_MAKE_HASHABLE_CONSTANT_TIME(secret_key)
 CRYPTO_MAKE_HASHABLE(key_image)
 CRYPTO_MAKE_COMPARABLE(signature)
diff --git a/src/crypto/generic-ops.h b/src/crypto/generic-ops.h
index 62bc758c9..42b98706e 100644
--- a/src/crypto/generic-ops.h
+++ b/src/crypto/generic-ops.h
@@ -33,19 +33,30 @@
 #include <cstddef>
 #include <cstring>
 #include <functional>
+#include <sodium/crypto_verify_32.h>
 
 #define CRYPTO_MAKE_COMPARABLE(type) \
 namespace crypto { \
   inline bool operator==(const type &_v1, const type &_v2) { \
-    return std::memcmp(&_v1, &_v2, sizeof(type)) == 0; \
+    return !memcmp(&_v1, &_v2, sizeof(_v1)); \
   } \
   inline bool operator!=(const type &_v1, const type &_v2) { \
-    return std::memcmp(&_v1, &_v2, sizeof(type)) != 0; \
+    return !operator==(_v1, _v2); \
   } \
 }
 
-#define CRYPTO_MAKE_HASHABLE(type) \
-CRYPTO_MAKE_COMPARABLE(type) \
+#define CRYPTO_MAKE_COMPARABLE_CONSTANT_TIME(type) \
+namespace crypto { \
+  inline bool operator==(const type &_v1, const type &_v2) { \
+    static_assert(sizeof(_v1) == 32, "constant time comparison is only implenmted for 32 bytes"); \
+    return crypto_verify_32((const unsigned char*)&_v1, (const unsigned char*)&_v2) == 0; \
+  } \
+  inline bool operator!=(const type &_v1, const type &_v2) { \
+    return !operator==(_v1, _v2); \
+  } \
+}
+
+#define CRYPTO_DEFINE_HASH_FUNCTIONS(type) \
 namespace crypto { \
   static_assert(sizeof(std::size_t) <= sizeof(type), "Size of " #type " must be at least that of size_t"); \
   inline std::size_t hash_value(const type &_v) { \
@@ -60,3 +71,12 @@ namespace std { \
     } \
   }; \
 }
+
+#define CRYPTO_MAKE_HASHABLE(type) \
+CRYPTO_MAKE_COMPARABLE(type) \
+CRYPTO_DEFINE_HASH_FUNCTIONS(type)
+
+#define CRYPTO_MAKE_HASHABLE_CONSTANT_TIME(type) \
+CRYPTO_MAKE_COMPARABLE_CONSTANT_TIME(type) \
+CRYPTO_DEFINE_HASH_FUNCTIONS(type)
+
diff --git a/src/device/device_ledger.cpp b/src/device/device_ledger.cpp
index 7a34dad5e..c4e9e40b7 100644
--- a/src/device/device_ledger.cpp
+++ b/src/device/device_ledger.cpp
@@ -136,7 +136,8 @@ namespace hw {
     }
 
     bool operator==(const crypto::key_derivation &d0, const crypto::key_derivation &d1) {
-      return !memcmp(&d0, &d1, sizeof(d0));
+      static_assert(sizeof(crypto::key_derivation) == 32, "key_derivation must be 32 bytes");
+      return !crypto_verify_32((const unsigned char*)&d0, (const unsigned char*)&d1);
     }
 
     /* ===================================================================== */
diff --git a/src/ringct/rctTypes.h b/src/ringct/rctTypes.h
index a3ccf2e85..452a68eb2 100644
--- a/src/ringct/rctTypes.h
+++ b/src/ringct/rctTypes.h
@@ -36,6 +36,7 @@
 #include <vector>
 #include <iostream>
 #include <cinttypes>
+#include <sodium/crypto_verify_32.h>
 
 extern "C" {
 #include "crypto/crypto-ops.h"
@@ -81,7 +82,7 @@ namespace rct {
         unsigned char operator[](int i) const {
             return bytes[i];
         }
-        bool operator==(const key &k) const { return !memcmp(bytes, k.bytes, sizeof(bytes)); }
+        bool operator==(const key &k) const { return !crypto_verify_32(bytes, k.bytes); }
         unsigned char bytes[32];
     };
     typedef std::vector<key> keyV; //vector of keys
@@ -524,16 +525,16 @@ namespace rct {
     static inline const crypto::secret_key rct2sk(const rct::key &k) { return (const crypto::secret_key&)k; }
     static inline const crypto::key_image rct2ki(const rct::key &k) { return (const crypto::key_image&)k; }
     static inline const crypto::hash rct2hash(const rct::key &k) { return (const crypto::hash&)k; }
-    static inline bool operator==(const rct::key &k0, const crypto::public_key &k1) { return !memcmp(&k0, &k1, 32); }
-    static inline bool operator!=(const rct::key &k0, const crypto::public_key &k1) { return memcmp(&k0, &k1, 32); }
+    static inline bool operator==(const rct::key &k0, const crypto::public_key &k1) { return !crypto_verify_32(k0.bytes, (const unsigned char*)&k1); }
+    static inline bool operator!=(const rct::key &k0, const crypto::public_key &k1) { return crypto_verify_32(k0.bytes, (const unsigned char*)&k1); }
 }
 
 
 namespace cryptonote {
-    static inline bool operator==(const crypto::public_key &k0, const rct::key &k1) { return !memcmp(&k0, &k1, 32); }
-    static inline bool operator!=(const crypto::public_key &k0, const rct::key &k1) { return memcmp(&k0, &k1, 32); }
-    static inline bool operator==(const crypto::secret_key &k0, const rct::key &k1) { return !memcmp(&k0, &k1, 32); }
-    static inline bool operator!=(const crypto::secret_key &k0, const rct::key &k1) { return memcmp(&k0, &k1, 32); }
+    static inline bool operator==(const crypto::public_key &k0, const rct::key &k1) { return !crypto_verify_32((const unsigned char*)&k0, k1.bytes); }
+    static inline bool operator!=(const crypto::public_key &k0, const rct::key &k1) { return crypto_verify_32((const unsigned char*)&k0, k1.bytes); }
+    static inline bool operator==(const crypto::secret_key &k0, const rct::key &k1) { return !crypto_verify_32((const unsigned char*)&k0, k1.bytes); }
+    static inline bool operator!=(const crypto::secret_key &k0, const rct::key &k1) { return crypto_verify_32((const unsigned char*)&k0, k1.bytes); }
 }
 
 namespace rct {
diff --git a/tests/performance_tests/equality.h b/tests/performance_tests/equality.h
new file mode 100644
index 000000000..8d24d7da7
--- /dev/null
+++ b/tests/performance_tests/equality.h
@@ -0,0 +1,72 @@
+// Copyright (c) 2014-2018, The Monero Project
+// 
+// All rights reserved.
+// 
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+// 
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+//    conditions and the following disclaimer.
+// 
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+//    of conditions and the following disclaimer in the documentation and/or other
+//    materials provided with the distribution.
+// 
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+//    used to endorse or promote products derived from this software without specific
+//    prior written permission.
+// 
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+// 
+// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers
+
+#pragma once
+
+#include <string.h>
+#include <sodium/crypto_verify_32.h>
+
+struct memcmp32
+{
+  static const size_t loop_count = 1000000000;
+  static int call(const unsigned char *k0, const unsigned char *k1){ return memcmp(k0, k1, 32); }
+};
+
+struct verify32
+{
+  static const size_t loop_count = 10000000;
+  static int call(const unsigned char *k0, const unsigned char *k1){ return crypto_verify_32(k0, k1); }
+};
+
+template<typename f, bool equal>
+class test_equality
+{
+public:
+  static const size_t loop_count = f::loop_count;
+
+  bool init()
+  {
+    for (int n = 0; n < 32; ++n)
+      k0[n] = n;
+    for (int n = 0; n < 32; ++n)
+      k1[n] = equal ? n : n + 1;
+    return true;
+  }
+
+  bool test()
+  {
+    return equal == !f::call(k0, k1);
+  }
+
+private:
+  unsigned char k0[32];
+  unsigned char k1[32];
+};
+
diff --git a/tests/performance_tests/main.cpp b/tests/performance_tests/main.cpp
index bc3622ea8..1733e3409 100644
--- a/tests/performance_tests/main.cpp
+++ b/tests/performance_tests/main.cpp
@@ -51,6 +51,7 @@
 #include "sc_reduce32.h"
 #include "cn_fast_hash.h"
 #include "rct_mlsag.h"
+#include "equality.h"
 
 namespace po = boost::program_options;
 
@@ -151,6 +152,11 @@ int main(int argc, char** argv)
   TEST_PERFORMANCE3(filter, test_ringct_mlsag, 1, 10, true);
   TEST_PERFORMANCE3(filter, test_ringct_mlsag, 1, 100, true);
 
+  TEST_PERFORMANCE2(filter, test_equality, memcmp32, true);
+  TEST_PERFORMANCE2(filter, test_equality, memcmp32, false);
+  TEST_PERFORMANCE2(filter, test_equality, verify32, false);
+  TEST_PERFORMANCE2(filter, test_equality, verify32, false);
+
   std::cout << "Tests finished. Elapsed time: " << timer.elapsed_ms() / 1000 << " sec" << std::endl;
 
   return 0;
diff --git a/tests/unit_tests/crypto.cpp b/tests/unit_tests/crypto.cpp
index 9e1680568..29fa88f9d 100644
--- a/tests/unit_tests/crypto.cpp
+++ b/tests/unit_tests/crypto.cpp
@@ -81,3 +81,18 @@ TEST(Crypto, null_keys)
   ASSERT_EQ(memcmp(crypto::null_skey.data, zero, 32), 0);
   ASSERT_EQ(memcmp(crypto::null_pkey.data, zero, 32), 0);
 }
+
+TEST(Crypto, verify_32)
+{
+  // all bytes are treated the same, so we can brute force just one byte
+  unsigned char k0[32] = {0}, k1[32] = {0};
+  for (unsigned int i0 = 0; i0 < 256; ++i0)
+  {
+    k0[0] = i0;
+    for (unsigned int i1 = 0; i1 < 256; ++i1)
+    {
+      k1[0] = i1;
+      ASSERT_EQ(!crypto_verify_32(k0, k1), i0 == i1);
+    }
+  }
+}