From b6be850e0df9dd7e0fa5101c911e5c8b0ec1f807 Mon Sep 17 00:00:00 2001 From: Luke Howard Date: Fri, 24 Dec 2021 10:25:11 +1100 Subject: [PATCH] kdc: remove outpadata from astgs_request_t Remove the outpadata field from astgs_request_t, because it's not something we wish to expose publically (yet it is something that Samba needs in the client_access plugin API, to add Windows error information). Instead, allocate rep->padata at the start of AS/TGS request handling, and ensure it is valid for the lifetime of the request until it is encoded (at which point it will be freed and set to NULL if zero length, to avoid sending a zero length METHOD-DATA to the client). (The previous approach of setting rep->padata to point to &r->outpadata was fragile, because it required clearing the pointer before freeing the KDC-REP.) --- kdc/gss_preauth.c | 4 ++-- kdc/kdc_locl.h | 2 -- kdc/kerberos5.c | 44 ++++++++++++++++++++++++-------------------- kdc/krb5tgs.c | 15 ++++++++------- kdc/pkinit.c | 2 +- 5 files changed, 35 insertions(+), 32 deletions(-) diff --git a/kdc/gss_preauth.c b/kdc/gss_preauth.c index a2c192067..ce62a29af 100644 --- a/kdc/gss_preauth.c +++ b/kdc/gss_preauth.c @@ -816,12 +816,12 @@ _kdc_gss_mk_pa_reply(astgs_request_t r, /* only return padata in error case if we have an error token */ if (!GSS_ERROR(gcp->major) || gcp->output_token.length) { - ret = krb5_padata_add(r->context, &r->outpadata, KRB5_PADATA_GSS, + ret = krb5_padata_add(r->context, r->rep.padata, KRB5_PADATA_GSS, gcp->output_token.value, gcp->output_token.length); if (ret) return ret; - /* token is now owned by outpadata */ + /* token is now owned by r->rep.padata */ gcp->output_token.length = 0; gcp->output_token.value = NULL; } diff --git a/kdc/kdc_locl.h b/kdc/kdc_locl.h index 3032ec0ab..e7b86151d 100644 --- a/kdc/kdc_locl.h +++ b/kdc/kdc_locl.h @@ -70,8 +70,6 @@ struct kdc_patypes; struct astgs_request_desc { ASTGS_REQUEST_DESC_COMMON_ELEMENTS; - METHOD_DATA outpadata; - /* Only AS */ const struct kdc_patypes *pa_used; struct as_request_pa_state *pa_state; diff --git a/kdc/kerberos5.c b/kdc/kerberos5.c index 477488f6e..43566c3e2 100644 --- a/kdc/kerberos5.c +++ b/kdc/kerberos5.c @@ -767,12 +767,12 @@ pa_enc_chal_validate(astgs_request_t r, ret = _krb5_make_pa_enc_challenge(r->context, challengecrypto, KRB5_KU_ENC_CHALLENGE_KDC, - &r->outpadata); + r->rep.padata); krb5_crypto_destroy(r->context, challengecrypto); if (ret) goto out; - set_salt_padata(&r->outpadata, k->salt); + set_salt_padata(r->rep.padata, k->salt); /* * Success @@ -949,7 +949,7 @@ pa_enc_ts_validate(astgs_request_t r, } free_PA_ENC_TS_ENC(&p); - set_salt_padata(&r->outpadata, pa_key->salt); + set_salt_padata(r->rep.padata, pa_key->salt); ret = krb5_copy_keyblock_contents(r->context, &pa_key->key, &r->reply_key); if (ret) @@ -1098,6 +1098,8 @@ _kdc_encode_reply(krb5_context context, EncTicketPart *et = &r->et; EncKDCRepPart *ek = &r->ek; + heim_assert(rep->padata != NULL, "reply padata uninitialized"); + ASN1_MALLOC_ENCODE(EncTicketPart, buf, buf_size, et, &len, ret); if(ret) { const char *msg = krb5_get_error_message(context, ret); @@ -1169,8 +1171,7 @@ _kdc_encode_reply(krb5_context context, if (ret) return ret; - free_METHOD_DATA(&r->outpadata); - rep->padata = &r->outpadata; + free_METHOD_DATA(r->rep.padata); ret = krb5_padata_add(context, rep->padata, KRB5_PADATA_FX_FAST, @@ -1195,6 +1196,12 @@ _kdc_encode_reply(krb5_context context, } } + if (rep->padata->len == 0) { + free_METHOD_DATA(rep->padata); + free(rep->padata); + rep->padata = NULL; + } + if(rep->msg_type == krb_as_rep && !config->encode_as_rep_as_tgs_rep) ASN1_MALLOC_ENCODE(EncASRepPart, buf, buf_size, ek, &len, ret); else @@ -2095,7 +2102,12 @@ _kdc_as_rep(astgs_request_t r) memset(rep, 0, sizeof(*rep)); - rep->padata = &r->outpadata; /* so we don't need to make this public */ + ALLOC(rep->padata); + if (rep->padata == NULL) { + ret = ENOMEM; + krb5_set_error_message(r->context, ret, N_("malloc: out of memory", "")); + goto out; + } /* * Look for FAST armor and unwrap @@ -2179,7 +2191,7 @@ _kdc_as_rep(astgs_request_t r) r->cname, fixed_client_name); free(fixed_client_name); - ret = _kdc_fast_mk_error(r, &r->outpadata, r->armor_crypto, + ret = _kdc_fast_mk_error(r, r->rep.padata, r->armor_crypto, &req->req_body, KRB5_KDC_ERR_WRONG_REALM, NULL, r->client->entry.principal, r->server_princ, @@ -2285,7 +2297,7 @@ _kdc_as_rep(astgs_request_t r) NULL, &ckey, &default_salt); if (ret2 == 0) { ret2 = get_pa_etype_info_both(r->context, config, &b->etype, - &r->outpadata, ckey, !default_salt); + r->rep.padata, ckey, !default_salt); if (ret2 != 0) ret = ret2; } @@ -2331,7 +2343,7 @@ _kdc_as_rep(astgs_request_t r) continue; } - ret = krb5_padata_add(r->context, &r->outpadata, + ret = krb5_padata_add(r->context, r->rep.padata, pat[n].type, NULL, 0); if (ret) goto out; @@ -2345,7 +2357,7 @@ _kdc_as_rep(astgs_request_t r) NULL, &ckey, &default_salt); if (ret == 0) { ret = get_pa_etype_info_both(r->context, config, &b->etype, - &r->outpadata, ckey, !default_salt); + r->rep.padata, ckey, !default_salt); if (ret) goto out; } @@ -2435,7 +2447,6 @@ _kdc_as_rep(astgs_request_t r) /* * Build reply */ - rep->pvno = 5; rep->msg_type = krb_as_rep; @@ -2680,9 +2691,6 @@ _kdc_as_rep(astgs_request_t r) if (ret) goto out; - if (r->outpadata.len == 0) - rep->padata = NULL; - /* Add the PAC */ if (!r->et.flags.anonymous) { generate_pac(r, skey, krbtgt_key, is_tgs); @@ -2761,15 +2769,12 @@ _kdc_as_rep(astgs_request_t r) } out: - r->rep.padata = NULL; /* may point to outpadata */ - free_AS_REP(&r->rep); - /* * In case of a non proxy error, build an error message. */ if (ret != 0 && ret != HDB_ERR_NOT_FOUND_HERE && r->reply->length == 0) ret = _kdc_fast_mk_error(r, - &r->outpadata, + r->rep.padata, r->armor_crypto, &req->req_body, ret, r->e_text, @@ -2781,12 +2786,11 @@ out: if (r->pa_used && r->pa_used->cleanup) r->pa_used->cleanup(r); + free_AS_REP(&r->rep); free_EncTicketPart(&r->et); free_EncKDCRepPart(&r->ek); _kdc_free_fast_state(&r->fast); - if (r->outpadata.len) - free_METHOD_DATA(&r->outpadata); if (r->client_princ) { krb5_free_principal(r->context, r->client_princ); r->client_princ = NULL; diff --git a/kdc/krb5tgs.c b/kdc/krb5tgs.c index 1f115b43e..353004edd 100644 --- a/kdc/krb5tgs.c +++ b/kdc/krb5tgs.c @@ -785,8 +785,6 @@ tgs_make_reply(astgs_request_t r, _kdc_log_timestamp(r, "TGS-REQ", et->authtime, et->starttime, et->endtime, et->renew_till); - rep->padata = r->outpadata.len ? &r->outpadata : NULL; - if (krb5_enctype_valid(r->context, serverkey->keytype) != 0 && _kdc_is_weak_exception(server->entry.principal, serverkey->keytype)) { @@ -2389,7 +2387,7 @@ server_lookup: } pa.padata_type = KRB5_PADATA_SERVER_REFERRAL; - ret = add_METHOD_DATA(&priv->outpadata, &pa); + ret = add_METHOD_DATA(priv->rep.padata, &pa); krb5_data_free(&pa.padata_value); if (ret) { kdc_log(context, config, 4, @@ -2554,6 +2552,13 @@ _kdc_tgs_rep(astgs_request_t r) if (ret) goto out; + ALLOC(r->rep.padata); + if (r->rep.padata == NULL) { + ret = ENOMEM; + krb5_set_error_message(r->context, ret, N_("malloc: out of memory", "")); + goto out; + } + ret = tgs_build_reply(r, krbtgt, krbtgt_etype, @@ -2593,7 +2598,6 @@ out: free(csec); free(cusec); - r->rep.padata = NULL; /* may point to outpadata */ free_TGS_REP(&r->rep); free_TransitedEncoding(&r->et.transited); free(r->et.starttime); @@ -2632,9 +2636,6 @@ out: _kdc_free_fast_state(&r->fast); krb5_pac_free(r->context, r->pac); - if (r->outpadata.len) - free_METHOD_DATA(&r->outpadata); - if (auth_data) { free_AuthorizationData(auth_data); free(auth_data); diff --git a/kdc/pkinit.c b/kdc/pkinit.c index 6b5aa8d95..b355e4c88 100644 --- a/kdc/pkinit.c +++ b/kdc/pkinit.c @@ -1137,7 +1137,7 @@ _kdc_pk_mk_pa_reply(astgs_request_t r, pk_client_params *cp) const krb5_data *req_buffer = &r->request; krb5_keyblock *reply_key = &r->reply_key; krb5_keyblock *sessionkey = &r->session_key; - METHOD_DATA *md = &r->outpadata; + METHOD_DATA *md = r->rep.padata; krb5_error_code ret; void *buf = NULL; size_t len = 0, size = 0;