AES contexts not properly reinitialized in programs/test/benchmark.c

[ShellCode33]

Hey, a small bug report that is not really impactful considering it is not part of the crypto library itself, but I thought I would let you know anyway.

mbedtls/programs/test/benchmark.c

Lines 803 to 823 in 824a8dc

	#if defined(MBEDTLS_CIPHER_MODE_XTS)
	if (todo.aes_xts) {
	int keysize;
	mbedtls_aes_xts_context ctx;
	mbedtls_aes_xts_init(&ctx);
	for (keysize = 128; keysize <= 256; keysize += 128) {
	mbedtls_snprintf(title, sizeof(title), "AES-XTS-%d", keysize);
	memset(buf, 0, sizeof(buf));
	memset(tmp, 0, sizeof(tmp));
	CHECK_AND_CONTINUE(mbedtls_aes_xts_setkey_enc(&ctx, tmp, keysize * 2));
	TIME_AND_TSC(title,
	mbedtls_aes_crypt_xts(&ctx, MBEDTLS_AES_ENCRYPT, BUFSIZE,
	tmp, buf, buf));
	mbedtls_aes_xts_free(&ctx);
	}
	}
	#endif

Notice how the mbedtls_aes_xts_free function is called in the loop.

There is the same issue for GCM and CCM.

[davidhorstmann-arm]

Hi, thanks for this report!

I think that it is not actually a use-after-free. The mbedtls_aes_xts_context struct contains no pointers and we allocate it entirely on the stack so mbedtls_aes_xts_free() doesn't actually call free() on anything (just zeroizes the structure).

However, you're right that we're not following our own documentation here, which states that the mbedtls_aes_xts_context must be initialized (by a call to mbedtls_aes_xts_init()).

As you say, this is in a program rather than the actual library, so it's fine to report in public, but if you find something more critical, please use our security email alias as described here.

Thanks again for raising this!

[davidhorstmann-arm]

I think that it is not actually a use-after-free.

Having said that, I haven't looked into the CCM/GCM cases, it is possible these are different. I will check those and see if they are different.

[gilles-peskine-arm]

The functions mbedtls_xxx_free() (excluding mbedtls_free() itself) have a misleading name: they are closer to reset functions than to free functions. The name comes from the fact that they free nested pointers. They set the pointer fields to NULL, so they're idempotent.

In most cases, mbedtls_xxx_free(&foo) sets foo to the same state as after calling mbedtls_init_foo(&foo). There may be exceptions, however, because this is poorly documented and untested. In particular:

• ALT implementations define their own context type and associated functions. Due to the lack of documentation and testing, these implementations might not respect the invariants we expect.
• Some structures that contain a mutex may deviate from the pattern on platforms where mutex initialization allocates a resource.
• Structures where init() doesn't set all fields to 0 may be problematic if free() doesn't do the same. I'm not aware of any actual problem though, except maybe with mutexes. In particular, mbedtls_mpi_init() sets the sign field to 1, and mbedtls_mpi_free() does the same.

---

@ShellCode33 How did you notice the problem? If it's based on the function names, it's a false positive. If it's based on static analysis (i.e. actual semantic reasoning, not IA), the cited code looks correct to me. What exactly does the tool report?

[ShellCode33]

Thanks for the clarification! I didn't use any tool, I was trying to change some of the benchmarks and noticed this with my own eyes.

As you said it's a bit misleading, and what comforted me in the fact that it was a mistake is that it is not done like this for all AES benchmarks, take CBC for exemple:

mbedtls/programs/test/benchmark.c

Lines 726 to 737 in 824a8dc

	mbedtls_aes_init(&aes);
	for (keysize = 128; keysize <= 256; keysize += 64) {
	mbedtls_snprintf(title, sizeof(title), "AES-CBC-%d", keysize);
	memset(buf, 0, sizeof(buf));
	memset(tmp, 0, sizeof(tmp));
	CHECK_AND_CONTINUE(mbedtls_aes_setkey_enc(&aes, tmp, keysize));
	TIME_AND_TSC(title,
	mbedtls_aes_crypt_cbc(&aes, MBEDTLS_AES_ENCRYPT, BUFSIZE, tmp, buf, buf));
	}
	mbedtls_aes_free(&aes);

But I guess there's a reason for it I don't know about