Commit e8ebb28
committed
net/ssl-bear: fix OOM crashes and pre-existing leaks in x509 cert parsing
Three interlocking bugs in the BearSSL cert-parsing path, all
triggered by OOM during trust-anchor load:
=== Bug 1: blobdup had no NULL check ===
static uint8_t* blobdup(const void * src, size_t len)
{
uint8_t *ret = (uint8_t*)malloc(len);
memcpy(ret, src, len); /* NULL-deref on OOM */
return ret;
}
Called three times from append_cert_x509 to copy the RSA .n / .e
or EC .q public-key blobs into the trust-anchor slot. On malloc
failure the memcpy segfaulted before blobdup could return.
Fix: NULL-check the malloc and return NULL. Each call site now
propagates the NULL back out of append_cert_x509.
=== Bug 2: vdn_append did realloc-to-self ===
current_vdn = (uint8_t*)realloc(current_vdn, current_vdn_size + len);
memcpy(current_vdn + current_vdn_size, src, len);
This is the classic 'x = realloc(x, n)' pattern: on failure,
current_vdn is overwritten with NULL and the original buffer is
leaked, then the next line memcpy's into NULL.
vdn_append is a BearSSL x509-decoder callback with a void return
type, so there is no way to signal failure mid-decode. Latched a
file-scope 'current_vdn_failed' flag: on realloc failure set the
flag, skip the memcpy, and short-circuit subsequent invocations.
append_cert_x509 then checks the flag after br_x509_decoder_push
returns and bails if it is set.
=== Bug 3: append_cert_x509 had pre-existing leaks of current_vdn ===
Two early-return paths (the '!pk || !isCA' check at line 71 and
the default case of the key_type switch) returned 'false' without
freeing current_vdn. Ownership had not yet been transferred to
ta->dn.data when those paths fired, so the buffer was orphaned
every time a non-CA or unsupported-key-type certificate appeared
in the PEM bundle.
/etc/ssl/certs/ca-certificates.crt on a typical Linux system
contains ~140 certificates and is loaded once at SSL-startup, so
this was a bounded leak in practice, but still a leak. Added
'free(current_vdn)' on both paths.
=== Bug 4: partial-blobdup failure would crash TLS handshakes ===
ta->pkey.key.rsa.n = blobdup(...);
ta->pkey.key.rsa.e = blobdup(...);
break; /* no check; TAs_NUM++ and proceed */
With Bug 1 fixed, blobdup now returns NULL cleanly on OOM. But
append_cert_x509 still committed the half-built trust anchor
regardless. BearSSL dereferences ta->pkey.key.rsa.n / .e during
every TLS handshake against a certificate chain that terminates
at this TA; a NULL field there causes a crash in the handshake
path, well away from the original OOM.
Added explicit checks after each blobdup call. On any NULL,
free any siblings that did succeed (free(NULL) is a no-op for
the already-NULL one), free current_vdn, zero out the pointer
fields so debugging is cleaner, and return false without
incrementing TAs_NUM.
=== Not fixed in this commit ===
ssl_socket_init in this same file has a separate no-NULL-check
calloc bug that will be addressed in a follow-up. The mbedtls
variant (net_socket_ssl_mbed.c) has the same ssl_socket_init
NULL-deref plus an mbedtls-sub-object leak on its 'error:' label;
also follow-up material.
Thread-safety: unchanged. append_cert_x509 and vdn_append run
only from ssl_socket_init -> initialize() at startup. The file
already carries a TODO on 'initialize()' noting it is not thread
safe; the new current_vdn_failed flag is no worse than the
existing current_vdn / current_vdn_size statics it lives
alongside.
Reachability: every SSL-enabled connection at startup, via
initialize() -> append_certs_pem_x509 -> append_cert_x509 ->
vdn_append / blobdup. Both malloc and realloc here are for
small allocations (DN bytes are ~100 bytes, public-key blobs
are ~250-550 bytes for RSA-2048), so OOM is unlikely but
possible under extreme memory pressure at startup.1 parent 22be8c8 commit e8ebb28
1 file changed
Lines changed: 69 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
45 | 51 | | |
46 | 52 | | |
47 | 53 | | |
48 | 54 | | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
49 | 61 | | |
50 | 62 | | |
51 | 63 | | |
52 | 64 | | |
53 | 65 | | |
54 | | - | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
55 | 82 | | |
56 | 83 | | |
57 | 84 | | |
| |||
64 | 91 | | |
65 | 92 | | |
66 | 93 | | |
| 94 | + | |
67 | 95 | | |
68 | 96 | | |
69 | 97 | | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
70 | 108 | | |
71 | 109 | | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
72 | 115 | | |
| 116 | + | |
73 | 117 | | |
74 | 118 | | |
75 | 119 | | |
| |||
83 | 127 | | |
84 | 128 | | |
85 | 129 | | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
86 | 144 | | |
87 | 145 | | |
88 | 146 | | |
89 | 147 | | |
90 | 148 | | |
91 | 149 | | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
92 | 156 | | |
93 | 157 | | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
94 | 162 | | |
95 | 163 | | |
96 | 164 | | |
| |||
0 commit comments