Blame SOURCES/krb5-krad-remote.patch

38a7f7
From a8551b609fd50458ca3c06a9dd345b6cdf18689b Mon Sep 17 00:00:00 2001
38a7f7
From: Greg Hudson <ghudson@mit.edu>
38a7f7
Date: Tue, 9 Nov 2021 13:00:43 -0500
38a7f7
Subject: [PATCH 1/2] Avoid use after free during libkrad cleanup
38a7f7
38a7f7
libkrad client requests contain a list of references to remotes, with
38a7f7
no back-references or reference counts.  To prevent accesses to
38a7f7
dangling references during cleanup, cancel all requests on all remotes
38a7f7
before freeing any remotes.
38a7f7
38a7f7
Remove the code for aging out unused servers.  This code was fairly
38a7f7
safe as all requests referencing a remote should have completed or
38a7f7
timed out during an hour of disuse, but in the current design we have
38a7f7
no way to guarantee or check that.  The set of addresses we send
38a7f7
RADIUS requests to will generally be small, so aging out servers is
38a7f7
unnecessary.
38a7f7
38a7f7
ticket: 9035 (new)
38a7f7
---
38a7f7
 src/lib/krad/client.c   | 42 ++++++++++++++---------------------------
38a7f7
 src/lib/krad/internal.h |  4 ++++
38a7f7
 src/lib/krad/remote.c   | 11 ++++++++---
38a7f7
 3 files changed, 26 insertions(+), 31 deletions(-)
38a7f7
38a7f7
diff --git a/src/lib/krad/client.c b/src/lib/krad/client.c
38a7f7
index 6365dd1c6..810940afc 100644
38a7f7
--- a/src/lib/krad/client.c
38a7f7
+++ b/src/lib/krad/client.c
38a7f7
@@ -64,7 +64,6 @@ struct request_st {
38a7f7
 
38a7f7
 struct server_st {
38a7f7
     krad_remote *serv;
38a7f7
-    time_t last;
38a7f7
     K5_LIST_ENTRY(server_st) list;
38a7f7
 };
38a7f7
 
38a7f7
@@ -81,15 +80,10 @@ get_server(krad_client *rc, const struct addrinfo *ai, const char *secret,
38a7f7
            krad_remote **out)
38a7f7
 {
38a7f7
     krb5_error_code retval;
38a7f7
-    time_t currtime;
38a7f7
     server *srv;
38a7f7
 
38a7f7
-    if (time(&currtime) == (time_t)-1)
38a7f7
-        return errno;
38a7f7
-
38a7f7
     K5_LIST_FOREACH(srv, &rc->servers, list) {
38a7f7
         if (kr_remote_equals(srv->serv, ai, secret)) {
38a7f7
-            srv->last = currtime;
38a7f7
             *out = srv->serv;
38a7f7
             return 0;
38a7f7
         }
38a7f7
@@ -98,7 +92,6 @@ get_server(krad_client *rc, const struct addrinfo *ai, const char *secret,
38a7f7
     srv = calloc(1, sizeof(server));
38a7f7
     if (srv == NULL)
38a7f7
         return ENOMEM;
38a7f7
-    srv->last = currtime;
38a7f7
 
38a7f7
     retval = kr_remote_new(rc->kctx, rc->vctx, ai, secret, &srv->serv);
38a7f7
     if (retval != 0) {
38a7f7
@@ -173,28 +166,12 @@ request_new(krad_client *rc, krad_code code, const krad_attrset *attrs,
38a7f7
     return 0;
38a7f7
 }
38a7f7
 
38a7f7
-/* Close remotes that haven't been used in a while. */
38a7f7
-static void
38a7f7
-age(struct server_head *head, time_t currtime)
38a7f7
-{
38a7f7
-    server *srv, *tmp;
38a7f7
-
38a7f7
-    K5_LIST_FOREACH_SAFE(srv, head, list, tmp) {
38a7f7
-        if (currtime == (time_t)-1 || currtime - srv->last > 60 * 60) {
38a7f7
-            K5_LIST_REMOVE(srv, list);
38a7f7
-            kr_remote_free(srv->serv);
38a7f7
-            free(srv);
38a7f7
-        }
38a7f7
-    }
38a7f7
-}
38a7f7
-
38a7f7
 /* Handle a response from a server (or related errors). */
38a7f7
 static void
38a7f7
 on_response(krb5_error_code retval, const krad_packet *reqp,
38a7f7
             const krad_packet *rspp, void *data)
38a7f7
 {
38a7f7
     request *req = data;
38a7f7
-    time_t currtime;
38a7f7
     size_t i;
38a7f7
 
38a7f7
     /* Do nothing if we are already completed. */
38a7f7
@@ -221,10 +198,6 @@ on_response(krb5_error_code retval, const krad_packet *reqp,
38a7f7
     for (i = 0; req->remotes[i].remote != NULL; i++)
38a7f7
         kr_remote_cancel(req->remotes[i].remote, req->remotes[i].packet);
38a7f7
 
38a7f7
-    /* Age out servers that haven't been used in a while. */
38a7f7
-    if (time(&currtime) != (time_t)-1)
38a7f7
-        age(&req->rc->servers, currtime);
38a7f7
-
38a7f7
     request_free(req);
38a7f7
 }
38a7f7
 
38a7f7
@@ -247,10 +220,23 @@ krad_client_new(krb5_context kctx, verto_ctx *vctx, krad_client **out)
38a7f7
 void
38a7f7
 krad_client_free(krad_client *rc)
38a7f7
 {
38a7f7
+    server *srv;
38a7f7
+
38a7f7
     if (rc == NULL)
38a7f7
         return;
38a7f7
 
38a7f7
-    age(&rc->servers, -1);
38a7f7
+    /* Cancel all requests before freeing any remotes, since each request's
38a7f7
+     * callback data may contain references to multiple remotes. */
38a7f7
+    K5_LIST_FOREACH(srv, &rc->servers, list)
38a7f7
+        kr_remote_cancel_all(srv->serv);
38a7f7
+
38a7f7
+    while (!K5_LIST_EMPTY(&rc->servers)) {
38a7f7
+        srv = K5_LIST_FIRST(&rc->servers);
38a7f7
+        K5_LIST_REMOVE(srv, list);
38a7f7
+        kr_remote_free(srv->serv);
38a7f7
+        free(srv);
38a7f7
+    }
38a7f7
+
38a7f7
     free(rc);
38a7f7
 }
38a7f7
 
38a7f7
diff --git a/src/lib/krad/internal.h b/src/lib/krad/internal.h
38a7f7
index 223ffd730..fa012db78 100644
38a7f7
--- a/src/lib/krad/internal.h
38a7f7
+++ b/src/lib/krad/internal.h
38a7f7
@@ -120,6 +120,10 @@ kr_remote_send(krad_remote *rr, krad_code code, krad_attrset *attrs,
38a7f7
 void
38a7f7
 kr_remote_cancel(krad_remote *rr, const krad_packet *pkt);
38a7f7
 
38a7f7
+/* Cancel all requests awaiting responses. */
38a7f7
+void
38a7f7
+kr_remote_cancel_all(krad_remote *rr);
38a7f7
+
38a7f7
 /* Determine if this remote object refers to the remote resource identified
38a7f7
  * by the addrinfo struct and the secret. */
38a7f7
 krb5_boolean
38a7f7
diff --git a/src/lib/krad/remote.c b/src/lib/krad/remote.c
38a7f7
index c8912892c..01a5fd2a4 100644
38a7f7
--- a/src/lib/krad/remote.c
38a7f7
+++ b/src/lib/krad/remote.c
38a7f7
@@ -452,15 +452,20 @@ error:
38a7f7
     return retval;
38a7f7
 }
38a7f7
 
38a7f7
+void
38a7f7
+kr_remote_cancel_all(krad_remote *rr)
38a7f7
+{
38a7f7
+    while (!K5_TAILQ_EMPTY(&rr->list))
38a7f7
+        request_finish(K5_TAILQ_FIRST(&rr->list), ECANCELED, NULL);
38a7f7
+}
38a7f7
+
38a7f7
 void
38a7f7
 kr_remote_free(krad_remote *rr)
38a7f7
 {
38a7f7
     if (rr == NULL)
38a7f7
         return;
38a7f7
 
38a7f7
-    while (!K5_TAILQ_EMPTY(&rr->list))
38a7f7
-        request_finish(K5_TAILQ_FIRST(&rr->list), ECANCELED, NULL);
38a7f7
-
38a7f7
+    kr_remote_cancel_all(rr);
38a7f7
     free(rr->secret);
38a7f7
     if (rr->info != NULL)
38a7f7
         free(rr->info->ai_addr);
38a7f7
-- 
38a7f7
2.35.3
38a7f7