You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
171 lines
5.1 KiB
171 lines
5.1 KiB
From a8551b609fd50458ca3c06a9dd345b6cdf18689b Mon Sep 17 00:00:00 2001 |
|
From: Greg Hudson <ghudson@mit.edu> |
|
Date: Tue, 9 Nov 2021 13:00:43 -0500 |
|
Subject: [PATCH 1/2] 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 6365dd1c6..810940afc 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 223ffd730..fa012db78 100644 |
|
--- a/src/lib/krad/internal.h |
|
+++ b/src/lib/krad/internal.h |
|
@@ -120,6 +120,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 c8912892c..01a5fd2a4 100644 |
|
--- a/src/lib/krad/remote.c |
|
+++ b/src/lib/krad/remote.c |
|
@@ -452,15 +452,20 @@ error: |
|
return retval; |
|
} |
|
|
|
+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); |
|
-- |
|
2.35.3 |
|
|
|
|