Is/1481 improve randomness and ecdsa signatures

secure_enclave/EnclaveCommon.h

@@ -64,8 +64,6 @@ EXTERNC void LOG_TRACE(const char *_msg);
 
 extern uint32_t globalLogLevel_;
 
-extern unsigned char *globalRandom;
-
 extern domain_parameters curve;
 
 #define SAFE_FREE(__X__)                                                       \

secure_enclave/Signature.c

@@ -127,16 +127,20 @@ void signature_sign(signature sig, mpz_t message, mpz_t private_key, domain_para
 
     SAFE_CHAR_BUF(rand_char, 32);
 
-    get_global_random((unsigned char *) rand_char, 32);
-
     signature_sign_start:
 
 
     get_global_random((unsigned char *) rand_char, 32);
 
     mpz_import(seed, 32, 1, sizeof(rand_char[0]), 0, 0, rand_char);
 
-    mpz_mod(k, seed, curve->p);
+    // Do not compute seed mod n: the 256-bit source range is not an exact
+    // multiple of the curve order, so modulo reduction would make some nonce
+    // values slightly more likely than others. Rejection sampling preserves a
+    // uniform distribution over the valid ECDSA nonce range [1, n - 1].
+    if (mpz_sgn(seed) == 0 || mpz_cmp(seed, curve->n) >= 0)
+        goto signature_sign_start;
+    mpz_set(k, seed);
 
     //Calculate x
     point_multiplication(Q, k, curve->G, curve);
@@ -148,17 +152,22 @@ void signature_sign(signature sig, mpz_t message, mpz_t private_key, domain_para
         goto signature_sign_start;
 
 
-    //Calculate s
-    //s = k¯¹(e+d*r) mod n = (k¯¹ mod n) * ((e+d*r) mod n) mod n
-    //number_theory_inverse(t1, k, curve->n);//t1 = k¯¹ mod n
-    mpz_invert(t1, k, curve->n);
-    mpz_mul(t2, private_key, r);    //t2 = d*r
-    mpz_add(t3, message, t2);    //t3 = e+t2
-    mpz_mod(t4, t3, curve->n);    //t2 = t3 mod n
-    mpz_mul(t5, t4, t1);        //t3 = t2 * t1
-    mpz_mod(s, t5, curve->n);    //s = t3 mod n
+    // Calculate s
+    // s = k¯¹(e+d*r) mod n = (k¯¹ mod n) * ((e+d*r) mod n) mod n
+
+    if (mpz_invert(t1, k, curve->n) == 0) // t1 = k¯¹ mod n
+        goto signature_sign_start; // should never happen - only a defensive check
+
+    mpz_mul(t2, private_key, r);    // t2 = d*r
+    mpz_add(t3, message, t2);       // t3 = e+t2
+    mpz_mod(t4, t3, curve->n);      // t4 = t3 mod n
+    mpz_mul(t5, t4, t1);            // t5 = t4 * t1
+    mpz_mod(s, t5, curve->n);       // s = t5 mod n
+
+    if (mpz_sgn(s) == 0) // Start over if s=0
+        goto signature_sign_start;
 
-    //Calculate v
+    // Calculate v
 
     mpz_mod_ui(rem, Q->y, 2);
 
@@ -224,9 +233,9 @@ bool signature_verify(mpz_t message, signature sig, point public_key, domain_par
     bool result = false;
 
 
-    if (mpz_cmp(sig->r, one) < 0 &&
-        mpz_cmp(curve->n, sig->r) <= 0 &&
-        mpz_cmp(sig->s, one) < 0 &&
+    if (mpz_cmp(sig->r, one) < 0 ||
+        mpz_cmp(curve->n, sig->r) <= 0 ||
+        mpz_cmp(sig->s, one) < 0 ||
         mpz_cmp(curve->n, sig->s) <= 0) {
         goto clean;
     }

secure_enclave/secure_enclave.c

@@ -41,6 +41,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <assert.h>
 
 #include "secure_enclave_t.h"
+#include "sgx_error.h"
 #include "sgx_tcrypto.h"
 #include "sgx_tseal.h"
 #include <sgx_tgmp.h>
@@ -118,8 +119,6 @@ void *reallocate_function(void *, size_t, size_t);
 
 void free_function(void *, size_t);
 
-unsigned char *globalRandom = NULL;
-
 // -----------------------------------------------------------------------------------------
 // Helper functions
 // -----------------------------------------------------------------------------------------
@@ -216,15 +215,17 @@ void trustedEnclaveInit(uint64_t _logLevel) {
 
     enclave_init();
 
-    LOG_INFO("Reading random");
-
-    globalRandom = calloc(32,1);
+    LOG_INFO("Verifying hardware RNG");
 
-    int ret = sgx_read_rand(globalRandom, 32);
+    // Fail fast at init if the hardware RNG is unavailable, rather than
+    // discovering it later when generating keys. get_global_random reads from
+    // the same source on every call and also fails closed.
+    unsigned char rngSelfTest[32];
+    sgx_status_t ret = sgx_read_rand(rngSelfTest, sizeof(rngSelfTest));
 
     if(ret != SGX_SUCCESS)
     {
-        LOG_ERROR("sgx_read_rand failed. Aboring enclave.");
+        LOG_ERROR("sgx_read_rand failed. Aborting enclave.");
         abort();
     }
 
@@ -244,7 +245,6 @@ void trustedEnclaveInit(uint64_t _logLevel) {
 }
 
 void trustedEnclaveClear() {
-    free(globalRandom);
     enclave_clear();
 }
 
@@ -284,28 +284,21 @@ void *reallocate_function(void *ptr, size_t osize, size_t nsize) {
     return (void *) nptr;
 }
 
-volatile uint64_t counter = 0;
-
 void get_global_random(unsigned char *_randBuff, uint64_t _size) {
-    char errString[ENCLAVE_BUF_LEN];
-    int status;
-    int *errStatus = &status;
-
-    INIT_ERROR_STATE
+    // Randomness is read directly from the CPU's RDRAND-backed DRNG on every
+    // call. This is stateless and thread-safe: the hardware serves a distinct
+    // value per request across all logical cores. It is cryptographically unlikely
+    // that two threads will receive the same value
+    if (_randBuff == NULL || _size < 1 || _size > 32) {
+        LOG_ERROR("get_global_random called with invalid arguments. Aborting enclave.");
+        abort();
+    }
 
-    CHECK_STATE(_size <= 32)
-    CHECK_STATE(_randBuff);
-
-    const uint64_t counter_snapshot = ++counter;
-    sgx_sha_state_handle_t shaStateHandle;
-    CHECK_STATE(sgx_sha256_init(&shaStateHandle) == SGX_SUCCESS);
-    CHECK_STATE(sgx_sha256_update(globalRandom, 32, shaStateHandle) == SGX_SUCCESS);
-    CHECK_STATE(sgx_sha256_update((const uint8_t *)&counter_snapshot, sizeof(counter_snapshot), shaStateHandle) == SGX_SUCCESS);
-    unsigned char tmpBuffer[32];
-    CHECK_STATE(sgx_sha256_get_hash(shaStateHandle, (sgx_sha256_hash_t *)tmpBuffer) == SGX_SUCCESS);
-    CHECK_STATE(sgx_sha256_close(shaStateHandle) == SGX_SUCCESS);
-
-    memcpy(_randBuff, tmpBuffer, _size);
+    sgx_status_t status = sgx_read_rand(_randBuff, _size);
+    if (status != SGX_SUCCESS) {
+        LOG_ERROR("sgx_read_rand failed in get_global_random. Aborting enclave.");
+        abort();
+    }
 }
 
 static void sealHexSEK(int *errStatus, char *errString,

tests/integration/keys/ECDSAIntegrationTests.cpp

@@ -26,18 +26,55 @@
 #include "zmq_src/ZMQClient.h"
 
 #include <algorithm>
+#include <atomic>
+#include <chrono>
 #include <cstdint>
+#include <future>
 #include <json/value.h>
 #include <jsonrpccpp/client/connectors/httpclient.h>
 #include <jsonrpccpp/common/exception.h>
 #include <memory>
+#include <set>
 #include <string>
 #include <thread>
+#include <utility>
 #include <vector>
 
 using namespace jsonrpc;
 using namespace std;
 
+namespace {
+
+struct EcdsaKeygenResult {
+  sgx_status_t status = SGX_ERROR_UNEXPECTED;
+  int errStatus = -1;
+  string pubKeyX;
+  string pubKeyY;
+};
+
+EcdsaKeygenResult generateEcdsaKeyOnceForRaceCheck() {
+  vector<char> errMsg(BUF_LEN, 0);
+  int errStatus = 0;
+  vector<uint8_t> encrPrivKey(BUF_LEN, 0);
+  vector<char> pubKeyX(BUF_LEN, 0);
+  vector<char> pubKeyY(BUF_LEN, 0);
+  uint64_t encLen = 0;
+  int exportable = 0;
+
+  sgx_status_t status = trustedGenerateEcdsaKey(
+      eid, &errStatus, errMsg.data(), &exportable, encrPrivKey.data(), &encLen,
+      pubKeyX.data(), pubKeyY.data());
+
+  EcdsaKeygenResult result;
+  result.status = status;
+  result.errStatus = errStatus;
+  result.pubKeyX = string(pubKeyX.data());
+  result.pubKeyY = string(pubKeyY.data());
+  return result;
+}
+
+} // namespace
+
 class TestFixtureZMQSign {
 public:
   TestFixtureZMQSign() {
@@ -231,3 +268,52 @@ TEST_CASE_METHOD(TestFixtureZMQSign, "ZMQ-ecdsa",
   std::for_each(workers.begin(), workers.end(),
                 [](std::thread &t) { t.join(); });
 }
+
+TEST_CASE_METHOD(TestFixture, "ECDSA global_random concurrency distinctness",
+                 "[integration][ecdsa][security]") {
+  constexpr int kThreads = 8;
+  // A single burst of threads may not overlap on the RNG on any given run, so
+  // repeat the burst several times to raise the odds of real contention. The
+  // distinctness invariant holds across every attempt, not just within one.
+  constexpr int kAttempts = 15;
+
+  // Accumulates every public key produced across all attempts.
+  set<pair<string, string>> seenPubKeys;
+
+  for (int attempt = 0; attempt < kAttempts; ++attempt) {
+    INFO("attempt " << attempt);
+
+    atomic<bool> start{false};
+
+    auto worker = [&start]() {
+      while (!start.load(memory_order_acquire)) {
+        this_thread::yield();
+      }
+      return generateEcdsaKeyOnceForRaceCheck();
+    };
+
+    vector<future<EcdsaKeygenResult>> futures;
+    futures.reserve(kThreads);
+    for (int i = 0; i < kThreads; ++i) {
+      futures.push_back(async(launch::async, worker));
+    }
+    // Release all threads at once to maximize simultaneous pressure on the RNG.
+    start.store(true, memory_order_release);
+
+    for (auto &f : futures) {
+      REQUIRE(f.wait_for(chrono::seconds(10)) == future_status::ready);
+
+      EcdsaKeygenResult r = f.get();
+      REQUIRE(r.status == SGX_SUCCESS);
+      REQUIRE(r.errStatus == SGX_SUCCESS);
+      REQUIRE_FALSE(r.pubKeyX.empty());
+      REQUIRE_FALSE(r.pubKeyY.empty());
+
+      // Fail if this exact key was already produced by any other thread, in
+      // this attempt or an earlier one — that is a nonce/key collision.
+      auto pubKey = make_pair(r.pubKeyX, r.pubKeyY);
+      REQUIRE(seenPubKeys.count(pubKey) == 0);
+      seenPubKeys.insert(pubKey);
+    }
+  }
+}