CVE-2020-11684: Improper Removal of Sensitive Information Before Storage or Transfer (CWE-212)
value: AT91bootstrap before 3.9.2 does not properly wipe encryption and authentication keys from memory before passing control to a less privileged software component. This can be exploited to disclose these keys and subsequently encrypt and sign the next boot stage (such as the bootloader).
CWE description: The product stores, transfers, or shares a resource that contains sensitive information, but it does not properly remove that information before the product makes the resource available to unauthorized actors.
CVSS v3 base score: 9.1
Published: 2020-09-14T14:15Z
Repository: https://github.com/linux4sam/at91bootstrap
Language: C
Files changed: driver/secure.c, include/secure.h
Commit date: 2020-03-30 15:47:48 +0300
Commit message: driver: secure: move keys into static arrays

Move the keys into static arrays. This will avoid copying the keys
from the code section to the stack on the previous init_keys implementation.
Like this, the keys are hardcoded into data section at compile time,
and can be completely wiped after use.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Diff: diff --git a/driver/secure.c b/driver/secure.c
index ffb38e0e..ea4c5e9f 100644
--- a/driver/secure.c
+++ b/driver/secure.c
@@ -30,67 +30,64 @@
 #include "aes.h"
 #include "debug.h"
 #include "string.h"
 #include "autoconf.h"
 
-
-static inline void init_keys(at91_aes_key_size_t *key_size,
-			     unsigned int *cipher_key,
-			     unsigned int *cmac_key,
-			     unsigned int *iv)
-{
-#if defined(CONFIG_AES_KEY_SIZE_128)
-	*key_size = AT91_AES_KEY_SIZE_128;
-#elif defined(CONFIG_AES_KEY_SIZE_192)
-	*key_size = AT91_AES_KEY_SIZE_192;
-#elif defined(CONFIG_AES_KEY_SIZE_256)
-	*key_size = AT91_AES_KEY_SIZE_256;
-#else
-#error "bad AES key size"
+static unsigned int cipher_key[8] = {
+	CONFIG_AES_CIPHER_KEY_WORD0,
+	CONFIG_AES_CIPHER_KEY_WORD1,
+	CONFIG_AES_CIPHER_KEY_WORD2,
+	CONFIG_AES_CIPHER_KEY_WORD3,
+#if defined(CONFIG_AES_KEY_SIZE_192) || defined(CONFIG_AES_KEY_SIZE_256)
+	CONFIG_AES_CIPHER_KEY_WORD4,
+	CONFIG_AES_CIPHER_KEY_WORD5,
 #endif
+#if defined(CONFIG_AES_KEY_SIZE_256)
+	CONFIG_AES_CIPHER_KEY_WORD6,
+	CONFIG_AES_CIPHER_KEY_WORD7,
+#endif
+};
 
-	iv[0]		= CONFIG_AES_IV_WORD0;
-	iv[1]		= CONFIG_AES_IV_WORD1;
-	iv[2]		= CONFIG_AES_IV_WORD2;
-	iv[3]		= CONFIG_AES_IV_WORD3;
-
-	cipher_key[0]	= CONFIG_AES_CIPHER_KEY_WORD0;
-	cmac_key[0]	= CONFIG_AES_CMAC_KEY_WORD0;
-	cipher_key[1]	= CONFIG_AES_CIPHER_KEY_WORD1;
-	cmac_key[1]	= CONFIG_AES_CMAC_KEY_WORD1;
-	cipher_key[2]	= CONFIG_AES_CIPHER_KEY_WORD2;
-	cmac_key[2]	= CONFIG_AES_CMAC_KEY_WORD2;
-	cipher_key[3]	= CONFIG_AES_CIPHER_KEY_WORD3;
-	cmac_key[3]	= CONFIG_AES_CMAC_KEY_WORD3;
-
+static unsigned int cmac_key[8] = {
+	CONFIG_AES_CMAC_KEY_WORD0,
+	CONFIG_AES_CMAC_KEY_WORD1,
+	CONFIG_AES_CMAC_KEY_WORD2,
+	CONFIG_AES_CMAC_KEY_WORD3,
 #if defined(CONFIG_AES_KEY_SIZE_192) || defined(CONFIG_AES_KEY_SIZE_256)
-	cipher_key[4]	= CONFIG_AES_CIPHER_KEY_WORD4;
-	cmac_key[4]	= CONFIG_AES_CMAC_KEY_WORD4;
-	cipher_key[5]	= CONFIG_AES_CIPHER_KEY_WORD5;
-	cmac_key[5]	= CONFIG_AES_CMAC_KEY_WORD5;
+	CONFIG_AES_CMAC_KEY_WORD4,
+	CONFIG_AES_CMAC_KEY_WORD5,
 #endif
-
 #if defined(CONFIG_AES_KEY_SIZE_256)
-	cipher_key[6]	= CONFIG_AES_CIPHER_KEY_WORD6;
-	cmac_key[6]	= CONFIG_AES_CMAC_KEY_WORD6;
-	cipher_key[7]	= CONFIG_AES_CIPHER_KEY_WORD7;
-	cmac_key[7]	= CONFIG_AES_CMAC_KEY_WORD7;
+	CONFIG_AES_CMAC_KEY_WORD6,
+	CONFIG_AES_CMAC_KEY_WORD7,
 #endif
-}
+};
 
-int secure_decrypt(void *data, unsigned int data_length, int is_signed)
+static unsigned int iv[AT91_AES_IV_SIZE_WORD] = {
+	CONFIG_AES_IV_WORD0,
+	CONFIG_AES_IV_WORD1,
+	CONFIG_AES_IV_WORD2,
+	CONFIG_AES_IV_WORD3,
+};
+
+static int secure_decrypt(void *data, unsigned int data_length, int is_signed)
 {
 	at91_aes_key_size_t key_size;
-	unsigned int cmac_key[8], cipher_key[8];
-	unsigned int iv[AT91_AES_IV_SIZE_WORD];
 	unsigned int computed_cmac[AT91_AES_BLOCK_SIZE_WORD];
 	unsigned int fixed_length;
 	const unsigned int *cmac;
 	int rc = -1;
 
-	/* Init keys */
-	init_keys(&key_size, cipher_key, cmac_key, iv);
+#if defined(CONFIG_AES_KEY_SIZE_128)
+	key_size = AT91_AES_KEY_SIZE_128;
+#elif defined(CONFIG_AES_KEY_SIZE_192)
+	key_size = AT91_AES_KEY_SIZE_192;
+#elif defined(CONFIG_AES_KEY_SIZE_256)
+	key_size = AT91_AES_KEY_SIZE_256;
+#else
+#error "bad AES key size"
+#endif
 
 	/* Init periph */
 	at91_aes_init();
 
 	/* Check signature if required */
@@ -115,28 +112,36 @@ int secure_decrypt(void *data, unsigned int data_length, int is_signed)
 	rc = 0;
 exit:
 	/* Reset periph */
 	at91_aes_cleanup();
 
+	return rc;
+}
+
+static void wipe_keys()
+{
 	/* Reset keys */
 	memset(cmac_key, 0, sizeof(cmac_key));
 	memset(cipher_key, 0, sizeof(cipher_key));
 	memset(iv, 0, sizeof(iv));
-
-	return rc;
 }
 
 int secure_check(void *data)
 {
 	const at91_secure_header_t *header;
 	void *file;
+	int ret = -1;
 
 	if (secure_decrypt(data, sizeof(*header), 0))
-		return -1;
+		goto secure_wipe_keys;
 
 	header = (const at91_secure_header_t *)data;
 	if (header->magic != AT91_SECURE_MAGIC)
-		return -1;
+		goto secure_wipe_keys;
 
 	file = (unsigned char *)data + sizeof(*header);
-	return secure_decrypt(file, header->file_size, 1);
+	ret = secure_decrypt(file, header->file_size, 1);
+
+secure_wipe_keys:
+	wipe_keys();
+	return ret;
 }
diff --git a/include/secure.h b/include/secure.h
index 7b72313b..9bd2a77a 100644
--- a/include/secure.h
+++ b/include/secure.h
@@ -38,10 +38,8 @@ typedef struct at91_secure_header {
 	unsigned int		magic;
 	unsigned int		file_size;
 	unsigned int		reserved[2];
 } at91_secure_header_t;
 
-
-int secure_decrypt(void *data, unsigned int data_length, int is_signed);
 int secure_check(void *data);
 
 #endif /* #ifdef __SECURE_H__ */

Vulnerable code: 
static inline void init_keys(at91_aes_key_size_t *key_size,
			     unsigned int *cipher_key,
			     unsigned int *cmac_key,
			     unsigned int *iv)
{
#if defined(CONFIG_AES_KEY_SIZE_128)
	*key_size = AT91_AES_KEY_SIZE_128;
#elif defined(CONFIG_AES_KEY_SIZE_192)
	*key_size = AT91_AES_KEY_SIZE_192;
#elif defined(CONFIG_AES_KEY_SIZE_256)
	*key_size = AT91_AES_KEY_SIZE_256;
#else
#error "bad AES key size"
	iv[0]		= CONFIG_AES_IV_WORD0;
	iv[1]		= CONFIG_AES_IV_WORD1;
	iv[2]		= CONFIG_AES_IV_WORD2;
	iv[3]		= CONFIG_AES_IV_WORD3;

	cipher_key[0]	= CONFIG_AES_CIPHER_KEY_WORD0;
	cmac_key[0]	= CONFIG_AES_CMAC_KEY_WORD0;
	cipher_key[1]	= CONFIG_AES_CIPHER_KEY_WORD1;
	cmac_key[1]	= CONFIG_AES_CMAC_KEY_WORD1;
	cipher_key[2]	= CONFIG_AES_CIPHER_KEY_WORD2;
	cmac_key[2]	= CONFIG_AES_CMAC_KEY_WORD2;
	cipher_key[3]	= CONFIG_AES_CIPHER_KEY_WORD3;
	cmac_key[3]	= CONFIG_AES_CMAC_KEY_WORD3;

	cipher_key[4]	= CONFIG_AES_CIPHER_KEY_WORD4;
	cmac_key[4]	= CONFIG_AES_CMAC_KEY_WORD4;
	cipher_key[5]	= CONFIG_AES_CIPHER_KEY_WORD5;
	cmac_key[5]	= CONFIG_AES_CMAC_KEY_WORD5;

	cipher_key[6]	= CONFIG_AES_CIPHER_KEY_WORD6;
	cmac_key[6]	= CONFIG_AES_CMAC_KEY_WORD6;
	cipher_key[7]	= CONFIG_AES_CIPHER_KEY_WORD7;
	cmac_key[7]	= CONFIG_AES_CMAC_KEY_WORD7;
}
int secure_decrypt(void *data, unsigned int data_length, int is_signed)
	unsigned int cmac_key[8], cipher_key[8];
	unsigned int iv[AT91_AES_IV_SIZE_WORD];
	/* Init keys */
	init_keys(&key_size, cipher_key, cmac_key, iv);

	return rc;
		return -1;
		return -1;
	return secure_decrypt(file, header->file_size, 1);

int secure_decrypt(void *data, unsigned int data_length, int is_signed);
Fixed code: static unsigned int cipher_key[8] = {
	CONFIG_AES_CIPHER_KEY_WORD0,
	CONFIG_AES_CIPHER_KEY_WORD1,
	CONFIG_AES_CIPHER_KEY_WORD2,
	CONFIG_AES_CIPHER_KEY_WORD3,
#if defined(CONFIG_AES_KEY_SIZE_192) || defined(CONFIG_AES_KEY_SIZE_256)
	CONFIG_AES_CIPHER_KEY_WORD4,
	CONFIG_AES_CIPHER_KEY_WORD5,
#if defined(CONFIG_AES_KEY_SIZE_256)
	CONFIG_AES_CIPHER_KEY_WORD6,
	CONFIG_AES_CIPHER_KEY_WORD7,
#endif
};
static unsigned int cmac_key[8] = {
	CONFIG_AES_CMAC_KEY_WORD0,
	CONFIG_AES_CMAC_KEY_WORD1,
	CONFIG_AES_CMAC_KEY_WORD2,
	CONFIG_AES_CMAC_KEY_WORD3,
	CONFIG_AES_CMAC_KEY_WORD4,
	CONFIG_AES_CMAC_KEY_WORD5,
	CONFIG_AES_CMAC_KEY_WORD6,
	CONFIG_AES_CMAC_KEY_WORD7,
};
static unsigned int iv[AT91_AES_IV_SIZE_WORD] = {
	CONFIG_AES_IV_WORD0,
	CONFIG_AES_IV_WORD1,
	CONFIG_AES_IV_WORD2,
	CONFIG_AES_IV_WORD3,
};

static int secure_decrypt(void *data, unsigned int data_length, int is_signed)
#if defined(CONFIG_AES_KEY_SIZE_128)
	key_size = AT91_AES_KEY_SIZE_128;
#elif defined(CONFIG_AES_KEY_SIZE_192)
	key_size = AT91_AES_KEY_SIZE_192;
#elif defined(CONFIG_AES_KEY_SIZE_256)
	key_size = AT91_AES_KEY_SIZE_256;
#else
#error "bad AES key size"
#endif
	return rc;
}

static void wipe_keys()
{
	int ret = -1;
		goto secure_wipe_keys;
		goto secure_wipe_keys;
	ret = secure_decrypt(file, header->file_size, 1);

secure_wipe_keys:
	wipe_keys();
	return ret;