From d2e26c23f3caa7928c46c3a6bded85fdc1a68cb3 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 13 Jun 2018 18:23:06 +0100 Subject: [PATCH] add and use constant time 32 byte equality function --- README.md | 2 +- src/crypto/CMakeLists.txt | 1 + src/crypto/crypto.h | 2 +- src/crypto/generic-ops.h | 28 ++++++++++-- src/device/device_ledger.cpp | 3 +- src/ringct/rctTypes.h | 15 ++++--- tests/performance_tests/equality.h | 72 ++++++++++++++++++++++++++++++ tests/performance_tests/main.cpp | 6 +++ tests/unit_tests/crypto.cpp | 15 +++++++ 9 files changed, 130 insertions(+), 14 deletions(-) create mode 100644 tests/performance_tests/equality.h 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 #include #include +#include #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 #include #include +#include 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 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 +#include + +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 +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); + } + } +}