Build Themis with sanitizers (cossacklabs#548)

* Support builds with sanitizers

Add a bunch of configuration options to enable various sanitizers
available for GCC and Clang:

  - WITH_ASAN - Address Sanitizer, mostly memory safety issues
  - WITH_MSAN - Memory Sanitizer, other types of memory safety
  - WITH_TSAN - Thread Sanitizer, mostly locks and threading
  - WITH_UBSAN - Undefined Behavior Sanitizer, various issues

The exact supported set of the sanitizers vary depending on the compiler
version so we check for them at startup and use only those which are
available. UBSan is especially different between GCC and Clang.

Obviously, these are opt-in options which are not enabled by default.
They are useful during development but should not affect production
builds.

There is also an option to make sanitizer warnings fatal which will be
useful for CI. Normally the developers do not need to enable it so that
they see all issues, but for CI we would like to abort the build if the
sanitizers report any issues.

* Run tests with sanitizers on CI

Teach CircleCI to execrise the test suite with all available sanitizers.
Unfortunately, some sanitizers are not available on all compilers, and
some produce a lot of unrelated errors so they are currently disabled.

* Blacklist some UBSan warnings

Unfortunately, there are quite a few places which trigger UBSan
(and rightly so). Currently we don't have time to review and fix
them properly, so let's disable the relevant warnings for now.

ed25519 implementation has been written by C gurus and has some
questionable numeric code. These warnings could be avoided by
refactoring the code to use correct types and casts, but it
requires careful attention. Leave it as is for now.

Other warnings are caused by misaligned accesses when handling
soter_container_t types. Everywhere in our codebase we cast
pointers to arbitrarily aligned bytes to C structs and technically
this is undefined behavior because on some processor architectures
it may cause CPU exceptions. This has been known for a long time,
but we did nothing about it. Silence these warnings for now, but
we'd better fix them at some point in the future so that we don't
have to maintain this exclusion blacklist.

And by the way, GCC does not support -fsanitize-blacklist so we
have disabled UBSan for GCC completely. The alternative is to
annotate affected functions with an attribute, but there are so
many of them that I don't want to litter history with it.

* Correctly signed size of Soter containers

The code expects the size field to contain unsigned size, but the
structure definition has signed size. We never use negative values
for anything, but expect the correct overflow behavior. One of the
tests actually triggers UBSan warning about this.

Switch the type to correct uint32_t to have the correct behavior.

This is technically public API, however no-one should be using
the structure field directly because it's encoded in big endian.
Switching to unsigned type also does not change the ABI (on the
platforms that we care about), so all in all this is a safe fix.

* Initialize CRC safely

UBSan gives us another warning about using "~0L" expression to get
"all 1-bits" initialization value. Technically, this is undefined
behavior because you cannot bit-flip a signed value and you cannot
cast a negative signed value into an unsigned one.

Just use a different equivalent initializer which expresses the same
idea and keeps UBSan happy.

.circleci/config.yml

@@ -29,13 +29,26 @@ jobs:
       CXX: clang++-8
       CLANG_FORMAT: clang-format-8
       CLANG_TIDY: clang-tidy-8
+      NO_NIST_STS: 1
       WITH_FATAL_WARNINGS: yes
+      WITH_FATAL_SANITIZERS: yes
     steps:
       - run: sudo apt-get update && sudo DEBIAN_FRONTEND=noninteractive apt-get -y install default-jdk nodejs npm
       - checkout
       - run: git reset HEAD && git submodule sync && git submodule update --init
       - run: make fmt_check ENGINE=boringssl
       - run: make fmt_check ENGINE=openssl
+      - run: make clean && make test CC=gcc-8   WITH_ASAN=1
+      - run: make clean && make test CC=gcc-8   WITH_TSAN=1
+      # Currently the code has quite a few UBSAN violations and GCC does not provide a convenient
+      # way to silence individual warnings. Disable UBSAN runs for GCC for now.
+#     - run: make clean && make test CC=gcc-8   WITH_UBSAN=1
+      - run: make clean && make test CC=clang-8 WITH_ASAN=1
+      # MSAN is currently supported only by Clang. However, it produces a lot of false positives
+      # due to OpenSSL not being instrumented and intentionally using uninitialized memory.
+#     - run: make clean && make test CC=clang-8 WITH_MSAN=1
+      - run: make clean && make test CC=clang-8 WITH_TSAN=1
+      - run: make clean && make test CC=clang-8 WITH_UBSAN=1
 
   x86_64:
     docker:

Makefile

@@ -229,12 +229,22 @@ endif
 # our compilation flags there, so do not export these variables.
 unexport CFLAGS LDFLAGS
 
+# Prevent undefined symbols in produced binaries, but allow them for sanitizers
+# which expect the libraries linked into the main executable to be underlinked.
+ifndef WITH_ASAN
+ifndef WITH_MSAN
+ifndef WITH_TSAN
+ifndef WITH_UBSAN
 ifdef IS_MACOS
 LDFLAGS += -Wl,-undefined,error
 endif
 ifdef IS_LINUX
 LDFLAGS += -Wl,--no-undefined
 endif
+endif
+endif
+endif
+endif
 
 CFLAGS += -O2 -fno-omit-frame-pointer -g
 
@@ -299,6 +309,64 @@ endif
 
 CFLAGS += -fvisibility=hidden
 
+#
+# Enable code sanitizers on demand and if supported by compiler
+#
+
+ifdef WITH_ASAN
+ifeq (yes,$(call supported,-fsanitize=address))
+SANITIZERS += -fsanitize=address
+else
+$(error -fsanitize=address requested but $(CC) does not seem to support it)
+endif
+endif
+
+ifdef WITH_MSAN
+ifeq (yes,$(call supported,-fsanitize=memory))
+SANITIZERS += -fsanitize=memory -fsanitize-memory-track-origins=2
+else
+$(error -fsanitize=memory requested but $(CC) does not seem to support it)
+endif
+endif
+
+ifdef WITH_TSAN
+ifeq (yes,$(call supported,-fsanitize=thread))
+SANITIZERS += -fsanitize=thread
+else
+$(error -fsanitize=thread requested but $(CC) does not seem to support it)
+endif
+endif
+
+ifdef WITH_UBSAN
+ifeq (yes,$(call supported,-fsanitize=undefined))
+SANITIZERS += -fsanitize=undefined
+else
+$(error -fsanitize=undefined requested but $(CC) does not seem to support it)
+endif
+ifeq (yes,$(call supported,-fsanitize=integer))
+SANITIZERS += -fsanitize=integer
+else
+$(warning -fsanitize=integer not supported by $(CC), skipping...)
+endif
+ifeq (yes,$(call supported,-fsanitize=nullability))
+SANITIZERS += -fsanitize=nullability
+else
+$(warning -fsanitize=nullability not supported by $(CC), skipping...)
+endif
+ifeq (yes,$(call supported,-fsanitize-blacklist=src/soter/blacklist-ubsan.txt))
+SANITIZERS += -fsanitize-blacklist=src/soter/blacklist-ubsan.txt
+else
+$(warning -fsanitize-blacklist not supported by $(CC), skipping...)
+endif
+endif
+
+ifeq (yes,$(WITH_FATAL_SANITIZERS))
+SANITIZERS += -fno-sanitize-recover=all
+endif
+
+CFLAGS  += $(SANITIZERS)
+LDFLAGS += $(SANITIZERS)
+
 # Binary format compatibility with Themis 0.9.6 on x86_64 architecture.
 # https://github.com/cossacklabs/themis/pull/279
 ifeq ($(NO_SCELL_COMPAT),)

src/soter/blacklist-ubsan.txt

@@ -0,0 +1,26 @@
+# Disable ubsan warnings about integer handling in third-party ed25519 library
+[integer]
+src:src/soter/ed25519/*
+
+# FIXME: avoid misaligned memory accesses instead of silencing warnings here
+# Disable warnings for `soter_container_hdr_t*` casts
+[alignment]
+src:src/soter/boringssl/soter_asym_cipher.c
+src:src/soter/boringssl/soter_asym_ka.c
+src:src/soter/boringssl/soter_ec_key.c
+src:src/soter/boringssl/soter_ecdsa_common.c
+src:src/soter/boringssl/soter_rsa_common.c
+src:src/soter/boringssl/soter_rsa_key.c
+src:src/soter/openssl/soter_asym_cipher.c
+src:src/soter/openssl/soter_asym_ka.c
+src:src/soter/openssl/soter_ec_key.c
+src:src/soter/openssl/soter_ecdsa_common.c
+src:src/soter/openssl/soter_rsa_common.c
+src:src/soter/openssl/soter_rsa_key.c
+src:src/soter/soter_container.c
+src:src/themis/secure_keygen.c
+src:src/themis/secure_message_wrapper.c
+src:src/themis/secure_session.c
+src:src/themis/secure_session_message.c
+src:src/themis/secure_session_serialize.c
+src:src/themis/secure_session_utils.c

src/soter/soter_container.h

@@ -24,8 +24,8 @@
 
 struct soter_container_hdr_type {
     char tag[SOTER_CONTAINER_TAG_LENGTH];
-    int32_t size; /* Size is data + sizeof(soter_container_hdr_t), so should be not less than
-                     sizeof(soter_container_hdr_t). Network byte order. */
+    uint32_t size; /* Size is data + sizeof(soter_container_hdr_t), so should be not less than
+                      sizeof(soter_container_hdr_t). Network byte order. */
     uint32_t crc;
 };
 

src/soter/soter_crc32.c

@@ -63,7 +63,7 @@ static uint32_t crc_c[256] = {
 
 soter_crc32_t soter_crc32_create(void)
 {
-    return ~0L;
+    return 0xFFFFFFFF;
 }
 
 void soter_crc32_update(soter_crc32_t* crc, const void* buf, size_t len)