From 37202c0ac5db1166d17917fd6619236c9c8af673 Mon Sep 17 00:00:00 2001 From: Greg Hudson Date: Sun, 17 Nov 2024 13:54:12 -0500 Subject: [PATCH] Fix minor logic errors In k5_externalize_auth_context(), serialize the correct field when remote_port is set. This is not a reachable bug because the function is only accessible via gss_export_sec_context(), and the GSS library does not set a remote port. In generic_gss_oid_to_str(), remove an inconsistently-applied test for a null minor_status. Also remove minor_status null checks from generic_gss_release_oid() and generic_gss_str_to_oid(), but add output initializations and pointer checks to the API functions in g_oid_ops.c in a similar manner to other GSSAPI functions. Remove gssint_copy_oid_set() and replace its one call with a call to generic_gss_copy_oid_set(). In the checksum functions, avoid crashing if the caller passes a null key and checksum type 0. An error will be returned instead when find_cksumtype() can't find the checksum type. (krb5_k_verify_checksum() already had this check.) In pkinit_open_session(), remove an unnecessary null check for ctx->p11_module_name, and add a check for p11name being null due to an asprintf() failure. In profile_add_node(), add a check for null ret_node in the duplicate subsection check. This is not a reachable bug becaues the function is currently never called with null ret_node and null value. In ksu's main(), check for krb5_cc_default_name() returning NULL (which only happens on allocation failure). Also clean up some vestiges left behind by commit 9ebae7cb434b9b177c0af85c67a6d6267f46bc68. In the KDC and kpropd write_pid_file(), avoid briefly leaking the file handle on write failure. In get_authorized_princ_names(), close login_fp if we fail to open k5users_path. Reported by Valery Fedorenko. --- src/clients/ksu/heuristic.c | 5 +- src/clients/ksu/main.c | 30 +++------- src/kadmin/server/ovsec_kadmd.c | 26 +++------ src/kdc/main.c | 9 +-- src/kprop/kpropd.c | 7 ++- src/lib/crypto/krb/make_checksum.c | 2 +- src/lib/crypto/krb/make_checksum_iov.c | 2 +- src/lib/crypto/krb/verify_checksum_iov.c | 2 +- src/lib/gssapi/generic/oid_ops.c | 9 +-- src/lib/gssapi/mechglue/g_oid_ops.c | 58 +++++++++++++++---- src/lib/gssapi/mechglue/mglueP.h | 6 -- src/lib/gssapi/spnego/spnego_mech.c | 2 +- src/lib/krb5/krb/ser_actx.c | 2 +- .../preauth/pkinit/pkinit_crypto_openssl.c | 30 +++++----- src/util/profile/prof_tree.c | 3 +- 15 files changed, 104 insertions(+), 89 deletions(-) diff --git a/src/clients/ksu/heuristic.c b/src/clients/ksu/heuristic.c index 6ed94eb887c..fcc1ddf8a76 100644 --- a/src/clients/ksu/heuristic.c +++ b/src/clients/ksu/heuristic.c @@ -227,8 +227,11 @@ get_authorized_princ_names(const char *luser, char *cmd, char ***princ_list) } } if (!k5users_flag){ - if ((users_fp = fopen(k5users_path, "r")) == NULL) + users_fp = fopen(k5users_path, "r"); + if (users_fp == NULL) { + close_time(1, NULL, k5login_flag, login_fp); return 0; + } if ( fowner(users_fp, pwd->pw_uid) == FALSE){ close_time(k5users_flag,users_fp, k5login_flag,login_fp); diff --git a/src/clients/ksu/main.c b/src/clients/ksu/main.c index debad94861a..ca3981ea753 100644 --- a/src/clients/ksu/main.c +++ b/src/clients/ksu/main.c @@ -101,7 +101,6 @@ main(int argc, char ** argv) krb5_ccache cc_source = NULL; const char * cc_source_tag = NULL; - const char * cc_source_tag_tmp = NULL; char * cmd = NULL, * exec_cmd = NULL; int errflg = 0; krb5_boolean auth_val; @@ -274,23 +273,13 @@ main(int argc, char ** argv) case 'c': if (cc_source_tag == NULL) { cc_source_tag = xstrdup(optarg); - if ( strchr(cc_source_tag, ':')){ - cc_source_tag_tmp = strchr(cc_source_tag, ':') + 1; - - if (!ks_ccache_name_is_initialized(ksu_context, - cc_source_tag)) { - com_err(prog_name, errno, - _("while looking for credentials cache %s"), - cc_source_tag_tmp); - exit (1); - } - } - else { - fprintf(stderr, _("malformed credential cache name %s\n"), + if (!ks_ccache_name_is_initialized(ksu_context, + cc_source_tag)) { + com_err(prog_name, errno, + _("while looking for credentials cache %s"), cc_source_tag); - errflg++; + exit(1); } - } else { fprintf(stderr, _("Only one -c option allowed\n")); errflg++; @@ -374,11 +363,10 @@ main(int argc, char ** argv) if (cc_source_tag == NULL){ cc_source_tag = krb5_cc_default_name(ksu_context); - cc_source_tag_tmp = strchr(cc_source_tag, ':'); - if (cc_source_tag_tmp == 0) - cc_source_tag_tmp = cc_source_tag; - else - cc_source_tag_tmp++; + if (cc_source_tag == NULL) { + fprintf(stderr, _("ksu: failed to get default ccache name\n")); + exit(1); + } } /* get a handle for the cache */ diff --git a/src/kadmin/server/ovsec_kadmd.c b/src/kadmin/server/ovsec_kadmd.c index 5450bae80b8..6e080e197d5 100644 --- a/src/kadmin/server/ovsec_kadmd.c +++ b/src/kadmin/server/ovsec_kadmd.c @@ -235,7 +235,7 @@ log_badverf(gss_name_t client_name, gss_name_t server_name, OM_uint32 minor; gss_buffer_desc client, server; gss_OID gss_type; - const char *a; + const char *a, *cname, *sname; rpcproc_t proc; unsigned int i; const char *procname; @@ -249,19 +249,11 @@ log_badverf(gss_name_t client_name, gss_name_t server_name, (void)gss_display_name(&minor, client_name, &client, &gss_type); (void)gss_display_name(&minor, server_name, &server, &gss_type); - if (client.value == NULL) { - client.value = "(null)"; - clen = sizeof("(null)") - 1; - } else { - clen = client.length; - } + cname = (client.value == NULL) ? "(null)" : client.value; + clen = (client.value == NULL) ? sizeof("(null)") - 1 : client.length; trunc_name(&clen, &cdots); - if (server.value == NULL) { - server.value = "(null)"; - slen = sizeof("(null)") - 1; - } else { - slen = server.length; - } + sname = (server.value == NULL) ? "(null)" : server.value; + slen = (server.value == NULL) ? sizeof("(null)") - 1 : server.length; trunc_name(&slen, &sdots); a = client_addr(rqst->rq_xprt); @@ -277,14 +269,14 @@ log_badverf(gss_name_t client_name, gss_name_t server_name, krb5_klog_syslog(LOG_NOTICE, _("WARNING! Forged/garbled request: %s, claimed " "client = %.*s%s, server = %.*s%s, addr = %s"), - procname, (int)clen, (char *)client.value, cdots, - (int)slen, (char *)server.value, sdots, a); + procname, (int)clen, cname, cdots, (int)slen, sname, + sdots, a); } else { krb5_klog_syslog(LOG_NOTICE, _("WARNING! Forged/garbled request: %d, claimed " "client = %.*s%s, server = %.*s%s, addr = %s"), - proc, (int)clen, (char *)client.value, cdots, - (int)slen, (char *)server.value, sdots, a); + proc, (int)clen, cname, cdots, (int)slen, sname, + sdots, a); } (void)gss_release_buffer(&minor, &client); diff --git a/src/kdc/main.c b/src/kdc/main.c index c5a66ddde80..5571451df06 100644 --- a/src/kdc/main.c +++ b/src/kdc/main.c @@ -842,14 +842,15 @@ write_pid_file(const char *path) { FILE *file; unsigned long pid; + int st1, st2; file = fopen(path, "w"); if (file == NULL) return errno; - pid = (unsigned long) getpid(); - if (fprintf(file, "%ld\n", pid) < 0 || fclose(file) == EOF) - return errno; - return 0; + pid = (unsigned long)getpid(); + st1 = (fprintf(file, "%ld\n", pid) < 0) ? errno : 0; + st2 = (fclose(file) == EOF) ? errno : 0; + return st1 ? st1 : st2; } static void diff --git a/src/kprop/kpropd.c b/src/kprop/kpropd.c index 64afd3946bb..be48062ff57 100644 --- a/src/kprop/kpropd.c +++ b/src/kprop/kpropd.c @@ -181,14 +181,15 @@ write_pid_file(const char *path) { FILE *fp; unsigned long pid; + int st1, st2; fp = fopen(path, "w"); if (fp == NULL) return errno; pid = (unsigned long)getpid(); - if (fprintf(fp, "%ld\n", pid) < 0 || fclose(fp) == EOF) - return errno; - return 0; + st1 = (fprintf(fp, "%ld\n", pid) < 0) ? errno : 0; + st2 = (fclose(fp) == EOF) ? errno : 0; + return st1 ? st1 : st2; } typedef void (*sig_handler_fn)(int sig); diff --git a/src/lib/crypto/krb/make_checksum.c b/src/lib/crypto/krb/make_checksum.c index 398c84a8dbb..3c57e4173b7 100644 --- a/src/lib/crypto/krb/make_checksum.c +++ b/src/lib/crypto/krb/make_checksum.c @@ -40,7 +40,7 @@ krb5_k_make_checksum(krb5_context context, krb5_cksumtype cksumtype, krb5_octet *trunc; krb5_error_code ret; - if (cksumtype == 0) { + if (cksumtype == 0 && key != NULL) { ret = krb5int_c_mandatory_cksumtype(context, key->keyblock.enctype, &cksumtype); if (ret != 0) diff --git a/src/lib/crypto/krb/make_checksum_iov.c b/src/lib/crypto/krb/make_checksum_iov.c index 84e98b141b9..c9e9da87f57 100644 --- a/src/lib/crypto/krb/make_checksum_iov.c +++ b/src/lib/crypto/krb/make_checksum_iov.c @@ -39,7 +39,7 @@ krb5_k_make_checksum_iov(krb5_context context, krb5_crypto_iov *checksum; const struct krb5_cksumtypes *ctp; - if (cksumtype == 0) { + if (cksumtype == 0 && key != NULL) { ret = krb5int_c_mandatory_cksumtype(context, key->keyblock.enctype, &cksumtype); if (ret != 0) diff --git a/src/lib/crypto/krb/verify_checksum_iov.c b/src/lib/crypto/krb/verify_checksum_iov.c index 47a25a93b4e..532e45cd9df 100644 --- a/src/lib/crypto/krb/verify_checksum_iov.c +++ b/src/lib/crypto/krb/verify_checksum_iov.c @@ -40,7 +40,7 @@ krb5_k_verify_checksum_iov(krb5_context context, krb5_data computed; krb5_crypto_iov *checksum; - if (checksum_type == 0) { + if (checksum_type == 0 && key != NULL) { ret = krb5int_c_mandatory_cksumtype(context, key->keyblock.enctype, &checksum_type); if (ret != 0) diff --git a/src/lib/gssapi/generic/oid_ops.c b/src/lib/gssapi/generic/oid_ops.c index 253d64694dd..0d65a95fcf0 100644 --- a/src/lib/gssapi/generic/oid_ops.c +++ b/src/lib/gssapi/generic/oid_ops.c @@ -68,8 +68,7 @@ OM_uint32 generic_gss_release_oid(OM_uint32 *minor_status, gss_OID *oid) { - if (minor_status) - *minor_status = 0; + *minor_status = 0; if (oid == NULL || *oid == GSS_C_NO_OID) return(GSS_S_COMPLETE); @@ -245,8 +244,7 @@ generic_gss_oid_to_str(OM_uint32 *minor_status, unsigned char *cp; struct k5buf buf; - if (minor_status != NULL) - *minor_status = 0; + *minor_status = 0; if (oid_str != GSS_C_NO_BUFFER) { oid_str->length = 0; @@ -353,8 +351,7 @@ generic_gss_str_to_oid(OM_uint32 *minor_status, int brace = 0; gss_OID oid; - if (minor_status != NULL) - *minor_status = 0; + *minor_status = 0; if (oid_out != NULL) *oid_out = GSS_C_NO_OID; diff --git a/src/lib/gssapi/mechglue/g_oid_ops.c b/src/lib/gssapi/mechglue/g_oid_ops.c index f29fb3b33e0..fa87d809565 100644 --- a/src/lib/gssapi/mechglue/g_oid_ops.c +++ b/src/lib/gssapi/mechglue/g_oid_ops.c @@ -36,6 +36,13 @@ OM_uint32 KRB5_CALLCONV gss_create_empty_oid_set(OM_uint32 *minor_status, gss_OID_set *oid_set) { OM_uint32 status; + + if (minor_status != NULL) + *minor_status = 0; + if (oid_set != NULL) + *oid_set = GSS_C_NO_OID_SET; + if (minor_status == NULL || oid_set == NULL) + return GSS_S_CALL_INACCESSIBLE_WRITE; status = generic_gss_create_empty_oid_set(minor_status, oid_set); if (status != GSS_S_COMPLETE) map_errcode(minor_status); @@ -47,6 +54,14 @@ gss_add_oid_set_member(OM_uint32 *minor_status, gss_OID member_oid, gss_OID_set *oid_set) { OM_uint32 status; + + if (minor_status != NULL) + *minor_status = 0; + if (minor_status == NULL || oid_set == NULL) + return GSS_S_CALL_INACCESSIBLE_WRITE; + if (member_oid == GSS_C_NO_OID || member_oid->length == 0 || + member_oid->elements == NULL) + return GSS_S_CALL_INACCESSIBLE_READ; status = generic_gss_add_oid_set_member(minor_status, member_oid, oid_set); if (status != GSS_S_COMPLETE) map_errcode(minor_status); @@ -57,13 +72,33 @@ OM_uint32 KRB5_CALLCONV gss_test_oid_set_member(OM_uint32 *minor_status, gss_OID member, gss_OID_set set, int *present) { + if (minor_status != NULL) + *minor_status = 0; + if (present != NULL) + *present = 0; + if (minor_status == NULL || present == NULL) + return GSS_S_CALL_INACCESSIBLE_WRITE; + if (member == GSS_C_NO_OID || set == GSS_C_NO_OID_SET) + return GSS_S_CALL_INACCESSIBLE_READ; return generic_gss_test_oid_set_member(minor_status, member, set, present); } OM_uint32 KRB5_CALLCONV gss_oid_to_str(OM_uint32 *minor_status, gss_OID oid, gss_buffer_t oid_str) { - OM_uint32 status = generic_gss_oid_to_str(minor_status, oid, oid_str); + OM_uint32 status; + + if (minor_status != NULL) + *minor_status = 0; + if (oid_str != GSS_C_NO_BUFFER) { + oid_str->length = 0; + oid_str->value = NULL; + } + if (minor_status == NULL || oid_str == GSS_C_NO_BUFFER) + return GSS_S_CALL_INACCESSIBLE_WRITE; + if (oid == GSS_C_NO_OID || oid->length == 0 || oid->elements == NULL) + return GSS_S_CALL_INACCESSIBLE_READ; + status = generic_gss_oid_to_str(minor_status, oid, oid_str); if (status != GSS_S_COMPLETE) map_errcode(minor_status); return status; @@ -72,21 +107,22 @@ gss_oid_to_str(OM_uint32 *minor_status, gss_OID oid, gss_buffer_t oid_str) OM_uint32 KRB5_CALLCONV gss_str_to_oid(OM_uint32 *minor_status, gss_buffer_t oid_str, gss_OID *oid) { - OM_uint32 status = generic_gss_str_to_oid(minor_status, oid_str, oid); + OM_uint32 status; + + if (minor_status != NULL) + *minor_status = 0; + if (oid != NULL) + *oid = GSS_C_NO_OID; + if (minor_status == NULL || oid == NULL) + return GSS_S_CALL_INACCESSIBLE_WRITE; + if (GSS_EMPTY_BUFFER(oid_str)) + return GSS_S_CALL_INACCESSIBLE_READ; + status = generic_gss_str_to_oid(minor_status, oid_str, oid); if (status != GSS_S_COMPLETE) map_errcode(minor_status); return status; } -OM_uint32 -gssint_copy_oid_set( - OM_uint32 *minor_status, - const gss_OID_set_desc * const oidset, - gss_OID_set *new_oidset) -{ - return generic_gss_copy_oid_set(minor_status, oidset, new_oidset); -} - int KRB5_CALLCONV gss_oid_equal( gss_const_OID first_oid, diff --git a/src/lib/gssapi/mechglue/mglueP.h b/src/lib/gssapi/mechglue/mglueP.h index edd759cb0b0..2fdd4a9ba59 100644 --- a/src/lib/gssapi/mechglue/mglueP.h +++ b/src/lib/gssapi/mechglue/mglueP.h @@ -799,12 +799,6 @@ OM_uint32 gssint_create_union_context( gss_union_ctx_id_t * /* ctx_out */ ); -OM_uint32 gssint_copy_oid_set( - OM_uint32 *, /* minor_status */ - const gss_OID_set_desc * const, /* oid set */ - gss_OID_set * /* new oid set */ -); - gss_OID gss_find_mechanism_from_name_type (gss_OID); /* name_type */ OM_uint32 gss_add_mech_name_type diff --git a/src/lib/gssapi/spnego/spnego_mech.c b/src/lib/gssapi/spnego/spnego_mech.c index 5923c880b8d..43ba63ab2a7 100644 --- a/src/lib/gssapi/spnego/spnego_mech.c +++ b/src/lib/gssapi/spnego/spnego_mech.c @@ -379,7 +379,7 @@ spnego_gss_acquire_cred_from(OM_uint32 *minor_status, &amechs, time_rec); if (actual_mechs && amechs != GSS_C_NULL_OID_SET) { - (void) gssint_copy_oid_set(&tmpmin, amechs, actual_mechs); + (void) generic_gss_copy_oid_set(&tmpmin, amechs, actual_mechs); } (void) gss_release_oid_set(&tmpmin, &amechs); diff --git a/src/lib/krb5/krb/ser_actx.c b/src/lib/krb5/krb/ser_actx.c index 6de35a146ac..ed8e25596b5 100644 --- a/src/lib/krb5/krb/ser_actx.c +++ b/src/lib/krb5/krb/ser_actx.c @@ -171,7 +171,7 @@ k5_externalize_auth_context(krb5_auth_context auth_context, /* Now handle remote_port, if appropriate */ if (!kret && auth_context->remote_port) { (void) krb5_ser_pack_int32(TOKEN_RPORT, &bp, &remain); - kret = k5_externalize_address(auth_context->remote_addr, + kret = k5_externalize_address(auth_context->remote_port, &bp, &remain); } diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c index 4ae2c00ad5e..14370ae34be 100644 --- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c +++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c @@ -3602,20 +3602,22 @@ pkinit_open_session(krb5_context context, /* Login if needed */ if (tinfo.flags & CKF_LOGIN_REQUIRED) { - if (cctx->p11_module_name != NULL) { - if (cctx->slotid != PK_NOSLOT) { - if (asprintf(&p11name, - "PKCS11:module_name=%s:slotid=%ld:token=%.*s", - cctx->p11_module_name, (long)cctx->slotid, - (int)label_len, tinfo.label) < 0) - p11name = NULL; - } else { - if (asprintf(&p11name, - "PKCS11:module_name=%s,token=%.*s", - cctx->p11_module_name, - (int)label_len, tinfo.label) < 0) - p11name = NULL; - } + if (cctx->slotid != PK_NOSLOT) { + if (asprintf(&p11name, + "PKCS11:module_name=%s:slotid=%ld:token=%.*s", + cctx->p11_module_name, (long)cctx->slotid, + (int)label_len, tinfo.label) < 0) + p11name = NULL; + } else { + if (asprintf(&p11name, + "PKCS11:module_name=%s,token=%.*s", + cctx->p11_module_name, + (int)label_len, tinfo.label) < 0) + p11name = NULL; + } + if (p11name == NULL) { + ret = ENOMEM; + goto cleanup; } if (cctx->defer_id_prompt) { /* Supply the identity name to be passed to the responder. */ diff --git a/src/util/profile/prof_tree.c b/src/util/profile/prof_tree.c index 3e2aaa1cf61..b02675741d7 100644 --- a/src/util/profile/prof_tree.c +++ b/src/util/profile/prof_tree.c @@ -219,7 +219,8 @@ errcode_t profile_add_node(struct profile_node *section, const char *name, } else if (value == NULL && cmp == 0 && p->value == NULL && p->deleted != 1) { /* Found duplicate subsection, so don't make a new one. */ - *ret_node = p; + if (ret_node) + *ret_node = p; return 0; } else if (check_final && cmp == 0 && p->final) { /* This key already exists with the final flag and we were asked