Skip to content

Fix memory leaks and use-after-free detected by ASan #735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jfclere wants to merge 1 commit into from
+48 −22
Closed

Fix memory leaks and use-after-free detected by ASan #735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions modules/md/md_acme.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,9 +419,11 @@ static apr_status_t md_acme_req_send(md_acme_req_t *req, int get_as_post)
md_log_perror(MD_LOG_MARK, MD_LOG_ERR, 0, req->p,
"HTTP method %s against: %s", req->method, req->url);
rv = APR_ENOTIMPL;
goto leave;
}
md_log_perror(MD_LOG_MARK, MD_LOG_DEBUG, rv, req->p, "req sent");

/* on_response has already called md_acme_req_done which destroyed
* req and its pool, unless it returned APR_EAGAIN for a retry. */
md_log_perror(MD_LOG_MARK, MD_LOG_DEBUG, rv, acme->p, "req sent");
if (APR_EAGAIN == rv && req->max_retries > 0) {
--req->max_retries;
rv = md_acme_req_send(req, 1);
Expand Down
40 changes: 28 additions & 12 deletions modules/md/md_crypt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,10 +600,13 @@ md_pkey_spec_t *md_pkeys_spec_get(const md_pkeys_spec_t *pks, int index)
return NULL;
}

static apr_status_t pkey_cleanup(void *data);

static md_pkey_t *make_pkey(apr_pool_t *p)
{
md_pkey_t *pkey = apr_pcalloc(p, sizeof(*pkey));
pkey->pool = p;
apr_pool_cleanup_register(p, pkey, pkey_cleanup, apr_pool_cleanup_null);
return pkey;
}

Expand Down Expand Up @@ -647,7 +650,6 @@ apr_status_t md_pkey_fload(md_pkey_t **ppkey, apr_pool_t *p,

if (pkey->pkey != NULL) {
rv = APR_SUCCESS;
apr_pool_cleanup_register(p, pkey, pkey_cleanup, apr_pool_cleanup_null);
}
else {
unsigned long err = ERR_get_error();
Expand Down Expand Up @@ -773,7 +775,6 @@ apr_status_t md_pkey_read_http(md_pkey_t **ppkey, apr_pool_t *pool,
goto leave;
}
rv = APR_SUCCESS;
apr_pool_cleanup_register(pool, pkey, pkey_cleanup, apr_pool_cleanup_null);

leave:
*ppkey = (APR_SUCCESS == rv)? pkey : NULL;
Expand Down Expand Up @@ -1951,6 +1952,8 @@ static apr_status_t mk_x509(X509 **px, md_pkey_t *pkey, const char *cn,
X509_NAME *n = NULL;
BIGNUM *big_rnd = NULL;
ASN1_INTEGER *asn1_rnd = NULL;
ASN1_TIME *not_before = NULL;
ASN1_TIME *not_after = NULL;
unsigned char rnd[20];
int days;
apr_status_t rv;
Expand Down Expand Up @@ -1995,10 +1998,10 @@ static apr_status_t mk_x509(X509 **px, md_pkey_t *pkey, const char *cn,
}
/* validity */
days = (int)((apr_time_sec(valid_for) + MD_SECS_PER_DAY - 1)/ MD_SECS_PER_DAY);
if (!X509_set_notBefore(x, ASN1_TIME_set(NULL, time(NULL)))) {
rv = APR_EGENERAL; goto out;
}
if (!X509_set_notAfter(x, ASN1_TIME_adj(NULL, time(NULL), days, 0))) {
not_before = ASN1_TIME_set(NULL, time(NULL));
not_after = ASN1_TIME_adj(NULL, time(NULL), days, 0);
if (!not_before || !X509_set_notBefore(x, not_before)
|| !not_after || !X509_set_notAfter(x, not_after)) {
rv = APR_EGENERAL; goto out;
}

Expand All @@ -2007,6 +2010,8 @@ static apr_status_t mk_x509(X509 **px, md_pkey_t *pkey, const char *cn,
if (APR_SUCCESS != rv && x) X509_free(x);
if (big_rnd) BN_free(big_rnd);
if (asn1_rnd) ASN1_INTEGER_free(asn1_rnd);
ASN1_TIME_free(not_before);
ASN1_TIME_free(not_after);
if (n) X509_NAME_free(n);
return rv;
}
Expand Down Expand Up @@ -2228,6 +2233,8 @@ apr_status_t md_cert_get_ari_cert_id(const char **pari_cert_id,
int i = -1, sder_len;
unsigned char *ucp, *sbuf;

apr_status_t rv;

*pari_cert_id = NULL;
s_aki = X509_get_ext_d2i(cert->x509, NID_authority_key_identifier, &i, NULL);
if (s_aki == NULL) {
Expand All @@ -2239,7 +2246,8 @@ apr_status_t md_cert_get_ari_cert_id(const char **pari_cert_id,
if (aki == NULL) {
md_log_perror(MD_LOG_MARK, MD_LOG_ERR, 0, p,
"cert has no authority key id in extension");
return APR_ENOENT;
rv = APR_ENOENT;
goto out;
}
akid_buf.len = (apr_size_t)ASN1_STRING_length(aki);
akid_buf.data = (const char *)ASN1_STRING_get0_data(aki);
Expand All @@ -2249,26 +2257,34 @@ apr_status_t md_cert_get_ari_cert_id(const char **pari_cert_id,
if (!serial) {
md_log_perror(MD_LOG_MARK, MD_LOG_ERR, 0, p,
"cert has no serial number");
return APR_ENOENT;
rv = APR_ENOENT;
goto out;
}
memset(&ser_buf, 0, sizeof(ser_buf));
bn = ASN1_INTEGER_to_BN(serial, NULL);
if (!bn) {
return APR_EINVAL;
rv = APR_EINVAL;
goto out;
}
sbuf = apr_pcalloc(p, BN_num_bytes(bn));
sder_len = BN_bn2bin(bn, sbuf);
BN_free(bn);
if (sder_len < 1)
return APR_EINVAL;
if (sder_len < 1) {
rv = APR_EINVAL;
goto out;
}
ser_buf.len = (apr_size_t)sder_len;
ser_buf.data = (const char *)sbuf;
(void)ucp;

*pari_cert_id = apr_psprintf(p, "%s.%s",
md_util_base64url_encode(&akid_buf, p),
md_util_base64url_encode(&ser_buf, p));
return APR_SUCCESS;
rv = APR_SUCCESS;

out:
AUTHORITY_KEYID_free(s_aki);
return rv;
#else
*pari_cert_id = NULL;
(void)cert;
Expand Down
18 changes: 10 additions & 8 deletions modules/md/md_json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ int md_json_itera(md_json_itera_cb *cb, void *baton, md_json_t *json, ...)
va_list ap;
size_t index;
json_t *val;
md_json_t wrap;
md_json_t *wrap;

va_start(ap, json);
j = jselect(json, ap);
Expand All @@ -822,10 +822,11 @@ int md_json_itera(md_json_itera_cb *cb, void *baton, md_json_t *json, ...)
return 0;
}

wrap.p = json->p;
wrap = apr_pcalloc(json->p, sizeof(*wrap));
wrap->p = json->p;
json_array_foreach(j, index, val) {
wrap.j = val;
if (!cb(baton, index, &wrap)) {
wrap->j = val;
if (!cb(baton, index, wrap)) {
return 0;
}
}
Expand All @@ -838,7 +839,7 @@ int md_json_iterkey(md_json_iterkey_cb *cb, void *baton, md_json_t *json, ...)
va_list ap;
const char *key;
json_t *val;
md_json_t wrap;
md_json_t *wrap;

va_start(ap, json);
j = jselect(json, ap);
Expand All @@ -848,10 +849,11 @@ int md_json_iterkey(md_json_iterkey_cb *cb, void *baton, md_json_t *json, ...)
return 0;
}

wrap.p = json->p;
wrap = apr_pcalloc(json->p, sizeof(*wrap));
wrap->p = json->p;
json_object_foreach(j, key, val) {
wrap.j = val;
if (!cb(baton, key, &wrap)) {
wrap->j = val;
if (!cb(baton, key, wrap)) {
return 0;
}
}
Expand Down
3 changes: 3 additions & 0 deletions modules/ssl/ssl_engine_kernel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,9 @@ static int ssl_hook_Access_classic(request_rec *r, SSLSrvConfigRec *sc, SSLDirCo
* Modifying the SSL on a slave connection is no good.
*/
apr_table_setn(r->notes, "ssl-renegotiate-forbidden", "cipher-suite");
if (cipher_list_old) {
sk_SSL_CIPHER_free(cipher_list_old);
}
return HTTP_FORBIDDEN;
}

Expand Down
3 changes: 3 additions & 0 deletions support/a2md/md_cmd_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ int main(int argc, const char *const *argv)
fprintf(stderr, "error initializing pool\n");
return 1;
}
apr_allocator_owner_set(allocator, p);

md_http_use_implementation(md_curl_get_impl(p));
md_acme_init(p, BASE_VERSION, 1);
Expand All @@ -462,5 +463,7 @@ int main(int argc, const char *const *argv)
fprintf(stdout, "%s\n", out ? out : "<failed to serialize!>");
}

apr_pool_destroy(p);

return (rv == APR_SUCCESS)? 0 : 1;
}