Fix IP SAN handling in cert_write and cert_req - #11002

Description

The cert_write and cert_req sample programs mishandle IP: entries in the san parameter in two ways. This PR fixes both.

1. Stack buffer overflow with an IPv6 address

Both programs declare a 4-byte buffer and pass it to mbedtls_x509_crt_parse_cn_inet_pton():

uint8_t ip[4] = { 0 };                                   /* cert_write.c:319, cert_req.c:156 */
ip_addr_len = mbedtls_x509_crt_parse_cn_inet_pton(subtype_value, ip);  /* cert_write.c:646, cert_req.c:264 */
cur->node.san.unstructured_name.len = sizeof(ip);        /* cert_write.c:653, cert_req.c:271 */

The function's documentation in include/mbedtls/x509.h says:

The buffer MUST be 16 bytes to save IPv6, and should be 4-byte aligned if the result will be used as struct in_addr. e.g. uint32_t dst[4]

A value containing : is parsed as IPv6 and 16 bytes are written, so 12 bytes land past the end of ip. Reproduce with an ASan build:

programs/x509/cert_write issuer_crt=ca.crt issuer_key=ca.key subject_key=sub.key \
    san=IP:2001:db8::1 output_file=out.crt

ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 16 at 0x... thread T0
    #0 inet_pton
    #1 x509_inet_pton_ipv6           library/x509_crt.c:2846
    #2 mbedtls_x509_crt_parse_cn_inet_pton  library/x509_crt.c:2860
    #3 main                          programs/x509/cert_write.c:646
    [5840, 5844) 'ip' (line 319)
    [5856, 5864) 'tmp_san_dirname' (line 663) <== Memory access at offset 5844 partially underflows this variable

cert_req gives the same report against its ip at line 156. Without ASan the program does not crash, and the length is still hard coded to 4. The usage text says IP:value (Only IPv4 is supported), but nothing enforces that.

That note dates from the earlier hand-written IPv4 parser. #7638 replaced it with mbedtls_x509_crt_parse_cn_inet_pton(), which accepts IPv6, but kept the 4-byte buffer. The SAN writer copies unstructured_name.len bytes as given, so a 16-byte address is encoded correctly once the programs pass the right length. This PR makes IPv6 work.

2. Several IP entries all get the last address

Every IP entry points at that one shared ip buffer, and the SAN list is only written after all entries are parsed. So:

cert_write ... san=IP:1.2.3.4;IP:5.6.7.8
openssl x509 -in out.crt -noout -ext subjectAltName
    IP Address:5.6.7.8, IP Address:5.6.7.8

With san=IP:2001:db8::1;IP:10.0.0.1 and only the first fix applied, the IPv4 address overwrites the first 4 bytes of the IPv6 one, giving IP Address:A00:1:0:0:0:0:0:1.

Fix

Two commits, applied identically to both programs.

1. Use a 16-byte buffer and set the SAN length to the value mbedtls_x509_crt_parse_cn_inet_pton() returns. Update the usage text to IP:value (IPv4 or IPv6).
2. Give each IP entry its own 16-byte buffer from mbedtls_calloc(), and free it in the existing cleanup loop at exit:, as is already done for DN entries. This removes the stack buffer, and with it a redundant #if defined(MBEDTLS_X509_CRT_PARSE_C) in cert_req.c. The whole program already requires that option.

Security impact

The overflow is only reachable through the san command-line argument of a sample program, which the person running it controls. The library itself is not affected. Its own caller in x509_crt.c and the x509parse test suite both pass 16-byte buffers. This matches the earlier "Fix possible NULL dereference issue in X509 cert_write/cert_req program if an entry in the san parameter is not separated by a colon" fixes, which were listed as ordinary bugfixes in 3.6.0.

Testing

There is no test harness that runs cert_write or cert_req, as #7638 also noted. I tested by hand with an ASan and UBSan build (clang, -fsanitize=address,undefined -fno-sanitize-recover=all, ASAN_OPTIONS=detect_leaks=1). The script runs both programs with five inputs and compares the SAN that openssl x509 / openssl req decodes:

That is 6 of 10 checks failing on the parent and 10 of 10 passing with this PR, on development, mbedtls-4.1 and mbedtls-3.6.

I also broke each part of the fix and reran the checks:

• buffer back to 4 bytes: 4 checks fail, ASan reports a heap-buffer-overflow
• length hard-coded to 4 again: 4 checks fail
• no free in the cleanup loop: 4 checks fail with a LeakSanitizer report
• no free of the buffer when parsing fails: no check fails. Both programs call mbedtls_exit() from inside main, so LeakSanitizer still sees the local pointer. I kept the free because the buffer is otherwise leaked.

framework/scripts/code_style.py (uncrustify 0.75.1), framework/scripts/check_files.py and framework/scripts/assemble_changelog.py pass. Both programs build with gcc and MBEDTLS_FATAL_WARNINGS=ON on all three branches.

Not changed

• No automated test. Covering the programs would need a new harness, or a new certificate in the framework's data_files, which only exercises the parser. I left that for the maintainers to decide.
• When an IP or DN entry fails to parse, the list node allocated just before it is still not freed. That was already the case and the program exits right away, so I did not touch it.
• The writer emits SAN entries in reverse order of the san list. That is unchanged.
• Passing san= twice replaces the first list without freeing it. That leak was already there. With this PR it also holds the IP buffers of the first list.
• cert_write and cert_req still duplicate their SAN parsing. Moving it into one shared helper would stop a fix like this one being needed twice, but it is a larger change.

PR checklist

• changelog provided
• framework PR not required
• TF-PSA-Crypto development PR not required because: no change to crypto code
• TF-PSA-Crypto 1.1 PR not required because: no change to crypto code
• mbedtls development PR provided
• mbedtls 4.1 PR provided [4.1] Fix IP SAN handling in cert_write and cert_req #11003
• mbedtls 3.6 PR provided [3.6] Fix IP SAN handling in cert_write and cert_req #11004
• tests not required because: there is no test harness for cert_write and cert_req. Tested by hand under ASan as described above.