From 997b7111cab3e6a6bee9a3c5b44db6c99878d359 Mon Sep 17 00:00:00 2001 From: Kris Kwiatkowski Date: Sat, 10 Jul 2021 00:33:09 +0100 Subject: [PATCH] backport some changes from ct study --- CMakeLists.txt | 48 ++++++++++++++--------------- src/common/ct_check.h | 55 ++++++++++++++++++++++++++++++---- src/common/utils.c | 13 ++++++++ src/common/utils.h | 25 +++++++++++++++- test/ct.cpp | 52 +++++++++++++++++++++++++++++++- test/ut.cpp | 70 +++++++++++++++++++++---------------------- 6 files changed, 197 insertions(+), 66 deletions(-) create mode 100644 src/common/utils.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 56cfeb37..b9b3def1 100644 --- a/CMakeLists.txt +++ b/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. diff --git a/src/common/ct_check.h b/src/common/ct_check.h index 90569431..7bdc4b7a 100644 --- a/src/common/ct_check.h +++ b/src/common/ct_check.h @@ -1,22 +1,30 @@ #ifndef CT_CHECK_H #define CT_CHECK_H +#include + // 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 #include +#endif #elif defined(PQC_USE_CTGRIND) +#include +#include #include #include #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 +#include + +// 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; +} diff --git a/src/common/utils.h b/src/common/utils.h index 45142dab..11111ef0 100644 --- a/src/common/utils.h +++ b/src/common/utils.h @@ -1,8 +1,14 @@ #ifndef PQC_COMMON_UTILS_ #define PQC_COMMON_UTILS_ +#include +#include #include +#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 diff --git a/test/ct.cpp b/test/ct.cpp index 6209215e..e30746f6 100644 --- a/test/ct.cpp +++ b/test/ct.cpp @@ -2,7 +2,7 @@ #include #include -#include +#include 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); +} diff --git a/test/ut.cpp b/test/ut.cpp index 8ea7a2c8..701c11bf 100644 --- a/test/ut.cpp +++ b/test/ut.cpp @@ -6,24 +6,24 @@ TEST(KEM,OneOff) { - for (int i=0; i ct(pqc_ciphertext_bsz(p)); - std::vector ss1(pqc_shared_secret_bsz(p)); - std::vector ss2(pqc_shared_secret_bsz(p)); - std::vector sk(pqc_private_key_bsz(p)); - std::vector 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 ct(pqc_ciphertext_bsz(p)); + std::vector ss1(pqc_shared_secret_bsz(p)); + std::vector ss2(pqc_shared_secret_bsz(p)); + std::vector sk(pqc_private_key_bsz(p)); + std::vector 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 dist(0, 0xFF); uint8_t msg[1234] = {0}; - for (int i=0; i sig(pqc_signature_bsz(p)); - std::vector sk(pqc_private_key_bsz(p)); - std::vector 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 sig(pqc_signature_bsz(p)); + std::vector sk(pqc_private_key_bsz(p)); + std::vector 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())); + } }