Fix libkrad client cleanup code
Resolves: rhbz#2100351 Signed-off-by: Julien Rische <jrische@redhat.com>
This commit is contained in:
		
							parent
							
								
									6ea8af6747
								
							
						
					
					
						commit
						5fafe74f37
					
				
							
								
								
									
										171
									
								
								krb5-krad-remote.patch
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										171
									
								
								krb5-krad-remote.patch
									
									
									
									
									
										Normal file
									
								
							| @ -0,0 +1,171 @@ | |||||||
|  | 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 | ||||||
|  | 
 | ||||||
| @ -42,7 +42,7 @@ | |||||||
| Summary: The Kerberos network authentication system | Summary: The Kerberos network authentication system | ||||||
| Name: krb5 | Name: krb5 | ||||||
| Version: 1.19.1 | Version: 1.19.1 | ||||||
| Release: %{?zdpd}20%{?dist} | Release: %{?zdpd}21%{?dist} | ||||||
| 
 | 
 | ||||||
| # rharwood has trust path to signing key and verifies on check-in | # 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 | Source0: https://web.mit.edu/kerberos/dist/krb5/%{version}/krb5-%{version}%{?dashpre}.tar.gz | ||||||
| @ -95,6 +95,7 @@ Patch30: downstream-Use-newly-enforced-dejagnu-path-naming-convention.patch | |||||||
| Patch31: Try-harder-to-avoid-password-change-replay-errors.patch | Patch31: Try-harder-to-avoid-password-change-replay-errors.patch | ||||||
| Patch32: Add-configure-variable-for-default-PKCS-11-module.patch | Patch32: Add-configure-variable-for-default-PKCS-11-module.patch | ||||||
| Patch33: downstream-Allow-krad-UDP-TCP-localhost-connection-with-FIPS.patch | Patch33: downstream-Allow-krad-UDP-TCP-localhost-connection-with-FIPS.patch | ||||||
|  | Patch34: krb5-krad-remote.patch | ||||||
| 
 | 
 | ||||||
| License: MIT | License: MIT | ||||||
| URL: https://web.mit.edu/kerberos/www/ | URL: https://web.mit.edu/kerberos/www/ | ||||||
| @ -652,6 +653,10 @@ exit 0 | |||||||
| %{_libdir}/libkadm5srv_mit.so.* | %{_libdir}/libkadm5srv_mit.so.* | ||||||
| 
 | 
 | ||||||
| %changelog | %changelog | ||||||
|  | * Thu Jun 23 2022 Julien Rische <jrische@redhat.com> - 1.19.1-21 | ||||||
|  | - Fix libkrad client cleanup | ||||||
|  | - Resolves: rhbz#2100351 | ||||||
|  | 
 | ||||||
| * Thu May 12 2022 Julien Rische <jrische@redhat.com> - 1.19.1-20 | * Thu May 12 2022 Julien Rische <jrische@redhat.com> - 1.19.1-20 | ||||||
| - Fix OpenSSL 3 MD5 encyption in FIPS mode | - Fix OpenSSL 3 MD5 encyption in FIPS mode | ||||||
| - Allow libkrad UDP/TCP connection to localhost in FIPS mode | - Allow libkrad UDP/TCP connection to localhost in FIPS mode | ||||||
|  | |||||||
		Loading…
	
		Reference in New Issue
	
	Block a user