Repository: cossacklabs/themis (language: rust)

Issue:
Reusing Secure Comparator crashes with BoringSSL
### Issue description

Trying to reuse a Secure Comparator after performing a comparison causes a segmentation fault in BoringSSL library.

### Environment info

**OS:** Linux (macOS is affected as well)

**Themis version:** 0.10.0

**Crypto provider:** BoringSSL (google/boringssl@0a54e998481b0b5a8abd9717c5f7301a3b18b628)

### Details

Steps to reproduce:

1. Build BoringSSL.
2. Build Themis with that BoringSSL.
3. Build [this sample code](https://gist.github.com/ilammy/eff9ac4ee48c435d01c1b98d5233901e).
4. Run it.

(There's a script in that gist which does all of this.)

**Actual result:**

```console
$ ./a.out
[=] appending data to A: different
[=] appending data to B: test data
[-] begin   comparison: A
[~] proceed comparison: B
[~] proceed comparison: A
[~] proceed comparison: B
[~] proceed comparison: A
[!] comparison complete
[>] result A: (---) mismatch
[>] result B: (---) mismatch
[=] appending data to A: the same data
Segmentation fault (core dumped)
```

**Expected result:**

Definitely not a crash.

I'm not sure what is the expected behavior for Secure Comparator if it is reused. After some thought I have came up with at least three perfectly reasonable ways for it to behave:

- complete both comparison successfully, return "no match" for both
- complete both comparison successfully, return "no match" for the first comparison, but "match" the second one
- deny reusing Secure Comparators, fail with an error when trying to append data after the comparison has been completed

### Notes

The issue is not actual for OpenSSL and LibreSSL. In this case the sample returns "no match" for both comparisons.

The crash seems to be caused by misuse of EVP_MD by Themis. [OpenSSL documentation says](https://www.openssl.org/docs/man1.1.1/man3/EVP_DigestFinal_ex.html):

> After calling EVP_DigestFinal_ex() no additional calls to  can be made, but EVP_DigestInit_ex() can be called to initialize a new digest operation.

The erroneous call sequence:

```
secure_comparator_proceed_compare (of Alice-the-client)
  secure_comparator_alice_step1
    soter_hash_final(&comp_ctx->hash_ctx)
      EVP_DigestFinal_ex

secure_comparator_proceed_compare (of Bob-the-server)
  secure_comparator_bob_step2
    soter_hash_final(&comp_ctx->hash_ctx)
      EVP_DigestFinal_ex

secure_comparator_append_secret
  soter_hash_update(&comp_ctx->hash_ctx)
    EVP_DigestUpdate
```

BoringSSL seems to free the digest computation state after the digest value is retrieved:

```
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff75456ea in EVP_DigestUpdate (ctx=0x603500, data=0x400f5c, len=13)
    at themis/third_party/boringssl/src/crypto/fipsmodule/digest/digest.c:200
200	  ctx->digest->update(ctx, data, len);
(gdb) p *ctx
$1 = {digest = 0x0, md_data = 0x0, pctx = 0x0, pctx_ops = 0x0}
(gdb) where
#0  0x00007ffff75456ea in EVP_DigestUpdate (ctx=0x603500, data=0x400f5c, len=13)
    at themis/third_party/boringssl/src/crypto/fipsmodule/digest/digest.c:200
#1  0x00007ffff752045a in soter_hash_update ()
   from themis/install/lib/libsoter.so
#2  0x00007ffff7bcfa6b in secure_comparator_append_secret ()
   from themis/install/lib/libthemis.so
#3  0x0000000000400a0d in append_secret ()
#4  0x0000000000400cfb in main ()
```

So it really can't handle calling EVP_DigestUpdate() after EVP_DigestFinal_ex().


Discussion / hints:
Many crypto implementations have "finalisable" objects. Maybe, to solve this problem long-term we should add another error code, like `THEMIS_OBJECT_FINALIZED` or something - to give clear indication to the caller, that their program flow is somehow incorrect. Thoughts?

Another option specifically for SC might be: reinit all the internal structures to actually allow the reuse from a clean state

Fix patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
index e20be6c7e..fdeb5e2fd 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -52,6 +52,7 @@ jobs:
             - ~/valgrind
 
       - checkout
+      - run: git submodule update --init
       - run: make
       - run: make JAVA_HOME=/usr/lib/jvm/default-java themis_jni
       - run: sudo make install
@@ -59,19 +60,7 @@ jobs:
       - run: sudo make pythemis_install
       - run: sudo make rubythemis_install
       - run: sudo make jsthemis_install
-
-      - restore_cache:
-          keys:
-            - boringssl-chromium-stable
-
-      - run: if [ ! -d $HOME/boringssl ]; then cd $HOME && git clone https://boringssl.googlesource.com/boringssl && cd boringssl && git checkout chromium-stable && mkdir build && cd build && cmake .. && make && cp decrepit/libdecrepit.a crypto/; fi
-
-      - save_cache:
-          key: boringssl-chromium-stable
-          paths:
-            - ~/boringssl
-
-      - run: make ENGINE=boringssl ENGINE_INCLUDE_PATH=$HOME/boringssl/include ENGINE_LIB_PATH=$HOME/boringssl/build/crypto BUILD_PATH=build_with_boringssl prepare_tests_basic
+      - run: make ENGINE=boringssl BUILD_PATH=build_with_boringssl prepare_tests_basic
       - run: make BUILD_PATH=cover_build COVERAGE=y prepare_tests_basic
       - run: make prepare_tests_all
       - run: mkdir -p $HOME/go/src/$GOTHEMIS_IMPORT
diff --git a/Makefile b/Makefile
index c88d289ba..5cbad3732 100644
--- a/Makefile
+++ b/Makefile
@@ -258,7 +258,7 @@ soter_static: $(SOTER_OBJ)
 
 soter_shared: CMD = $(CC) -shared -o $(BIN_PATH)/lib$(SOTER_BIN).$(SHARED_EXT) $(SOTER_OBJ) $(LDFLAGS) $(COVERLDFLAGS)
 
-soter_shared: $(SOTER_OBJ)
+soter_shared: $(SOTER_OBJ) $(SOTER_ENGINE_DEPS)
 	@echo -n "link "
 	@$(BUILD_CMD)
 ifdef IS_MACOS
diff --git a/src/soter/boringssl/soter.mk b/src/soter/boringssl/soter.mk
index 04c0a842d..d8d04069a 100644
--- a/src/soter/boringssl/soter.mk
+++ b/src/soter/boringssl/soter.mk
@@ -19,13 +19,24 @@ SOTER_SRC += $(wildcard $(CRYPTO_ENGINE)/*.c)
 # Put path to your OpenSSL/LibreSSL here
 OPENSSL_DIR = libs/librebin
 
-ifneq ($(CRYPTO_ENGINE_INCLUDE_PATH),)
+ifeq ($(CRYPTO_ENGINE_INCLUDE_PATH),)
+	CRYPTO_ENGINE_CFLAGS += -Ithird_party/boringssl/src/include
+else
 	CRYPTO_ENGINE_CFLAGS += -I$(CRYPTO_ENGINE_INCLUDE_PATH)
 endif
-ifneq ($(CRYPTO_ENGINE_LIB_PATH),)
+ifeq ($(CRYPTO_ENGINE_LIB_PATH),)
+	CRYPTO_ENGINE_LDFLAGS += -L$(BIN_PATH)/boringssl/crypto -L$(BIN_PATH)/boringssl/decrepit
+	SOTER_ENGINE_DEPS += $(BIN_PATH)/boringssl/crypto/libcrypto.a $(BIN_PATH)/boringssl/decrepit/libdecrepit.a
+else
 	CRYPTO_ENGINE_LDFLAGS += -L$(CRYPTO_ENGINE_LIB_PATH)
 endif
 CRYPTO_ENGINE_LDFLAGS += -lcrypto -ldecrepit -lpthread
 
 CFLAGS += $(CRYPTO_ENGINE_CFLAGS)
 LDFLAGS += $(CRYPTO_ENGINE_LDFLAGS)
+
+$(BIN_PATH)/boringssl/crypto/libcrypto.a $(BIN_PATH)/boringssl/decrepit/libdecrepit.a:
+	@echo "building embedded BoringSSL..."
+	@mkdir -p $(BIN_PATH)/boringssl
+	@cd $(BIN_PATH)/boringssl && cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS="-fpic" ../../third_party/boringssl/src
+	@$(MAKE) -C $(BIN_PATH)/boringssl
\ No newline at end of file
diff --git a/src/soter/boringssl/soter_hash.c b/src/soter/boringssl/soter_hash.c
index 757d282cb..4529796fd 100644
--- a/src/soter/boringssl/soter_hash.c
+++ b/src/soter/boringssl/soter_hash.c
@@ -58,6 +58,11 @@ soter_status_t soter_hash_update(soter_hash_ctx_t *hash_ctx, const void *data, s
 		return SOTER_INVALID_PARAMETER;
 	}
 
+	if (!EVP_MD_CTX_md(&(hash_ctx->evp_md_ctx)))
+	{
+		return SOTER_INVALID_PARAMETER;
+	}
+
 	if (EVP_DigestUpdate(&(hash_ctx->evp_md_ctx), data, length))
 	{
 		return SOTER_SUCCESS;
@@ -77,6 +82,11 @@ soter_status_t soter_hash_final(soter_hash_ctx_t *hash_ctx, uint8_t* hash_value,
 		return SOTER_INVALID_PARAMETER;
 	}
 
+	if (!EVP_MD_CTX_md(&(hash_ctx->evp_md_ctx)))
+	{
+		return SOTER_INVALID_PARAMETER;
+	}
+
 	md_length = (size_t)EVP_MD_CTX_size(&(hash_ctx->evp_md_ctx));
 
 	if (!hash_value || (md_length > *hash_length))
diff --git a/src/soter/openssl/soter_hash.c b/src/soter/openssl/soter_hash.c
index 453bce93d..b4d12f4d1 100644
--- a/src/soter/openssl/soter_hash.c
+++ b/src/soter/openssl/soter_hash.c
@@ -64,6 +64,11 @@ soter_status_t soter_hash_update(soter_hash_ctx_t *hash_ctx, const void *data, s
 		return SOTER_INVALID_PARAMETER;
 	}
 
+	if (!EVP_MD_CTX_md(hash_ctx->evp_md_ctx))
+	{
+		return SOTER_INVALID_PARAMETER;
+	}
+
 	if (EVP_DigestUpdate(hash_ctx->evp_md_ctx, data, length))
 	{
 		return SOTER_SUCCESS;
@@ -83,6 +88,11 @@ soter_status_t soter_hash_final(soter_hash_ctx_t *hash_ctx, uint8_t* hash_value,
 		return SOTER_INVALID_PARAMETER;
 	}
 
+	if (!EVP_MD_CTX_md(hash_ctx->evp_md_ctx))
+	{
+		return SOTER_INVALID_PARAMETER;
+	}
+
 	md_length = (size_t)EVP_MD_CTX_size(hash_ctx->evp_md_ctx);
 
 	if (!hash_value || (md_length > *hash_length))
@@ -92,7 +102,7 @@ soter_status_t soter_hash_final(soter_hash_ctx_t *hash_ctx, uint8_t* hash_value,
 		return SOTER_BUFFER_TOO_SMALL;
 	}
 
-	if (EVP_DigestFinal_ex(hash_ctx->evp_md_ctx, hash_value, (unsigned int *)&md_length))
+	if (EVP_DigestFinal(hash_ctx->evp_md_ctx, hash_value, (unsigned int *)&md_length))
 	{
 		*hash_length = md_length;
 		return SOTER_SUCCESS;
diff --git a/third_party/boringssl/src b/third_party/boringssl/src
index 0a54e9984..74944287e 160000
--- a/third_party/boringssl/src
+++ b/third_party/boringssl/src
@@ -1,1 +1,1 @@
-Subproject commit 0a54e998481b0b5a8abd9717c5f7301a3b18b628
+Subproject commit 74944287e1ab95e091107c9f26ce46bd50e13043


Test patch:
diff --git a/tests/soter/soter_hash_test.c b/tests/soter/soter_hash_test.c
index 88738ef6c..ba5106a29 100644
--- a/tests/soter/soter_hash_test.c
+++ b/tests/soter/soter_hash_test.c
@@ -159,7 +159,7 @@ static void test_api_(soter_hash_ctx_t* ctx)
 
 	res = soter_hash_final(ctx, hash, &hash_len);
 	testsuite_fail_unless((SOTER_SUCCESS == res) && (32 == hash_len) && !memcmp(hash, result, hash_len), "soter_hash_final: normal value");
-
+	testsuite_fail_unless(SOTER_INVALID_PARAMETER == soter_hash_update(ctx, input, input_len), "soter_hash_update: use after final");
 	testsuite_fail_unless(SOTER_INVALID_PARAMETER == soter_hash_destroy(NULL), "soter_hash_destroy: invalid context");
     testsuite_fail_unless(SOTER_SUCCESS == soter_hash_cleanup(ctx), "soter_hash_cleanup: can't cleanup");
 }
diff --git a/tests/test.mk b/tests/test.mk
index 0f4623d15..26d2a8c14 100644
--- a/tests/test.mk
+++ b/tests/test.mk
@@ -42,7 +42,7 @@ nist_rng_test_suite_clean:
 
 soter_test: CMD = $(CC) -o $(TEST_BIN_PATH)/soter_test $(SOTER_TEST_OBJ) $(COMMON_TEST_OBJ) -L$(BIN_PATH) -lsoter $(LDFLAGS) $(COVERLDFLAGS)
 
-soter_test: nist_rng_test_suite soter_static $(SOTER_TEST_OBJ) $(COMMON_TEST_OBJ)
+soter_test: nist_rng_test_suite soter_static $(SOTER_ENGINE_DEPS) $(SOTER_TEST_OBJ) $(COMMON_TEST_OBJ)
 	@echo -n "link "
 	@$(BUILD_CMD)