backport some changes from ct study

CMakeLists.txt

@@ -15,7 +15,7 @@ set_property(GLOBAL PROPERTY obj_libs "")
 
 # Build with address sanitizer
 if(ADDRSAN)
-  string(APPEND EXTRA_CXX_FLAGS " -fsanitize=undefined,address,leak -fno-omit-frame-pointer")
+  string(APPEND EXTRA_C_CXX_FLAGS " -fsanitize=undefined,address,leak -fno-omit-frame-pointer")
   set(EXTRA_LDFLAGS " -fsanitize=undefined,address,leak")
 endif()
 
@@ -35,15 +35,15 @@ if(MEMSAN)
   set(LLVM_PRJ_INC ${LLVM_PRJ}/usr/local/include)
 
   # Add memory sanitizer instrumented libraries
-  set(CMAKE_ARGS_MEMCHECK_LIB "-stdlib=libc++")
+  set(CMAKE_ARGS_MEMCHECK_LIB "-stdlib=libc++ -L${LLVM_PRJ_LIB}")
   set(CMAKE_ARGS_MEMCHECK_INC "-isystem -I${LLVM_PRJ_INC} -I${LLVM_PRJ_INC}/c++/v1")
-  set(CMAKE_ARGS_MEMCHECK_FLAGS "-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -Wno-unused-command-line-argument")
+  set(CMAKE_ARGS_MEMCHECK_FLAGS "-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -Wno-unused-command-line-argument -fno-optimize-sibling-calls")
   # Enablin "keep-going" flag alows two things:
   # 1. Enables CT_EXPECT_UMR()/CT_REQUIRE_UMR() in tests. For some reason MSan will halt
   #    on error even if it expects UMR. And hence, CT can't be tested. This is probably a bug.
   # 2. reports all the errors from the run, not only the first one (don't fail-fast)
   string(APPEND CMAKE_ARGS_MEMCHECK_FLAGS " -mllvm -msan-keep-going=1")
-  set(EXTRA_CXX_FLAGS "${CMAKE_ARGS_MEMCHECK_FLAGS} ${CMAKE_ARGS_MEMCHECK_LIB} ${CMAKE_ARGS_MEMCHECK_INC} -DPQC_MEMSAN_BUILD")
+  set(EXTRA_C_CXX_FLAGS "${CMAKE_ARGS_MEMCHECK_FLAGS} ${CMAKE_ARGS_MEMCHECK_LIB} ${CMAKE_ARGS_MEMCHECK_INC} -DPQC_MEMSAN_BUILD")
   set(CXXLIBS_FOR_MEMORY_SANITIZER cxx cxxabi)
 endif()
 
@@ -57,7 +57,7 @@ if (NOT CMAKE_C_COMPILER_ID MATCHES "Clang")
 message(FATAL_ERROR "Constant time sanitizer requires Clang")
 endif()
 
-string(APPEND EXTRA_CXX_FLAGS " -DPQC_USE_CTSANITIZER")
+string(APPEND EXTRA_C_CXX_FLAGS " -DPQC_USE_CTSANITIZER")
 endif()
 
 # Contant time memory checks with CTGRIND (requires valgrind)
@@ -65,7 +65,7 @@ if (CTGRIND)
 if (MEMSAN OR CTSAN)
 message(FATAL_ERROR "Can't use memory sanitizer (MEMSAN) and CTGRIND")
 endif()
-string(APPEND EXTRA_CXX_FLAGS " -DPQC_USE_CTGRIND")
+string(APPEND EXTRA_C_CXX_FLAGS " -DPQC_USE_CTGRIND")
 endif()
 
 set(CMAKE_VERBOSE_MAKEFILE ON)
@@ -114,15 +114,15 @@ endif()
 
 # Global configuration
 
-string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-ignored-qualifiers \
-  -Wall \
-  -Werror \
-  -Wextra \
-  -Wshadow \
-  -Wno-variadic-macros \
-  -Wunused-result \
-  -Wno-unused-command-line-argument \
-  -Wno-undef")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wall")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Werror")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wextra")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wshadow")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-variadic-macros")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wunused-result")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-unused-command-line-argument")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-undef")
+string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-ignored-qualifiers")
 
 if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_GREATER 11.0)
 string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wno-stringop-overread \
@@ -135,8 +135,6 @@ include(.cmake/common.mk)
 # Control Debug/Release mode
 if(CMAKE_BUILD_TYPE_LOWER STREQUAL "debug")
   string(APPEND PQC_CMAKE_C_CXX_FLAGS " -g3 -O0 -Wno-unused")
-else()
-  string(APPEND PQC_CMAKE_C_CXX_FLAGS " -O3")
 endif()
 
 # Set CPU architecture
@@ -155,7 +153,7 @@ ExternalProject_Add(
   GIT_TAG        a3460d1aeeaa43fdf137a6adefef10ba0b59fe4b
   PREFIX         ${CMAKE_CURRENT_BINARY_DIR}/3rd/gtest
   INSTALL_DIR    ${CMAKE_CURRENT_BINARY_DIR}/3rd/gtest
-  CMAKE_ARGS     -DCMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/3rd/gtest -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_CXX_FLAGS=${EXTRA_CXX_FLAGS} -DCMAKE_C_FLAGS=${EXTRA_CXX_FLAGS} -Dgtest_disable_pthreads=ON
+  CMAKE_ARGS     -DCMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/3rd/gtest -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} -DCMAKE_CXX_FLAGS=${EXTRA_C_CXX_FLAGS} -DCMAKE_C_FLAGS=${EXTRA_C_CXX_FLAGS} -Dgtest_disable_pthreads=ON
 )
 if(MEMSAN)
 add_dependencies(gtest_project ${CXXLIBS_FOR_MEMORY_SANITIZER})
@@ -182,8 +180,8 @@ if(PQC_WEAK_RANDOMBYTES)
 endif()
 
 # Build CPU features
-set(CMAKE_C_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_CXX_FLAGS}")
-set(CMAKE_CXX_FLAGS "$${PQC_CMAKE_C_CXX_FLAGS} {EXTRA_CXX_FLAGS}")
+set(CMAKE_C_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_C_CXX_FLAGS}")
+set(CMAKE_CXX_FLAGS "$${PQC_CMAKE_C_CXX_FLAGS} {EXTRA_C_CXX_FLAGS}")
 set(BUILD_PIC ON CACHE BOOL "")
 add_subdirectory(3rd/cpu_features)
 
@@ -191,8 +189,8 @@ add_subdirectory(3rd/cpu_features)
 
 # Set C, CXX, and LD flags
 string(APPEND PQC_CMAKE_C_CXX_FLAGS " -Wpedantic")
-set(CMAKE_C_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_CXX_FLAGS}")
-set(CMAKE_CXX_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_CXX_FLAGS}")
+set(CMAKE_C_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_C_CXX_FLAGS}")
+set(CMAKE_CXX_FLAGS "${PQC_CMAKE_C_CXX_FLAGS} ${EXTRA_C_CXX_FLAGS}")
 string(APPEND LDFLAGS "${EXTRA_LDFLAGS}")
 
 include_directories(
@@ -328,6 +326,7 @@ add_library(
   src/common/randombytes.c
   src/common/sha2.c
   src/common/nistseedexpander.c
+  src/common/utils.c
   src/capi/pqapi.c
   ${COMMON_EXTRA_SRC})
 
@@ -391,6 +390,7 @@ target_link_directories(
 # github CI requires that
 add_dependencies(ut gtest_project)
 
+
 if(NOT CMAKE_BUILD_TYPE_LOWER STREQUAL "debug")
   # settings below are required by benchmark library
   set(CMAKE_BUILD_TYPE "Release" CACHE STRING "" FORCE)
@@ -403,8 +403,8 @@ if(NOT CMAKE_BUILD_TYPE_LOWER STREQUAL "debug")
   #endif()
   set(BENCHMARK_ENABLE_INSTALL OFF CACHE BOOL "" FORCE)
   set(BENCHMARK_ENABLE_EXCEPTIONS OFF CACHE BOOL "" FORCE)
-  set(CMAKE_C_FLAGS "${EXTRA_CXX_FLAGS}")
-  set(CMAKE_CXX_FLAGS "${EXTRA_CXX_FLAGS}")
+  set(CMAKE_C_FLAGS "${EXTRA_C_CXX_FLAGS}")
+  set(CMAKE_CXX_FLAGS "${EXTRA_C_CXX_FLAGS}")
   if (MEMSAN)
     set(BENCHMARK_USE_LIBCXX ON CACHE BOOL "" FORCE)
     # Since build requires C++20 it is safe to assume that std::regex is available.

src/common/ct_check.h

@@ -1,22 +1,30 @@
 #ifndef CT_CHECK_H
 #define CT_CHECK_H
 
+#include <stddef.h>
+
 // helper
 #define VOID(V) ((void)V)
 
 // Uses Clang's Memory Sanitizer
-#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature) && __has_feature(memory_sanitizer)
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
 #include <stddef.h>
 #include <sanitizer/msan_interface.h>
+#endif
 #elif defined(PQC_USE_CTGRIND)
+#include <stdio.h>
+#include <stdlib.h>
 #include <valgrind/valgrind.h>
 #include <valgrind/memcheck.h>
 #endif
 
 // Set sz bytes of memory starting at address p as uninitialized. Switches on constat time checks.
 static inline void ct_poison(const volatile void *p, size_t sz) {
-#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature) && __has_feature(memory_sanitizer)
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
 	__msan_allocated_memory(p,sz);
+#endif
 #elif defined(PQC_USE_CTGRIND)
 	VALGRIND_MAKE_MEM_UNDEFINED(p,sz);
 #else
@@ -26,8 +34,10 @@ static inline void ct_poison(const volatile void *p, size_t sz) {
 
 // Set sz bytes of memory starting at p as initialized. Switches off constat time checks.
 static inline void ct_purify(const volatile void *p, size_t sz) {
-#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature) && __has_feature(memory_sanitizer)
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
 	__msan_unpoison(p,sz);
+#endif
 #elif defined(PQC_USE_CTGRIND)
 	VALGRIND_MAKE_MEM_DEFINED(p,sz);
 #else
@@ -37,9 +47,11 @@ static inline void ct_purify(const volatile void *p, size_t sz) {
 
 // Function instructs memory sanitizer that code expects to do operation on unintialized memory.
 static inline void ct_expect_umr() {
-#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature) && __has_feature(memory_sanitizer)
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
 	__msan_set_expect_umr(1);
 #endif
+#endif
 }
 
 // Checks if action on unintialized memory has occured. If this is not a case
@@ -47,9 +59,42 @@ static inline void ct_expect_umr() {
 // MSan, the code needs to be compiled with `-mllvm -msan-keep-going=1` flags in order to work
 // correctly.
 static inline void ct_require_umr() {
-#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature) && __has_feature(memory_sanitizer)
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
 	__msan_set_expect_umr(0);
 #endif
+#endif
+}
+
+static inline void ct_print_shadow(const volatile void* p, size_t sz) {
+#if defined(PQC_USE_CTSANITIZER) && defined(__clang__) && defined(__has_feature)
+#if __has_feature(memory_sanitizer)
+	__msan_dump_shadow(p,sz);
+#endif
+#elif defined(PQC_USE_CTGRIND)
+	size_t r = 0;
+	uint8_t *b = NULL;
+
+	b = (uint8_t*)malloc(sz);
+	// crash if malloc fails
+	r = VALGRIND_GET_VBITS(p,b,sz);
+	if (r != 1) {
+		fprintf(stderr, "Can't get shadow memory [%s]\n",
+			(r==0)||(r==2)
+			?"INTERNAL" /*should not happen*/
+			:"NOT ADDRESABLE" /* some parts of p are not addressable.*/);
+		goto end;
+	}
+	for (r=0; r<sz; r++) {
+		fprintf(stderr, "%02X%c", ((const uint8_t*)b)[r], (r%8)?' ':' ');
+	}
+	fprintf(stderr, "\n");
+end:
+	free(b);
+
+#else
+	VOID(p),VOID(sz);
+#endif
 }
 
 #endif // CT_CHECK_H

src/common/utils.c

@@ -0,0 +1,13 @@
+#include <stdint.h>
+#include <stddef.h>
+
+// Constant time memcmp. Returns 0 if p==q, otherwise 1
+uint8_t ct_memcmp(const void *a, const void *b, size_t n) {
+    const uint8_t *pa = (uint8_t *) a, *pb = (uint8_t *) b;
+    uint8_t r = 0;
+
+    while (n--) { r |= *pa++ ^ *pb++; }
+    r   = (r >> 1) - r; // MSB == 1 iff r!=0
+    r >>= 7;
+    return r;
+}

src/common/utils.h

@@ -1,8 +1,14 @@
 #ifndef PQC_COMMON_UTILS_
 #define PQC_COMMON_UTILS_
 
+#include <stdint.h>
+#include <stddef.h>
 #include <cpuinfo_x86.h>
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 // Helper to stringify constants
 #define STR(x) STR_(x)
 #define STR_(x) #x
@@ -32,6 +38,23 @@
     (((uint16_t)(x)[0])<<8 |  \
      ((uint16_t)(x)[1])<<0)   \
 
-const X86Features * get_cpu_caps(void);
+#ifdef __cplusplus
+const cpu_features::X86Features*
+#else
+const X86Features*
+#endif
+get_cpu_caps(void);
+
+/**
+ * \brief Compares two arrays in constant time.
+ * \param [in] a first array
+ * \param [in] b second arrray
+ * \param [in] sz number of bytes to compare
+ * \returns 0 if arrays are equal, otherwise 1.
+ */
+uint8_t ct_memcmp(const void *p, const void *q, size_t n);
 
+#ifdef __cplusplus
+}
+#endif
 #endif

test/ct.cpp

@@ -2,7 +2,7 @@
 
 #include <gtest/gtest.h>
 #include <common/ct_check.h>
-#include <stdio.h>
+#include <common/utils.h>
 
 TEST(ConstantTime, CtGrind_Negative) {
     unsigned char a[16], b[16];
@@ -63,3 +63,53 @@ TEST(ConstantTime, CtGrind_Negative_UseSecretAsIndex) {
     ct_purify(&result, 1);
     ASSERT_EQ(result, 1);
 }
+
+TEST(ConstantTime, CtCheck_memcmp) {
+    unsigned char a[16], b[16];
+    memset(a, 42, sizeof(a));
+    memset(b, 42, sizeof(b));
+    uint8_t ret;
+
+    ct_poison(a, 16);
+    ret = ct_memcmp(a,b,16);
+    ct_expect_umr();
+    // Doesn't matter what we check. It's just to
+    // enusre UMR is triggered.
+    if (!ret) ASSERT_EQ(ret, 0);
+    ct_require_umr();
+    ct_purify(&ret, 1);
+
+    b[1] = 0;
+    ct_expect_umr();
+    ret = ct_memcmp(a,b,16);
+    if (ret) ASSERT_EQ(ret,1);
+    ct_require_umr();
+    ct_purify(&ret, 1);
+}
+
+TEST(ConstantTime, CtCheck_memcmp_chained) {
+    unsigned char a[16], b[16], c[16], d[16];
+    memset(a, 42, sizeof(a));
+    memset(b, 42, sizeof(b));
+    memset(d, 42, sizeof(b));
+    memset(c, 41, sizeof(c));
+    uint8_t ret;
+
+    ct_poison(a, 16);
+
+    ct_expect_umr();
+    // obviously must generate UMR if first check fails
+    // and second is not done
+    ret = (ct_memcmp(a,c,16)==0) && (ct_memcmp(a,b,16)==0);
+    ct_require_umr();
+    ct_purify(&ret, 1);
+    ASSERT_EQ(ret,0);
+
+    ct_expect_umr();
+    // it's still UMR even if both checks are OK
+    ret = (ct_memcmp(a,d,16)==0) && (ct_memcmp(a,b,16)==0);
+    ct_require_umr();
+
+    ct_purify(&ret, 1);
+    ASSERT_EQ(ret,1);
+}

test/ut.cpp

@@ -6,24 +6,24 @@
 
 TEST(KEM,OneOff) {
 
-	for (int i=0; i<PQC_ALG_KEM_MAX; i++) {
-		const pqc_ctx_t *p = pqc_kem_alg_by_id(i);
-
-	    std::vector<uint8_t> ct(pqc_ciphertext_bsz(p));
-	    std::vector<uint8_t> ss1(pqc_shared_secret_bsz(p));
-	    std::vector<uint8_t> ss2(pqc_shared_secret_bsz(p));
-	    std::vector<uint8_t> sk(pqc_private_key_bsz(p));
-	    std::vector<uint8_t> pk(pqc_public_key_bsz(p));
-
-		ASSERT_TRUE(
-			pqc_keygen(p, pk.data(), sk.data()));
-		ASSERT_TRUE(
-			pqc_kem_encapsulate(p, ct.data(), ss1.data(), pk.data()));
-		ASSERT_TRUE(
-			pqc_kem_decapsulate(p, ss2.data(), ct.data(), sk.data()));
-		ASSERT_TRUE(
-			std::equal(ss1.begin(), ss1.end(), ss2.begin()));
-	}
+    for (int i=0; i<PQC_ALG_KEM_MAX; i++) {
+        const pqc_ctx_t *p = pqc_kem_alg_by_id(i);
+
+        std::vector<uint8_t> ct(pqc_ciphertext_bsz(p));
+        std::vector<uint8_t> ss1(pqc_shared_secret_bsz(p));
+        std::vector<uint8_t> ss2(pqc_shared_secret_bsz(p));
+        std::vector<uint8_t> sk(pqc_private_key_bsz(p));
+        std::vector<uint8_t> pk(pqc_public_key_bsz(p));
+
+        ASSERT_TRUE(
+            pqc_keygen(p, pk.data(), sk.data()));
+        ASSERT_TRUE(
+            pqc_kem_encapsulate(p, ct.data(), ss1.data(), pk.data()));
+        ASSERT_TRUE(
+            pqc_kem_decapsulate(p, ss2.data(), ct.data(), sk.data()));
+        ASSERT_TRUE(
+            std::equal(ss1.begin(), ss1.end(), ss2.begin()));
+    }
 }
 
 TEST(SIGN,OneOff) {
@@ -32,21 +32,21 @@ TEST(SIGN,OneOff) {
     std::uniform_int_distribution<uint8_t> dist(0, 0xFF);
     uint8_t msg[1234] = {0};
 
-	for (int i=0; i<PQC_ALG_SIG_MAX; i++) {
-		const pqc_ctx_t *p = pqc_sig_alg_by_id(i);
-		// generate some random msg
-		for (auto &x : msg) {x = dist(rd);}
-
-	    std::vector<uint8_t> sig(pqc_signature_bsz(p));
-	    std::vector<uint8_t> sk(pqc_private_key_bsz(p));
-	    std::vector<uint8_t> pk(pqc_public_key_bsz(p));
-
-		ASSERT_TRUE(
-			pqc_keygen(p, pk.data(), sk.data()));
-		uint64_t sigsz = sig.size();
-		ASSERT_TRUE(
-			pqc_sig_create(p, sig.data(), &sigsz, msg, 1234, sk.data()));
-		ASSERT_TRUE(
-			pqc_sig_verify(p, sig.data(), sigsz, msg, 1234, pk.data()));
-	}
+    for (int i=0; i<PQC_ALG_SIG_MAX; i++) {
+        const pqc_ctx_t *p = pqc_sig_alg_by_id(i);
+        // generate some random msg
+        for (auto &x : msg) {x = dist(rd);}
+
+        std::vector<uint8_t> sig(pqc_signature_bsz(p));
+        std::vector<uint8_t> sk(pqc_private_key_bsz(p));
+        std::vector<uint8_t> pk(pqc_public_key_bsz(p));
+
+        ASSERT_TRUE(
+            pqc_keygen(p, pk.data(), sk.data()));
+        uint64_t sigsz = sig.size();
+        ASSERT_TRUE(
+            pqc_sig_create(p, sig.data(), &sigsz, msg, 1234, sk.data()));
+        ASSERT_TRUE(
+            pqc_sig_verify(p, sig.data(), sigsz, msg, 1234, pk.data()));
+    }
 }