Error when generating a certificate via mbedtls_x509write_crt_pem and then parsing it.

[Faless]

Description

• Type: Bug
• Priority: Major

---

Bug

mbed TLS build:
Version: 2.16.8 (build statically, with GCC)

Due to the changes in #2632 (and its backport #3488), creating a PEM certificate via mbedtls_x509write_crt_pem, then reading it into a mbedtls_x509_crt via mbedtls_x509_crt_parse will fail when passing the full buffer size in 2.16.8 while not failing in 2.16.7.

(see example code below, compiled with gcc example.c -I./build/include/ -Lbuild/library/ -lmbedtls -lmbedx509 -lmbedcrypto -Wl,-rpath,"build/library").

#include <mbedtls/ctr_drbg.h>
#include <mbedtls/entropy.h>
#include <mbedtls/pem.h>
#include <mbedtls/ssl.h>

#include <stdio.h>
#include <string.h>

int main(int argc, char **argv) {
	// Init entropy/random number generator.
	mbedtls_entropy_context entropy;
	mbedtls_ctr_drbg_context ctr_drbg;

	mbedtls_ctr_drbg_init(&ctr_drbg);
	mbedtls_entropy_init(&entropy);

	mbedtls_ctr_drbg_seed(&ctr_drbg, mbedtls_entropy_func, &entropy, NULL, 0);

	// Create key
	mbedtls_pk_context pkey;
	mbedtls_pk_init(&pkey);
	int ret = mbedtls_pk_setup(&pkey, mbedtls_pk_info_from_type(MBEDTLS_PK_RSA));
	ret = mbedtls_rsa_gen_key(mbedtls_pk_rsa(pkey), mbedtls_ctr_drbg_random, &ctr_drbg, 2048, 65537);

	// Create certificate
	mbedtls_x509write_cert crt;
	mbedtls_x509write_crt_init(&crt);

	mbedtls_x509write_crt_set_subject_key(&crt, &(pkey));
	mbedtls_x509write_crt_set_issuer_key(&crt, &(pkey));
	mbedtls_x509write_crt_set_subject_name(&crt, "CN=example.com,O=A Game Company,C=IT");
	mbedtls_x509write_crt_set_issuer_name(&crt, "CN=example.com,O=A Game Company,C=IT");
	mbedtls_x509write_crt_set_version(&crt, MBEDTLS_X509_CRT_VERSION_3);
	mbedtls_x509write_crt_set_md_alg(&crt, MBEDTLS_MD_SHA256);

	mbedtls_mpi serial;
	mbedtls_mpi_init(&serial);
	uint8_t rand_serial[20];
	mbedtls_ctr_drbg_random(&ctr_drbg, rand_serial, 20);
	mbedtls_mpi_read_binary(&serial, rand_serial, 20);
	mbedtls_x509write_crt_set_serial(&crt, &serial);

	mbedtls_x509write_crt_set_validity(&crt, "20140101000000", "20340101000000");
	mbedtls_x509write_crt_set_basic_constraints(&crt, 1, -1);
	mbedtls_x509write_crt_set_basic_constraints(&crt, 1, 0);

	// Write certificate to buffer.
	unsigned char buf[4096];
	memset(buf, 0, 4096);
	mbedtls_x509_crt out;
	mbedtls_x509_crt_init(&out);
	mbedtls_x509write_crt_pem(&crt, buf, 4096, mbedtls_ctr_drbg_random, &ctr_drbg); // In 2.16.8, due to #3488/#2632 , this writes at the end of the buffer.

	// Parse the wrote buffer in output certificate.
	int err = mbedtls_x509_crt_parse(&out, buf, 4096); // This fail in 2.16.8 because buf[buflen-1] is not '\0'.
	// buf[4095] = '\0'; would work around it while still hacky.
	// Using `strlen((char *)buf) + 1` instead of 4096 would also work.
	if (err != 0) {
	        mbedtls_mpi_free(&serial);
	        mbedtls_x509write_crt_free(&crt);
	        printf("Generated invalid certificate\n");
	        return 1;
	}

	mbedtls_mpi_free(&serial);
	mbedtls_x509write_crt_free(&crt);
	mbedtls_x509_crt_free(&out);
	return 0;
}

[paul-elliott-arm]

Hi! Do you guys already have a potential patch that you think fixes this? If so, then do feel free to submit that for review.

[Faless]

Hi, no actually, we added the aforementioned workaround in our code, using strlen to detect the actual written bytes.

[gilles-peskine-arm]

I agree that this is a bug. It's an unintended regression. I've posted a more detailed analysis in #3896.

I think that we should relax the idiom for PEM parsing functions to require the input to be null-terminated, i.e. require that there is a null byte in the buffer, even if it isn't the last byte.

However, we may also or alternatively change the writing functions to zero out the temporary data at the end of the buffer. The temporary data could cause other problems, such as a data leak after writing a private key, if the caller only wiped the first strlen bytes of the buffer and not the whole buffer.

[mpg]

Hi @Faless. Thanks for your report, especially providing an example file which makes it very easy to reproduce the problem and understand what exactly is going on. And of course pointing to the PR that introduced the issue is appreciated!

Also, sorry for not replying earlier - we usually try to give at least an initial reply within a few business days, but for some reason we appear to have missed this report.

Regarding the issue, I agree that this is a change of observable behaviour in mbedtls_x509write_crt_pem() which breaks working and reasonable code. Now if we wanted to be picky about it, the documentation never said the bytes at the end of the buffer would be preserved, so the code was relying on undocumented behaviour. But to be honest, that assumption was quite reasonable, and I don't think we really realised this change would have that effect - if we had, we probably would have thought twice about backporting it.

Regarding the resolution, I'm think we could make mbedtls_x509write_crt_pem() zeroize all bytes at the end of the buffer. While it wouldn't completely restore the 2.16.7 behaviour (which is probably not desirable because it would increase stack usage again, and some people may have come to rely on the lower stack usage in the meantime), it would at least fix the common case that your example is demonstrating. Would you agree this would be a reasonable resolution?

[mpg]

Oops, I failed to refresh the page before replying and didn't noticed that Paul and Gilles had already done so.

[mu578]

About this topic and facilitating round trip; did someone write a support function such as?

mbedtls_x509write_cert_init_with_x509(mbedtls_x509write_cert * ctx, const mbedtls_x509_cert * existing);

THX if any.

[mu578]

@Faless give away

#	define ___CERT_PEM_BEGIN "-----BEGIN CERTIFICATE-----\n"
#	define ___CERT_PEM_END   "-----END CERTIFICATE-----\n"
#	define ___MAX_HEAP_BLOCK_ALLOWED  /* implementation defined */

#	define mbedtlsext_addrck(addr)        (addr != NULL)
#	define mbedtlsext_alloc(size)         malloc(size)
#	define mbedtlsext_dealloc(addr)       if (mbedtlsext_addrck(addr)) { free(addr); } addr = NULL
#	define mbedtlsext_memcpy(dst, src, n) memcpy(dst, src, n)

#	define mbedtlsext_reinterpret_cast(type, expr) (type)expr

#	define mbedtlsext_sizeck(size) \
	(size > 0 && mbedtlsext_reinterpret_cast(size_t, size) < ___MAX_HEAP_BLOCK_ALLOWED)

#	define mbedtlsext_offsetck(expr, limit) \
	(mbedtlsext_reinterpret_cast(off_t, (expr)) >= 0 && mbedtlsext_reinterpret_cast(off_t, (limit)) \
	>= 0 && mbedtlsext_reinterpret_cast(size_t, (expr)) < mbedtlsext_reinterpret_cast(size_t, (limit)))

int mbedtlsext_x509_crt_write_der(
	  const mbedtls_x509_crt * crts
	, size_t                   ncrt
	, off_t                    idx
	, size_t *                 olen
	, unsigned char **         dst
) {
	int ret = -1;

	const mbedtls_x509_crt * crt;

	if (
		   mbedtlsext_addrck(crts)
		&& mbedtlsext_sizeck(ncrt)
		&& mbedtlsext_offsetck(idx, ncrt)
		&& mbedtlsext_addrck(olen)
		&& mbedtlsext_addrck(dst)
	) {
		*olen = 0;
		*dst  = NULL;
		 crt  = &(crts);
		while (idx && mbedtlsext_addrck(crt)) {
			crt = crt->next;
			--idx;
		}
		if (idx == 0 && mbedtlsext_addrck(crt)) {
			if (
				   mbedtlsext_addrck(crt->raw.p)
				&& mbedtlsext_sizeck(crt->raw.len)
			) {
				if (mbedtlsext_sizeck(crt->raw.len)) {
					*dst = mbedtlsext_alloc(crt->raw.len);
					if (mbedtlsext_addrck(dst)) {
						mbedtlsext_memcpy(*dst, crt->raw.p, crt->raw.len);
						*olen = crt->raw.len;
						 ret  = 0;
					}
				}
			}
		}
	}
	return ret;
}

int mbedtlsext_x509_crt_write_pem(
	  const mbedtls_x509_crt * crts
	, size_t                   ncrt
	, off_t                    idx
	, size_t *                 oblen
	, size_t *                 oslen
	, char **                  dst
) {
	int ret = -1;

	const mbedtls_x509_crt * crt;
	size_t blen;

	if (
		   mbedtlsext_addrck(crts)
		&& mbedtlsext_sizeck(ncrt)
		&& mbedtlsext_offsetck(idx, ncrt)
		&& mbedtlsext_addrck(written)
		&& mbedtlsext_addrck(oblen)
		&& mbedtlsext_addrck(oslen)
		&& mbedtlsext_addrck(dst)
	) {
		*oblen = 0;
		*oslen = 0;
		*dst   = NULL;
		 crt   = &(crts);
		while (idx && mbedtlsext_addrck(crt)) {
			crt = crt->next;
			--idx;
		}
		if (idx == 0 && mbedtlsext_addrck(crt)) {
			if (
				   mbedtlsext_addrck(crt->raw.p)
				&& mbedtlsext_sizeck(crt->raw.len)
			) {
				ret = mbedtls_pem_write_buffer(
					  ___CERT_PEM_BEGIN
					, ___CERT_PEM_END
					, crt->raw.p
					, crt->raw.len
					, null
					, 0
					, &blen
				);
			}
			if (
				   (ret == 0 || ret == MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL)
				&& mbedtlsext_sizeck(blen)
			) {
				*dst = mbedtlsext_alloc(blen);
				if (mbedtlsext_addrck(*dst)) {
					if (0 != (ret = mbedtls_pem_write_buffer(
							  ___CERT_PEM_BEGIN
							, ___CERT_PEM_END
							, crt->raw.p
							, crt->raw.len
							, mbedtlsext_reinterpret_cast(unsigned char *, *dst)
							, blen
							, &blen
					))) {
						mbedtlsext_dealloc(*dst);
						return ret;
					}
					*oblen = blen;
					*oslen = blen - 1;
				} else {
					ret = -2;
				}
			}
		}
	}
	return ret;
}