From 489deee29f427f22e2a26de729319bdb70819c37 Mon Sep 17 00:00:00 2001 From: Greg Hudson Date: Tue, 5 Mar 2024 19:53:07 -0500 Subject: [PATCH 2/2] Fix two unlikely memory leaks In gss_krb5int_make_seal_token_v3(), one of the bounds checks (which could probably never be triggered) leaks plain.data. Fix this leak and use current practices for cleanup throughout the function. In xmt_rmtcallres() (unused within the tree and likely elsewhere), store port_ptr into crp->port_ptr as soon as it is allocated; otherwise it could leak if the subsequent xdr_u_int32() operation fails. (cherry picked from commit c5f9c816107f70139de11b38aa02db2f1774ee0d) --- src/lib/gssapi/krb5/k5sealv3.c | 56 +++++++++++++++------------------- src/lib/rpc/pmap_rmt.c | 9 +++--- 2 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/lib/gssapi/krb5/k5sealv3.c b/src/lib/gssapi/krb5/k5sealv3.c index 3b4f8cb837..e881eee835 100644 --- a/src/lib/gssapi/krb5/k5sealv3.c +++ b/src/lib/gssapi/krb5/k5sealv3.c @@ -65,7 +65,7 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, int conf_req_flag, int toktype) { size_t bufsize = 16; - unsigned char *outbuf = 0; + unsigned char *outbuf = NULL; krb5_error_code err; int key_usage; unsigned char acceptor_flag; @@ -75,9 +75,13 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, #endif size_t ec; unsigned short tok_id; - krb5_checksum sum; + krb5_checksum sum = { 0 }; krb5_key key; krb5_cksumtype cksumtype; + krb5_data plain = empty_data(); + + token->value = NULL; + token->length = 0; acceptor_flag = ctx->initiate ? 0 : FLAG_SENDER_IS_ACCEPTOR; key_usage = (toktype == KG_TOK_WRAP_MSG @@ -107,14 +111,15 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, #endif if (toktype == KG_TOK_WRAP_MSG && conf_req_flag) { - krb5_data plain; krb5_enc_data cipher; size_t ec_max; size_t encrypt_size; /* 300: Adds some slop. */ - if (SIZE_MAX - 300 < message->length) - return ENOMEM; + if (SIZE_MAX - 300 < message->length) { + err = ENOMEM; + goto cleanup; + } ec_max = SIZE_MAX - message->length - 300; if (ec_max > 0xffff) ec_max = 0xffff; @@ -126,20 +131,20 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, #endif err = alloc_data(&plain, message->length + 16 + ec); if (err) - return err; + goto cleanup; /* Get size of ciphertext. */ encrypt_size = krb5_encrypt_size(plain.length, key->keyblock.enctype); if (encrypt_size > SIZE_MAX / 2) { err = ENOMEM; - goto error; + goto cleanup; } bufsize = 16 + encrypt_size; /* Allocate space for header plus encrypted data. */ outbuf = gssalloc_malloc(bufsize); if (outbuf == NULL) { - free(plain.data); - return ENOMEM; + err = ENOMEM; + goto cleanup; } /* TOK_ID */ @@ -164,11 +169,8 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, cipher.ciphertext.length = bufsize - 16; cipher.enctype = key->keyblock.enctype; err = krb5_k_encrypt(context, key, key_usage, 0, &plain, &cipher); - zap(plain.data, plain.length); - free(plain.data); - plain.data = 0; if (err) - goto error; + goto cleanup; /* Now that we know we're returning a valid token.... */ ctx->seq_send++; @@ -181,7 +183,6 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, /* If the rotate fails, don't worry about it. */ #endif } else if (toktype == KG_TOK_WRAP_MSG && !conf_req_flag) { - krb5_data plain; size_t cksumsize; /* Here, message is the application-supplied data; message2 is @@ -193,21 +194,19 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, wrap_with_checksum: err = alloc_data(&plain, message->length + 16); if (err) - return err; + goto cleanup; err = krb5_c_checksum_length(context, cksumtype, &cksumsize); if (err) - goto error; + goto cleanup; assert(cksumsize <= 0xffff); bufsize = 16 + message2->length + cksumsize; outbuf = gssalloc_malloc(bufsize); if (outbuf == NULL) { - free(plain.data); - plain.data = 0; err = ENOMEM; - goto error; + goto cleanup; } /* TOK_ID */ @@ -239,23 +238,15 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, if (message2->length) memcpy(outbuf + 16, message2->value, message2->length); - sum.contents = outbuf + 16 + message2->length; - sum.length = cksumsize; - err = krb5_k_make_checksum(context, cksumtype, key, key_usage, &plain, &sum); - zap(plain.data, plain.length); - free(plain.data); - plain.data = 0; if (err) { zap(outbuf,bufsize); - goto error; + goto cleanup; } if (sum.length != cksumsize) abort(); memcpy(outbuf + 16 + message2->length, sum.contents, cksumsize); - krb5_free_checksum_contents(context, &sum); - sum.contents = 0; /* Now that we know we're actually generating the token... */ ctx->seq_send++; @@ -285,12 +276,13 @@ gss_krb5int_make_seal_token_v3 (krb5_context context, token->value = outbuf; token->length = bufsize; - return 0; + outbuf = NULL; + err = 0; -error: +cleanup: + krb5_free_checksum_contents(context, &sum); + zapfree(plain.data, plain.length); gssalloc_free(outbuf); - token->value = NULL; - token->length = 0; return err; } diff --git a/src/lib/rpc/pmap_rmt.c b/src/lib/rpc/pmap_rmt.c index 8c7e30c21a..0748af34a7 100644 --- a/src/lib/rpc/pmap_rmt.c +++ b/src/lib/rpc/pmap_rmt.c @@ -160,11 +160,12 @@ xdr_rmtcallres( caddr_t port_ptr; port_ptr = (caddr_t)(void *)crp->port_ptr; - if (xdr_reference(xdrs, &port_ptr, sizeof (uint32_t), - xdr_u_int32) && xdr_u_int32(xdrs, &crp->resultslen)) { - crp->port_ptr = (uint32_t *)(void *)port_ptr; + if (!xdr_reference(xdrs, &port_ptr, sizeof (uint32_t), + (xdrproc_t)xdr_u_int32)) + return (FALSE); + crp->port_ptr = (uint32_t *)(void *)port_ptr; + if (xdr_u_int32(xdrs, &crp->resultslen)) return ((*(crp->xdr_results))(xdrs, crp->results_ptr)); - } return (FALSE); } -- 2.44.0