crypto: support openssl crypto backend by billatarm · Pull Request #354 · Aorimn/dislocker

billatarm · 2026-03-04T19:26:39Z

Support configurable cryptographic backends.

To do this, we have to address a few limitations:

The current ssl_bindings.h requires the inclusion of mbedtls headers to figure out how to build. This is problematic if one wants to support additional crypto backends, whether in tree or out of tree.
Detect which backend is being requested, at configure time, and then use the bindings specified. Move everything into an include directory to make backend switching as easy as pointing it to a new backend directory for the proper ssl_bindings.h. Keep mbedtls as the default so nothing changes.

This can be configured by cmake .. -DCRYPTO_BACKEND=mbedtls

New backends can be added now by simply creating a directory and updating the CMake files to point to a new directory.

Additionally, add some unit tests around the crypto routines, so new backend authors get some basic test coverage.

Support configurable cryptographic backends. To do this, we have to address a few limitations: 1. The current ssl_bindings.h requires the inclusion of mbedtls headers to figure out how to build. This is problematic if one wants to support additional crypto backends, whether in tree or out of tree. 2. Detect which backend is being requested, at configure time, and then use the bindings specified. Move everything into an include directory to make backend switching as easy as pointing it to a new backend directory for the proper ssl_bindings.h. Keep mbedtls as the default so nothing changes. This can be configured by cmake .. -DCRYPTO_BACKEND=mbedtls New backends can be added now by simply creating a directory and updating the CMake files to point to a new directory. Additionally, add some unit tests around the crypto routines, so new backend authors get some basic test coverage. Signed-off-by: Bill Roberts <bill.roberts@arm.com>

williamcroberts · 2026-03-24T18:41:14Z

@Aorimn could you take a look at this?

abhinavagarwal07

Nice work on the pluggable backend design — the include-path switching approach is clean and the tests are a welcome addition. A few bugs and one performance concern to sort out before this is ready to merge.

abhinavagarwal07 · 2026-04-15T07:37:36Z

+
+target_link_libraries(crypto_tests PRIVATE ${PROJECT_NAME} dislocker_crypto_backend)
+
+add_test(NAME cyrpto_tests COMMAND crypto_tests)


Typo: cyrpto_tests → crypto_tests

abhinavagarwal07 · 2026-04-15T07:37:36Z

+
+	AES_SETDEC_KEY(&ctx, key_aes_256, sizeof(key_aes_256) * 8);
+
+	AES_CBC(&ctx, AES_DECRYPT, sizeof(expected_cbc_128), iv, expected_cbc_256, buf);


This should be sizeof(expected_cbc_256). Both arrays happen to be 512 bytes so it passes today, but it'll silently break if the vectors ever diverge.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+	/* mbedtls requires a mutable IV */
+	NEW_ARRAY_FROM(char, iv, static_iv);
+
+	/* Test AES CBC 128 Encryption */


Comments here and on line 121 say "CBC 128" but this function tests CBC 256.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+	if (key_len > sizeof(ctx->key))
+		return 1;
+
+	memcpy(ctx->key, key, key_len);


memcpy is used here but this header doesn't include <string.h>. It happens to work because callers include it first, but the header should be self-contained.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+#define DIS_OSSL_CIPHER_ECB 0
+#define DIS_OSSL_CIPHER_CBC 1
+
+static inline const EVP_CIPHER *dis_ossle_get_cipher(size_t key_len, int ossl_mode)


Typo: dis_ossle_get_cipher → dis_ossl_get_cipher (stray e)

abhinavagarwal07 · 2026-04-15T07:37:37Z

+	if (!ossl_cipher)
+		return rc;
+
+	EVP_CIPHER_CTX *ossl_ctx = EVP_CIPHER_CTX_new();


This allocates and tears down an EVP context on every call. AES_ECB_ENC is called per 16-byte block from the XTS/XEX inner loops, so for a 512-byte sector you're doing ~33 malloc/init/free cycles. Over a full disk decryption that's tens of millions of unnecessary allocations — going to be dramatically slower than the mbedtls path.

Worth either caching the EVP_CIPHER_CTX in the dis_ossl_aes_ctx struct or initializing it once at set-key time.

This allocates and tears down an EVP context on every call. AES_ECB_ENC is called per 16-byte block from the XTS/XEX inner loops, so for a 512-byte sector you're doing ~33 malloc/init/free cycles. Over a full disk decryption that's tens of millions of unnecessary allocations — going to be dramatically slower than the mbedtls path.

Probably not as much as one thinks, since alloc and free are fairly cheap unless the heap needs to grow. I didn't notice a decryption speed difference. But I didn't like this approach either because of what you state.

Worth either caching the EVP_CIPHER_CTX in the dis_ossl_aes_ctx struct or initializing it once at set-key time.

I think it would be cleaner if I just refactored the crypto interface, where we have an init, use, free pattern by the underlying crypto routines.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+#define AES_CBC(ctx, mode, size, iv, in, out) dis_ossl_aes_crypt(ctx, mode, size, iv, in, out, DIS_OSSL_CIPHER_CBC)
+
+/*
+ * OpenSSL doesn't provide XEX nor XTS, so just use the dislocker implementations. Note that


This isn't quite right — OpenSSL does provide XTS (EVP_aes_128_xts() / EVP_aes_256_xts()). It doesn't have XEX, which is correct. Using OpenSSL's native XTS would also get you hardware acceleration (AES-NI) for free.

Ahh I see that it has XTS, I didn't see that before when I looked.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+    ${CMAKE_SOURCE_DIR}/include/${PROJECT_NAME}/encryption/mbedtls
+  )
+elseif(CRYPTO_BACKEND STREQUAL "openssl")
+  find_package(OpenSSL REQUIRED)


The mbedtls path specifies a minimum version (find_package(MbedTLS 3 REQUIRED)). Should probably do the same here — something like find_package(OpenSSL 1.1.0 REQUIRED) given the EVP API usage.

abhinavagarwal07 · 2026-04-15T07:37:37Z

+	ADD_TEST(test_ecb_encrypt_256);
+
+	ADD_TEST(test_cbc_encrypt_128);
+	ADD_TEST(test_cbc_encrypt_256);


Any plans to add XTS/XEX test coverage? Those are the modes BitLocker actually uses and the paths most likely to diverge between backends.

I can add those

abhinavagarwal07 · 2026-04-15T07:37:37Z

+#ifndef TEST_VECTORS_H_
+#define TEST_VECTORS_H_
+
+static const char key_aes_128[] = {


These vectors are declared as char but the AES functions expect unsigned char *. The project builds with -Wall -Wextra which will flag -Wpointer-sign mismatches. Should be static const unsigned char throughout.

billatarm mentioned this pull request Mar 4, 2026

using openssl API for xts decoding #110

Open

abhinavagarwal07 suggested changes Apr 15, 2026

View reviewed changes


		target_link_libraries(crypto_tests PRIVATE ${PROJECT_NAME} dislocker_crypto_backend)

		add_test(NAME cyrpto_tests COMMAND crypto_tests)


		AES_SETDEC_KEY(&ctx, key_aes_256, sizeof(key_aes_256) * 8);

		AES_CBC(&ctx, AES_DECRYPT, sizeof(expected_cbc_128), iv, expected_cbc_256, buf);

Conversation

billatarm commented Mar 4, 2026

Uh oh!

williamcroberts commented Mar 24, 2026

Uh oh!

abhinavagarwal07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants