krb5 commit: Avoid use after free during libkrad cleanup

[Greg Hudson]

https://github.com/krb5/krb5/commit/ce160f8826bae223876a6527a731c36b6912db15
commit ce160f8826bae223876a6527a731c36b6912db15
Author: Greg Hudson <ghudson at mit.edu>
Date:   Tue Nov 9 13:00:43 2021 -0500

    Avoid use after free during libkrad cleanup
    
    libkrad client requests contain a list of references to remotes, with
    no back-references or reference counts.  To prevent accesses to
    dangling references during cleanup, cancel all requests on all remotes
    before freeing any remotes.
    
    Remove the code for aging out unused servers.  This code was fairly
    safe as all requests referencing a remote should have completed or
    timed out during an hour of disuse, but in the current design we have
    no way to guarantee or check that.  The set of addresses we send
    RADIUS requests to will generally be small, so aging out servers is
    unnecessary.
    
    ticket: 9035 (new)

 src/lib/krad/client.c   |   42 ++++++++++++++----------------------------
 src/lib/krad/internal.h |    4 ++++
 src/lib/krad/remote.c   |   11 ++++++++---
 3 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/src/lib/krad/client.c b/src/lib/krad/client.c
index 6365dd1..810940a 100644
--- a/src/lib/krad/client.c
+++ b/src/lib/krad/client.c
@@ -64,7 +64,6 @@ struct request_st {
 
 struct server_st {
     krad_remote *serv;
-    time_t last;
     K5_LIST_ENTRY(server_st) list;
 };
 
@@ -81,15 +80,10 @@ get_server(krad_client *rc, const struct addrinfo *ai, const char *secret,
            krad_remote **out)
 {
     krb5_error_code retval;
-    time_t currtime;
     server *srv;
 
-    if (time(&currtime) == (time_t)-1)
-        return errno;
-
     K5_LIST_FOREACH(srv, &rc->servers, list) {
         if (kr_remote_equals(srv->serv, ai, secret)) {
-            srv->last = currtime;
             *out = srv->serv;
             return 0;
         }
@@ -98,7 +92,6 @@ get_server(krad_client *rc, const struct addrinfo *ai, const char *secret,
     srv = calloc(1, sizeof(server));
     if (srv == NULL)
         return ENOMEM;
-    srv->last = currtime;
 
     retval = kr_remote_new(rc->kctx, rc->vctx, ai, secret, &srv->serv);
     if (retval != 0) {
@@ -173,28 +166,12 @@ request_new(krad_client *rc, krad_code code, const krad_attrset *attrs,
     return 0;
 }
 
-/* Close remotes that haven't been used in a while. */
-static void
-age(struct server_head *head, time_t currtime)
-{
-    server *srv, *tmp;
-
-    K5_LIST_FOREACH_SAFE(srv, head, list, tmp) {
-        if (currtime == (time_t)-1 || currtime - srv->last > 60 * 60) {
-            K5_LIST_REMOVE(srv, list);
-            kr_remote_free(srv->serv);
-            free(srv);
-        }
-    }
-}
-
 /* Handle a response from a server (or related errors). */
 static void
 on_response(krb5_error_code retval, const krad_packet *reqp,
             const krad_packet *rspp, void *data)
 {
     request *req = data;
-    time_t currtime;
     size_t i;
 
     /* Do nothing if we are already completed. */
@@ -221,10 +198,6 @@ on_response(krb5_error_code retval, const krad_packet *reqp,
     for (i = 0; req->remotes[i].remote != NULL; i++)
         kr_remote_cancel(req->remotes[i].remote, req->remotes[i].packet);
 
-    /* Age out servers that haven't been used in a while. */
-    if (time(&currtime) != (time_t)-1)
-        age(&req->rc->servers, currtime);
-
     request_free(req);
 }
 
@@ -247,10 +220,23 @@ krad_client_new(krb5_context kctx, verto_ctx *vctx, krad_client **out)
 void
 krad_client_free(krad_client *rc)
 {
+    server *srv;
+
     if (rc == NULL)
         return;
 
-    age(&rc->servers, -1);
+    /* Cancel all requests before freeing any remotes, since each request's
+     * callback data may contain references to multiple remotes. */
+    K5_LIST_FOREACH(srv, &rc->servers, list)
+        kr_remote_cancel_all(srv->serv);
+
+    while (!K5_LIST_EMPTY(&rc->servers)) {
+        srv = K5_LIST_FIRST(&rc->servers);
+        K5_LIST_REMOVE(srv, list);
+        kr_remote_free(srv->serv);
+        free(srv);
+    }
+
     free(rc);
 }
 
diff --git a/src/lib/krad/internal.h b/src/lib/krad/internal.h
index 0143d15..7619563 100644
--- a/src/lib/krad/internal.h
+++ b/src/lib/krad/internal.h
@@ -109,6 +109,10 @@ kr_remote_send(krad_remote *rr, krad_code code, krad_attrset *attrs,
 void
 kr_remote_cancel(krad_remote *rr, const krad_packet *pkt);
 
+/* Cancel all requests awaiting responses. */
+void
+kr_remote_cancel_all(krad_remote *rr);
+
 /* Determine if this remote object refers to the remote resource identified
  * by the addrinfo struct and the secret. */
 krb5_boolean
diff --git a/src/lib/krad/remote.c b/src/lib/krad/remote.c
index 7e491e9..06ae751 100644
--- a/src/lib/krad/remote.c
+++ b/src/lib/krad/remote.c
@@ -422,14 +422,19 @@ error:
 }
 
 void
+kr_remote_cancel_all(krad_remote *rr)
+{
+    while (!K5_TAILQ_EMPTY(&rr->list))
+        request_finish(K5_TAILQ_FIRST(&rr->list), ECANCELED, NULL);
+}
+
+void
 kr_remote_free(krad_remote *rr)
 {
     if (rr == NULL)
         return;
 
-    while (!K5_TAILQ_EMPTY(&rr->list))
-        request_finish(K5_TAILQ_FIRST(&rr->list), ECANCELED, NULL);
-
+    kr_remote_cancel_all(rr);
     free(rr->secret);
     if (rr->info != NULL)
         free(rr->info->ai_addr);