krb5 commit: Improve PKINIT client memory management
Greg Hudson
ghudson at MIT.EDU
Tue Mar 18 13:14:00 EDT 2014
https://github.com/krb5/krb5/commit/3c14324baffdc1848f75924deaf69e43f30e6621
commit 3c14324baffdc1848f75924deaf69e43f30e6621
Author: Greg Hudson <ghudson at mit.edu>
Date: Fri Mar 14 12:53:50 2014 -0400
Improve PKINIT client memory management
In pkinit_as_req_create, create and encode stack-allocated auth-pack
structures containing only alias pointers, instead of heap-allocated
structures containing a mix of alias pointers, owner pointers, and
appropriated caller memory. Keep everything we temporarily allocate
in separate local variables and free them through those variables.
In pa_pkinit_gen_req, use safer memory practices to avoid problems
like issue #7878. Free the checksum since pkinit_as_req_create no
longer takes ownership it. Remove a broken overly defensive check
after calling pkinit_as_req_create.
Remove init_krb5_auth_pack and init_krb5_auth_pack_draft9 as they are
no longer required.
src/plugins/preauth/pkinit/pkinit.h | 2 -
src/plugins/preauth/pkinit/pkinit_clnt.c | 134 ++++++++++++------------------
src/plugins/preauth/pkinit/pkinit_lib.c | 21 -----
3 files changed, 54 insertions(+), 103 deletions(-)
diff --git a/src/plugins/preauth/pkinit/pkinit.h b/src/plugins/preauth/pkinit/pkinit.h
index f9e1485..ee1334b 100644
--- a/src/plugins/preauth/pkinit/pkinit.h
+++ b/src/plugins/preauth/pkinit/pkinit.h
@@ -340,8 +340,6 @@ void init_krb5_pa_pk_as_req_draft9(krb5_pa_pk_as_req_draft9 **in);
void init_krb5_reply_key_pack(krb5_reply_key_pack **in);
void init_krb5_reply_key_pack_draft9(krb5_reply_key_pack_draft9 **in);
-void init_krb5_auth_pack(krb5_auth_pack **in);
-void init_krb5_auth_pack_draft9(krb5_auth_pack_draft9 **in);
void init_krb5_pa_pk_as_rep(krb5_pa_pk_as_rep **in);
void init_krb5_pa_pk_as_rep_draft9(krb5_pa_pk_as_rep_draft9 **in);
void init_krb5_subject_pk_info(krb5_subject_pk_info **in);
diff --git a/src/plugins/preauth/pkinit/pkinit_clnt.c b/src/plugins/preauth/pkinit/pkinit_clnt.c
index cfef5b9..2a00370 100644
--- a/src/plugins/preauth/pkinit/pkinit_clnt.c
+++ b/src/plugins/preauth/pkinit/pkinit_clnt.c
@@ -39,6 +39,7 @@
#include <dlfcn.h>
#include <sys/stat.h>
+#include "k5-int.h"
#include "pkinit.h"
#include "k5-json.h"
@@ -110,7 +111,7 @@ pa_pkinit_gen_req(krb5_context context,
krb5_data *der_req = NULL;
krb5_pa_data **return_pa_data = NULL;
- cksum.contents = NULL;
+ memset(&cksum, 0, sizeof(cksum));
reqctx->pa_type = pa_type;
pkiDebug("kdc_options = 0x%x till = %d\n",
@@ -158,31 +159,24 @@ pa_pkinit_gen_req(krb5_context context,
retval = pkinit_as_req_create(context, plgctx, reqctx, ctsec, cusec,
nonce, &cksum, request->client, request->server, &out_data);
- if (retval || !out_data->length) {
+ if (retval) {
pkiDebug("error %d on pkinit_as_req_create; aborting PKINIT\n",
(int) retval);
goto cleanup;
}
- retval = ENOMEM;
+
/*
* The most we'll return is two pa_data, normally just one.
* We need to make room for the NULL terminator.
*/
- return_pa_data = malloc(3 * sizeof(krb5_pa_data *));
+ return_pa_data = k5calloc(3, sizeof(*return_pa_data), &retval);
if (return_pa_data == NULL)
goto cleanup;
- return_pa_data[1] = NULL; /* in case of an early trip to cleanup */
- return_pa_data[2] = NULL; /* Terminate the list */
-
- return_pa_data[0] = malloc(sizeof(krb5_pa_data));
+ return_pa_data[0] = k5alloc(sizeof(*return_pa_data[0]), &retval);
if (return_pa_data[0] == NULL)
goto cleanup;
- return_pa_data[1] = malloc(sizeof(krb5_pa_data));
- if (return_pa_data[1] == NULL)
- goto cleanup;
-
return_pa_data[0]->magic = KV5M_PA_DATA;
if (pa_type == KRB5_PADATA_PK_AS_REQ_OLD)
@@ -191,6 +185,7 @@ pa_pkinit_gen_req(krb5_context context,
return_pa_data[0]->pa_type = pa_type;
return_pa_data[0]->length = out_data->length;
return_pa_data[0]->contents = (krb5_octet *) out_data->data;
+ *out_data = empty_data();
/*
* LH Beta 3 requires the extra pa-data, even for RFC requests,
@@ -199,31 +194,20 @@ pa_pkinit_gen_req(krb5_context context,
*/
if ((return_pa_data[0]->pa_type == KRB5_PADATA_PK_AS_REP_OLD
&& reqctx->opts->win2k_require_cksum) || (longhorn == 1)) {
+ return_pa_data[1] = k5alloc(sizeof(*return_pa_data[1]), &retval);
+ if (return_pa_data[1] == NULL)
+ goto cleanup;
return_pa_data[1]->pa_type = KRB5_PADATA_AS_CHECKSUM;
- return_pa_data[1]->length = 0;
- return_pa_data[1]->contents = NULL;
- } else {
- free(return_pa_data[1]);
- return_pa_data[1] = NULL; /* Move the list terminator */
}
+
*out_padata = return_pa_data;
- retval = 0;
+ return_pa_data = NULL;
cleanup:
- if (der_req != NULL)
- krb5_free_data(context, der_req);
-
- if (retval) {
- if (return_pa_data) {
- free(return_pa_data[0]);
- free(return_pa_data[1]);
- free(return_pa_data);
- }
- if (out_data) {
- free(out_data->data);
- }
- }
- free(out_data);
+ krb5_free_data(context, der_req);
+ krb5_free_checksum_contents(context, &cksum);
+ krb5_free_data(context, out_data);
+ krb5_free_pa_data(context, return_pa_data);
return retval;
}
@@ -240,13 +224,16 @@ pkinit_as_req_create(krb5_context context,
krb5_data ** as_req)
{
krb5_error_code retval = ENOMEM;
- krb5_subject_pk_info *info = NULL;
+ krb5_subject_pk_info info;
krb5_data *coded_auth_pack = NULL;
- krb5_auth_pack *auth_pack = NULL;
+ krb5_auth_pack auth_pack;
krb5_pa_pk_as_req *req = NULL;
- krb5_auth_pack_draft9 *auth_pack9 = NULL;
+ krb5_auth_pack_draft9 auth_pack9;
krb5_pa_pk_as_req_draft9 *req9 = NULL;
+ krb5_algorithm_identifier **cmstypes = NULL;
int protocol = reqctx->opts->dh_or_rsa;
+ unsigned char *dh_params = NULL, *dh_pubkey = NULL;
+ unsigned int dh_params_len, dh_pubkey_len;
pkiDebug("pkinit_as_req_create pa_type = %d\n", reqctx->pa_type);
@@ -254,34 +241,28 @@ pkinit_as_req_create(krb5_context context,
switch((int)reqctx->pa_type) {
case KRB5_PADATA_PK_AS_REQ_OLD:
protocol = RSA_PROTOCOL;
- init_krb5_auth_pack_draft9(&auth_pack9);
- if (auth_pack9 == NULL)
- goto cleanup;
- auth_pack9->pkAuthenticator.ctime = ctsec;
- auth_pack9->pkAuthenticator.cusec = cusec;
- auth_pack9->pkAuthenticator.nonce = nonce;
- auth_pack9->pkAuthenticator.kdcName = server;
- free(cksum->contents);
+ memset(&auth_pack9, 0, sizeof(auth_pack9));
+ auth_pack9.pkAuthenticator.ctime = ctsec;
+ auth_pack9.pkAuthenticator.cusec = cusec;
+ auth_pack9.pkAuthenticator.nonce = nonce;
+ auth_pack9.pkAuthenticator.kdcName = server;
break;
case KRB5_PADATA_PK_AS_REQ:
- init_krb5_subject_pk_info(&info);
- if (info == NULL)
- goto cleanup;
- init_krb5_auth_pack(&auth_pack);
- if (auth_pack == NULL)
- goto cleanup;
- auth_pack->pkAuthenticator.ctime = ctsec;
- auth_pack->pkAuthenticator.cusec = cusec;
- auth_pack->pkAuthenticator.nonce = nonce;
- auth_pack->pkAuthenticator.paChecksum = *cksum;
- auth_pack->clientDHNonce.length = 0;
- auth_pack->clientPublicValue = info;
- auth_pack->supportedKDFs = (krb5_data **) supported_kdf_alg_ids;
+ memset(&info, 0, sizeof(info));
+ memset(&auth_pack, 0, sizeof(auth_pack));
+ auth_pack.pkAuthenticator.ctime = ctsec;
+ auth_pack.pkAuthenticator.cusec = cusec;
+ auth_pack.pkAuthenticator.nonce = nonce;
+ auth_pack.pkAuthenticator.paChecksum = *cksum;
+ auth_pack.clientDHNonce.length = 0;
+ auth_pack.clientPublicValue = &info;
+ auth_pack.supportedKDFs = (krb5_data **)supported_kdf_alg_ids;
/* add List of CMS algorithms */
retval = create_krb5_supportedCMSTypes(context, plgctx->cryptoctx,
- reqctx->cryptoctx, reqctx->idctx,
- &auth_pack->supportedCMSTypes);
+ reqctx->cryptoctx,
+ reqctx->idctx, &cmstypes);
+ auth_pack.supportedCMSTypes = cmstypes;
if (retval)
goto cleanup;
break;
@@ -296,35 +277,29 @@ pkinit_as_req_create(krb5_context context,
case DH_PROTOCOL:
TRACE_PKINIT_CLIENT_REQ_DH(context);
pkiDebug("as_req: DH key transport algorithm\n");
- retval = pkinit_copy_krb5_data(&info->algorithm.algorithm, &dh_oid);
- if (retval) {
- pkiDebug("failed to copy dh_oid\n");
- goto cleanup;
- }
+ info.algorithm.algorithm = dh_oid;
/* create client-side DH keys */
- if ((retval = client_create_dh(context, plgctx->cryptoctx,
- reqctx->cryptoctx, reqctx->idctx, reqctx->opts->dh_size,
- (unsigned char **)
- &info->algorithm.parameters.data,
- &info->algorithm.parameters.length,
- (unsigned char **)
- &info->subjectPublicKey.data,
- &info->subjectPublicKey.length)) != 0) {
+ retval = client_create_dh(context, plgctx->cryptoctx,
+ reqctx->cryptoctx, reqctx->idctx,
+ reqctx->opts->dh_size, &dh_params,
+ &dh_params_len, &dh_pubkey, &dh_pubkey_len);
+ if (retval != 0) {
pkiDebug("failed to create dh parameters\n");
goto cleanup;
}
+ info.algorithm.parameters = make_data(dh_params, dh_params_len);
+ info.subjectPublicKey = make_data(dh_pubkey, dh_pubkey_len);
break;
case RSA_PROTOCOL:
TRACE_PKINIT_CLIENT_REQ_RSA(context);
pkiDebug("as_req: RSA key transport algorithm\n");
switch((int)reqctx->pa_type) {
case KRB5_PADATA_PK_AS_REQ_OLD:
- auth_pack9->clientPublicValue = NULL;
+ auth_pack9.clientPublicValue = NULL;
break;
case KRB5_PADATA_PK_AS_REQ:
- free_krb5_subject_pk_info(&info);
- auth_pack->clientPublicValue = NULL;
+ auth_pack.clientPublicValue = NULL;
break;
}
break;
@@ -338,10 +313,10 @@ pkinit_as_req_create(krb5_context context,
/* Encode the authpack */
switch((int)reqctx->pa_type) {
case KRB5_PADATA_PK_AS_REQ:
- retval = k5int_encode_krb5_auth_pack(auth_pack, &coded_auth_pack);
+ retval = k5int_encode_krb5_auth_pack(&auth_pack, &coded_auth_pack);
break;
case KRB5_PADATA_PK_AS_REQ_OLD:
- retval = k5int_encode_krb5_auth_pack_draft9(auth_pack9,
+ retval = k5int_encode_krb5_auth_pack_draft9(&auth_pack9,
&coded_auth_pack);
break;
}
@@ -451,12 +426,11 @@ pkinit_as_req_create(krb5_context context,
#endif
cleanup:
- if (auth_pack != NULL)
- auth_pack->supportedKDFs = NULL; /*alias to global constant*/
- free_krb5_auth_pack(&auth_pack);
+ free_krb5_algorithm_identifiers(&cmstypes);
+ free(dh_params);
+ free(dh_pubkey);
free_krb5_pa_pk_as_req(&req);
free_krb5_pa_pk_as_req_draft9(&req9);
- free(auth_pack9);
pkiDebug("pkinit_as_req_create retval=%d\n", (int) retval);
diff --git a/src/plugins/preauth/pkinit/pkinit_lib.c b/src/plugins/preauth/pkinit/pkinit_lib.c
index f1d8180..a7bf54e 100644
--- a/src/plugins/preauth/pkinit/pkinit_lib.c
+++ b/src/plugins/preauth/pkinit/pkinit_lib.c
@@ -302,27 +302,6 @@ init_krb5_reply_key_pack_draft9(krb5_reply_key_pack_draft9 **in)
}
void
-init_krb5_auth_pack(krb5_auth_pack **in)
-{
- (*in) = malloc(sizeof(krb5_auth_pack));
- if ((*in) == NULL) return;
- (*in)->clientPublicValue = NULL;
- (*in)->supportedCMSTypes = NULL;
- (*in)->clientDHNonce.length = 0;
- (*in)->clientDHNonce.data = NULL;
- (*in)->pkAuthenticator.paChecksum.contents = NULL;
- (*in)->supportedKDFs = NULL;
-}
-
-void
-init_krb5_auth_pack_draft9(krb5_auth_pack_draft9 **in)
-{
- (*in) = malloc(sizeof(krb5_auth_pack_draft9));
- if ((*in) == NULL) return;
- (*in)->clientPublicValue = NULL;
-}
-
-void
init_krb5_pa_pk_as_rep(krb5_pa_pk_as_rep **in)
{
(*in) = malloc(sizeof(krb5_pa_pk_as_rep));
More information about the cvs-krb5
mailing list