From fb3ea5b9437eb1e2df6e80c6b26d6982c95d2409 Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Thu, 23 Dec 2021 23:06:59 -0600 Subject: [PATCH] kdc: Add ret to common svc req elements We're logging SUCCESS even when the KDC sends error replies. That's because we're returning success to process_request() even when we send errors to clients. The error we want to send to the client, and that we succeed or fail to send it, are different statuses. Also, further move things into `r` and out of function arguments. --- kdc/bx509d.c | 1 - kdc/fast.c | 7 +++---- kdc/httpkadmind.c | 1 - kdc/kerberos5.c | 27 +++++++++++++++++++++++---- kdc/krb5tgs.c | 18 +++++++----------- kdc/process.c | 2 +- 6 files changed, 34 insertions(+), 22 deletions(-) diff --git a/kdc/bx509d.c b/kdc/bx509d.c index 5f3fbeb42..2f30744bf 100644 --- a/kdc/bx509d.c +++ b/kdc/bx509d.c @@ -126,7 +126,6 @@ typedef struct bx509_request_desc { char *ccname; char *freeme1; krb5_addresses tgt_addresses; /* For /get-tgt */ - krb5_error_code ret; char frombuf[128]; } *bx509_request_desc; diff --git a/kdc/fast.c b/kdc/fast.c index b5ed614dd..196965573 100644 --- a/kdc/fast.c +++ b/kdc/fast.c @@ -328,7 +328,6 @@ _kdc_fast_mk_error(astgs_request_t r, krb5_crypto armor_crypto, const KDC_REQ_BODY *req_body, krb5_error_code outer_error, - const char *e_text, krb5_principal error_client, krb5_principal error_server, time_t *csec, int *cusec, @@ -369,7 +368,7 @@ _kdc_fast_mk_error(astgs_request_t r, ret = krb5_mk_error(r->context, outer_error, - e_text, + r->e_text, NULL, error_client, error_server, @@ -393,7 +392,7 @@ _kdc_fast_mk_error(astgs_request_t r, } outer_error = KRB5_KDC_ERR_MORE_PREAUTH_DATA_REQUIRED; - e_text = NULL; + r->e_text = NULL; if (r->fast.flags.requested_hidden_names) { error_client = NULL; error_server = NULL; @@ -435,7 +434,7 @@ _kdc_fast_mk_error(astgs_request_t r, ret = krb5_mk_error(r->context, outer_error, - e_text, + r->e_text, (e_data.length ? &e_data : NULL), error_client, error_server, diff --git a/kdc/httpkadmind.c b/kdc/httpkadmind.c index fb68f0fd2..3dfb36518 100644 --- a/kdc/httpkadmind.c +++ b/kdc/httpkadmind.c @@ -119,7 +119,6 @@ typedef struct kadmin_request_desc { HEIM_SVC_REQUEST_DESC_COMMON_ELEMENTS; struct MHD_Connection *connection; - krb5_error_code ret; krb5_times token_times; /* * FIXME diff --git a/kdc/kerberos5.c b/kdc/kerberos5.c index 3a67db425..ce1138405 100644 --- a/kdc/kerberos5.c +++ b/kdc/kerberos5.c @@ -418,6 +418,21 @@ _kdc_r_log(astgs_request_t r, int level, const char *fmt, ...) va_end(ap); } +void +_kdc_set_const_e_text(astgs_request_t r, const char *e_text) +{ + /* We should never see this */ + if (r->e_text) { + kdc_log(r->context, r->config, 1, + "trying to replace e-text \"%s\" with \"%s\"\n", + r->e_text, e_text); + return; + } + + r->e_text = e_text; + kdc_log(r->context, r->config, 4, "%s", e_text); +} + void _kdc_set_e_text(astgs_request_t r, const char *fmt, ...) __attribute__ ((__format__ (__printf__, 2, 3))) @@ -430,9 +445,12 @@ _kdc_set_e_text(astgs_request_t r, const char *fmt, ...) vasprintf_ret = vasprintf(&e_text, fmt, ap); va_end(ap); - if (vasprintf_ret < 0 || !e_text) + if (vasprintf_ret < 0 || !e_text) { /* not much else to do... */ + kdc_log(r->context, r->config, 1, + "Could not set e_text: %s (out of memory)", fmt); return; + } /* We should never see this */ if (r->e_text) { @@ -2190,9 +2208,10 @@ _kdc_as_rep(astgs_request_t r) r->cname, fixed_client_name); free(fixed_client_name); + r->e_text = NULL; ret = _kdc_fast_mk_error(r, r->rep.padata, r->armor_crypto, - &req->req_body, KRB5_KDC_ERR_WRONG_REALM, - NULL, + &req->req_body, + r->ret = KRB5_KDC_ERR_WRONG_REALM, r->client->entry.principal, r->server_princ, NULL, NULL, r->reply); goto out; @@ -2776,7 +2795,7 @@ out: r->rep.padata, r->armor_crypto, &req->req_body, - ret, r->e_text, + r->ret = ret, r->client_princ, r->server_princ, NULL, NULL, diff --git a/kdc/krb5tgs.c b/kdc/krb5tgs.c index b9d840e19..1f02a7e64 100644 --- a/kdc/krb5tgs.c +++ b/kdc/krb5tgs.c @@ -859,7 +859,6 @@ tgs_check_authenticator(krb5_context context, krb5_kdc_configuration *config, krb5_auth_context ac, KDC_REQ_BODY *b, - const char **e_text, krb5_keyblock *key) { krb5_authenticator auth; @@ -982,7 +981,6 @@ tgs_parse_request(astgs_request_t r, hdb_entry_ex **krbtgt, krb5_enctype *krbtgt_etype, krb5_ticket **ticket, - const char **e_text, const char *from, const struct sockaddr *from_addr, time_t **csec, @@ -1183,8 +1181,8 @@ next_kvno: } } - ret = tgs_check_authenticator(r->context, config, - ac, b, e_text, &(*ticket)->ticket.key); + ret = tgs_check_authenticator(r->context, config, ac, b, + &(*ticket)->ticket.key); if (ret) { krb5_auth_con_free(r->context, ac); goto out; @@ -1426,7 +1424,6 @@ tgs_build_reply(astgs_request_t priv, hdb_entry_ex *krbtgt, krb5_enctype krbtgt_etype, krb5_ticket *ticket, - const char **e_text, AuthorizationData **auth_data, const struct sockaddr *from_addr) { @@ -1490,7 +1487,7 @@ tgs_build_reply(astgs_request_t priv, if (s == NULL) { ret = KRB5KDC_ERR_S_PRINCIPAL_UNKNOWN; - _kdc_set_e_text(priv, "No server in request"); + _kdc_set_const_e_text(priv, "No server in request"); goto out; } @@ -2498,12 +2495,13 @@ _kdc_tgs_rep(astgs_request_t r) hdb_entry_ex *krbtgt = NULL; krb5_ticket *ticket = NULL; - const char *e_text = NULL; krb5_enctype krbtgt_etype = ETYPE_NULL; time_t *csec = NULL; int *cusec = NULL; + r->e_text = NULL; + if(req->padata == NULL){ ret = KRB5KDC_ERR_PREAUTH_REQUIRED; /* XXX ??? */ kdc_log(r->context, config, 4, @@ -2532,7 +2530,6 @@ _kdc_tgs_rep(astgs_request_t r) &krbtgt, &krbtgt_etype, &ticket, - &e_text, from, from_addr, &csec, &cusec, &auth_data); @@ -2561,7 +2558,6 @@ _kdc_tgs_rep(astgs_request_t r) krbtgt, krbtgt_etype, ticket, - &e_text, &auth_data, from_addr); if (ret) { @@ -2574,7 +2570,7 @@ _kdc_tgs_rep(astgs_request_t r) if (datagram_reply && data->length > config->max_datagram_reply_length) { krb5_data_free(data); ret = KRB5KRB_ERR_RESPONSE_TOO_BIG; - e_text = "Reply packet too large"; + _kdc_set_const_e_text(r, "Reply packet too large"); } out: @@ -2586,7 +2582,7 @@ out: &error_method, r->armor_crypto, &req->req_body, - ret, r->e_text, + r->ret = ret, ticket != NULL ? ticket->client : NULL, ticket != NULL ? ticket->server : NULL, csec, cusec, diff --git a/kdc/process.c b/kdc/process.c index 56d40250c..23a0fe010 100644 --- a/kdc/process.c +++ b/kdc/process.c @@ -125,7 +125,7 @@ _kdc_audit_trail(kdc_request_t r, krb5_error_code ret) /* Get a symbolic name for some error codes */ #define CASE(x) case x : retname = #x; break - switch (ret) { + switch (ret ? ret : r->ret) { CASE(ENOMEM); CASE(EACCES); CASE(HDB_ERR_NOT_FOUND_HERE);