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 9ebae7c. 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.
greghudson · Nov 23, 2024 · 37202c0 · 37202c0
1 parent 15d5e8d
commit 37202c0
Show file tree

Hide file tree

Showing 15 changed files with 104 additions and 89 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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