rpms/krb5
6
0
Fork 0
Code Issues Pull Requests Releases Activity

Fix use-after-free during krad remote_shutdown()

Browse Source
This commit is contained in:
Robbie Harwood 2021-07-01 13:17:47 -04:00
parent c5044b0741
commit af96dc0c6c
3 changed files with 237 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

193
Clean-up-gssapi_krb5-ccache-name-functions.patch Normal file
View File

@ -0,0 +1,193 @@
From 8285f21d40e30477436128ae2c28403cd5575074 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Wed, 26 May 2021 18:22:10 -0400
Subject: [PATCH] Clean up gssapi_krb5 ccache name functions
Modernize kg_get_ccache_name() and kg_get_ccache_name(). Drop
unnecessary use of const in kg_get_ccache_name() so that its return
value can be properly freed. Fixes some static analyzer false
positives.
(cherry picked from commit f573f7f8ee5269103a0492d6521a3242c5ffb63b)
---
src/lib/gssapi/krb5/gssapiP_krb5.h | 3 +-
src/lib/gssapi/krb5/gssapi_krb5.c | 47 ++++++++--------------
src/lib/gssapi/krb5/set_ccache.c | 64 ++++++++++++------------------
3 files changed, 42 insertions(+), 72 deletions(-)
diff --git a/src/lib/gssapi/krb5/gssapiP_krb5.h b/src/lib/gssapi/krb5/gssapiP_krb5.h
index fd7abbd77..88d41130a 100644
--- a/src/lib/gssapi/krb5/gssapiP_krb5.h
+++ b/src/lib/gssapi/krb5/gssapiP_krb5.h
@@ -380,8 +380,7 @@ OM_uint32 kg_sync_ccache_name (krb5_context context, OM_uint32 *minor_status);
OM_uint32 kg_caller_provided_ccache_name (OM_uint32 *minor_status,
int *out_caller_provided_name);
-OM_uint32 kg_get_ccache_name (OM_uint32 *minor_status,
- const char **out_name);
+OM_uint32 kg_get_ccache_name (OM_uint32 *minor_status, char **out_name);
OM_uint32 kg_set_ccache_name (OM_uint32 *minor_status,
const char *name);
diff --git a/src/lib/gssapi/krb5/gssapi_krb5.c b/src/lib/gssapi/krb5/gssapi_krb5.c
index 46aa9b7a5..9915a8bb5 100644
--- a/src/lib/gssapi/krb5/gssapi_krb5.c
+++ b/src/lib/gssapi/krb5/gssapi_krb5.c
@@ -253,46 +253,31 @@ kg_caller_provided_ccache_name (OM_uint32 *minor_status,
}
OM_uint32
-kg_get_ccache_name (OM_uint32 *minor_status, const char **out_name)
+kg_get_ccache_name(OM_uint32 *minor_status, char **out_name)
{
- const char *name = NULL;
- OM_uint32 err = 0;
char *kg_ccache_name;
+ const char *def_name;
+ OM_uint32 err;
+ krb5_context context;
+
+ *out_name = NULL;
kg_ccache_name = k5_getspecific(K5_KEY_GSS_KRB5_CCACHE_NAME);
-
if (kg_ccache_name != NULL) {
- name = strdup(kg_ccache_name);
- if (name == NULL)
- err = ENOMEM;
+ *out_name = strdup(kg_ccache_name);
+ err = (*out_name == NULL) ? ENOMEM : 0;
} else {
- krb5_context context = NULL;
-
- /* Reset the context default ccache (see text above), and then
- retrieve it. */
+ /* Use the default ccache name. */
err = krb5_gss_init_context(&context);
- if (!err)
- err = krb5_cc_set_default_name (context, NULL);
- if (!err) {
- name = krb5_cc_default_name(context);
- if (name) {
- name = strdup(name);
- if (name == NULL)
- err = ENOMEM;
- }
- }
- if (err && context)
- save_error_info(err, context);
- if (context)
- krb5_free_context(context);
- }
-
- if (!err) {
- if (out_name) {
- *out_name = name;
- }
+ if (err)
+ goto cleanup;
+ def_name = krb5_cc_default_name(context);
+ *out_name = (def_name != NULL) ? strdup(def_name) : NULL;
+ err = (*out_name == NULL) ? ENOMEM : 0;
+ krb5_free_context(context);
}
+cleanup:
*minor_status = err;
return (*minor_status == 0) ? GSS_S_COMPLETE : GSS_S_FAILURE;
}
diff --git a/src/lib/gssapi/krb5/set_ccache.c b/src/lib/gssapi/krb5/set_ccache.c
index 8acf3ec90..91c3462be 100644
--- a/src/lib/gssapi/krb5/set_ccache.c
+++ b/src/lib/gssapi/krb5/set_ccache.c
@@ -26,7 +26,7 @@
/*
* Set ccache name used by gssapi, and optionally obtain old ccache
- * name. Caller should not free returned name.
+ * name. Caller must not free returned name.
*/
#include <string.h>
@@ -38,11 +38,9 @@ gss_krb5int_ccache_name(OM_uint32 *minor_status,
const gss_OID desired_object,
const gss_buffer_t value)
{
- char *old_name = NULL;
OM_uint32 err = 0;
- OM_uint32 minor = 0;
- char *gss_out_name;
struct krb5_gss_ccache_name_req *req;
+ char *old_name, *cur_name = NULL;
err = gss_krb5int_initialize_library();
if (err) {
@@ -57,45 +55,33 @@ gss_krb5int_ccache_name(OM_uint32 *minor_status,
req = (struct krb5_gss_ccache_name_req *)value->value;
- gss_out_name = k5_getspecific(K5_KEY_GSS_KRB5_SET_CCACHE_OLD_NAME);
+ /* Our job is simple if the caller doesn't want the current name. */
+ if (req->out_name == NULL)
+ return kg_set_ccache_name(minor_status, req->name);
- if (req->out_name) {
- const char *tmp_name = NULL;
+ /* Fetch the current name and change it. */
+ kg_get_ccache_name(&err, &cur_name);
+ if (err)
+ goto cleanup;
+ kg_set_ccache_name(&err, req->name);
+ if (err)
+ goto cleanup;
- if (!err) {
- kg_get_ccache_name (&err, &tmp_name);
- }
- if (!err) {
- old_name = gss_out_name;
- gss_out_name = (char *)tmp_name;
- }
- }
- /* If out_name was NULL, we keep the same gss_out_name value, and
- don't free up any storage (leave old_name NULL). */
+ /* Store the current name in a thread-specific variable. Free that
+ * variable's previous contents. */
+ old_name = k5_getspecific(K5_KEY_GSS_KRB5_SET_CCACHE_OLD_NAME);
+ err = k5_setspecific(K5_KEY_GSS_KRB5_SET_CCACHE_OLD_NAME, cur_name);
+ if (err)
+ goto cleanup;
+ free(old_name);
- if (!err)
- kg_set_ccache_name (&err, req->name);
-
- minor = k5_setspecific(K5_KEY_GSS_KRB5_SET_CCACHE_OLD_NAME, gss_out_name);
- if (minor) {
- /* Um. Now what? */
- if (err == 0) {
- err = minor;
- }
- free(gss_out_name);
- gss_out_name = NULL;
- }
-
- if (!err) {
- if (req->out_name) {
- *(req->out_name) = gss_out_name;
- }
- }
-
- if (old_name != NULL) {
- free (old_name);
- }
+ /* Give the caller an alias to the stored value. */
+ *req->out_name = cur_name;
+ cur_name = NULL;
+ err = 0;
+cleanup:
+ free(cur_name);
*minor_status = err;
return (*minor_status == 0) ? GSS_S_COMPLETE : GSS_S_FAILURE;
}

38
Fix-use-after-free-during-krad-remote_shutdown.patch Normal file
View File

@ -0,0 +1,38 @@
From bcd7b5e8aa0d325e9b178d9be3459759d39b631e Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Sat, 29 May 2021 13:25:59 -0400
Subject: [PATCH] Fix use-after-free during krad remote_shutdown()
Since elements of the queue can be removed on out-of-memory errors,
the correct call is K5_TAILQ_FOREACH_SAFE, not K5_TAILQ_FOREACH.
Reported by Coverity.
ticket: 9015 (new)
tags: pullup
target_version: 1.19-next
target_version: 1.18-next
(cherry picked from commit 8c88defb16b34937d5b72b4832c854ce2dbe32d1)
---
src/lib/krad/remote.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lib/krad/remote.c b/src/lib/krad/remote.c
index eca432424..7b5804b1d 100644
--- a/src/lib/krad/remote.c
+++ b/src/lib/krad/remote.c
@@ -220,12 +220,12 @@ static void
remote_shutdown(krad_remote *rr)
{
krb5_error_code retval;
- request *r;
+ request *r, *next;
remote_disconnect(rr);
/* Start timers for all unsent packets. */
- K5_TAILQ_FOREACH(r, &rr->list, list) {
+ K5_TAILQ_FOREACH_SAFE(r, &rr->list, list, next) {
if (r->timer == NULL) {
retval = request_start_timer(r, rr->vctx);
if (retval != 0)

7
krb5.spec
View File

@ -42,7 +42,7 @@
Summary: The Kerberos network authentication system
Name: krb5
Version: 1.19.1
Release: %{?zdpd}12%{?dist}
Release: %{?zdpd}13%{?dist}
# rharwood has trust path to signing key and verifies on check-in
Source0: https://web.mit.edu/kerberos/dist/krb5/%{version}/krb5-%{version}%{?dashpre}.tar.gz
@ -90,6 +90,8 @@ Patch24: Fix-leaks-on-error-in-kadm5-init-functions.patch
Patch25: Clean-up-context-after-failed-open-in-libkdb5.patch
Patch26: Use-asan-in-one-of-the-CI-builds.patch
Patch27: Using-locking-in-MEMORY-krb5_cc_get_principal.patch
Patch28: Fix-use-after-free-during-krad-remote_shutdown.patch
Patch29: Clean-up-gssapi_krb5-ccache-name-functions.patch
License: MIT
URL: https://web.mit.edu/kerberos/www/
@ -652,6 +654,9 @@ exit 0
%{_libdir}/libkadm5srv_mit.so.*
%changelog
* Thu Jul 01 2021 Robbie Harwood <rharwood@redhat.com> - 1.19.1-13
- Fix use-after-free during krad remote_shutdown()
* Mon Jun 28 2021 Robbie Harwood <rharwood@redhat.com> - 1.19.1-12
- MEMORY locking fix and static analysis pullup