--- Begin Message ---
- To: Debian Bug Tracking System <submit@bugs.debian.org>
- Subject: libapache2-mod-auth-kerb: memory leak plus incorrect handling of keytabs
- From: Sorin Manolache <sorinm@gmail.com>
- Date: Tue, 28 Jun 2022 00:36:00 +0200
- Message-id: <165636936049.257789.2122545852793894364.reportbug@local.orange.fr>
Package: libapache2-mod-auth-kerb
Version: 5.4-2.5
Severity: normal
Tags: patch upstream
X-Debbugs-Cc: sorinm@gmail.com
Dear Maintainer,
I think there are two problems with the code:
In authenticate_user_gss the keytab path (configured by the Krb5Keytab conf directive) is in the process environment using a putenv call.
First, there is an obvious memory leak because the string put into the environment is malloc'ed. It is never freed. Moreover, this malloc and putenv happens at every request.
Second, not only that putenv is not thread-safe but it may lead to functional bugs. Let us assume that we have two different directories, each with its own Krb5Keytab conf directive, each pointing to a different keytab file. When two requests are handled by two threads in parallel, each will putenv KRB5_KTNAME=/path/to/keytab. It is possible that the calls to the GSSAPI/Krb5 functions of one thread use the value of the environment variable KRB5_KTNAME set by the other thread.
I've written a patch to correct these two problems. The patch works in my setup but I have to tested it exhaustively. Please have a look.
Thank you and best regards,
Sorin
-- System Information:
Debian Release: bookworm/sid
APT prefers unstable
APT policy: (500, 'unstable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386
Kernel: Linux 5.17.0-1-amd64 (SMP w/8 CPU threads; PREEMPT)
Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=C, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled
Versions of packages libapache2-mod-auth-kerb depends on:
ii apache2-bin [apache2-api-20120211] 2.4.54-1
ii krb5-config 2.6+nmu1
ii libc6 2.33-7
ii libcom-err2 1.46.5-2
ii libgssapi-krb5-2 1.19.2-2+b2
ii libkrb5-3 1.19.2-2+b2
libapache2-mod-auth-kerb recommends no packages.
libapache2-mod-auth-kerb suggests no packages.
-- no debconf information
Index: mod_auth_kerb-5.4/src/mod_auth_kerb.c
===================================================================
--- mod_auth_kerb-5.4.orig/src/mod_auth_kerb.c
+++ mod_auth_kerb-5.4/src/mod_auth_kerb.c
@@ -228,7 +228,8 @@ static const char *
cmd_delegationlock(cmd_parms *cmd, void *dconf, const char *a1);
static int
-obtain_server_credentials(request_rec *r, const char *service_name);
+obtain_server_credentials(request_rec *r, const char *service_name,
+ const char *kt_name);
#ifdef STANDARD20_MODULE_STUFF
#define command(name, func, var, type, usage) \
@@ -1245,14 +1246,12 @@ end:
}
static int
-get_gss_creds(request_rec *r,
- kerb_auth_config *conf,
- gss_cred_id_t *server_creds)
+get_server_name(request_rec *r,
+ kerb_auth_config *conf,
+ gss_name_t *server_name)
{
gss_buffer_desc token = GSS_C_EMPTY_BUFFER;
- OM_uint32 major_status, minor_status, minor_status2;
- gss_name_t server_name = GSS_C_NO_NAME;
- gss_cred_usage_t usage = GSS_C_ACCEPT;
+ OM_uint32 major_status, minor_status;
char buf[1024];
int have_server_princ;
@@ -1260,8 +1259,8 @@ get_gss_creds(request_rec *r,
have_server_princ = conf->krb_service_name && strchr(conf->krb_service_name, '/') != NULL;
if (have_server_princ)
strncpy(buf, conf->krb_service_name, sizeof(buf));
- else if (conf->krb_service_name && strcmp(conf->krb_service_name,"Any") == 0) {
- *server_creds = GSS_C_NO_CREDENTIAL;
+ else if (conf->krb_service_name && strcmp(conf->krb_service_name,"Any") == 0) {
+ *server_name = GSS_C_NO_NAME;
return 0;
}
else
@@ -1274,7 +1273,7 @@ get_gss_creds(request_rec *r,
major_status = gss_import_name(&minor_status, &token,
(have_server_princ) ? (gss_OID) GSS_KRB5_NT_PRINCIPAL_NAME : (gss_OID) GSS_C_NT_HOSTBASED_SERVICE,
- &server_name);
+ server_name);
memset(&token, 0, sizeof(token));
if (GSS_ERROR(major_status)) {
log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -1283,7 +1282,7 @@ get_gss_creds(request_rec *r,
return HTTP_INTERNAL_SERVER_ERROR;
}
- major_status = gss_display_name(&minor_status, server_name, &token, NULL);
+ major_status = gss_display_name(&minor_status, *server_name, &token, NULL);
if (GSS_ERROR(major_status)) {
/* Perhaps we could just ignore this error but it's safer to give up now,
I think */
@@ -1295,15 +1294,46 @@ get_gss_creds(request_rec *r,
log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "Acquiring creds for %s",
token.value);
+ gss_release_buffer(&minor_status, &token);
+
+ return 0;
+}
+
+static int
+get_gss_creds(request_rec *r,
+ kerb_auth_config *conf,
+ gss_cred_id_t *server_creds)
+{
+ OM_uint32 major_status, minor_status, minor_status2;
+ gss_name_t server_name = GSS_C_NO_NAME;
+ gss_cred_usage_t usage = GSS_C_ACCEPT;
+ gss_key_value_element_desc e[1];
+ gss_key_value_set_desc kv = {0, &e[0]};
+
+ int rc = get_server_name(r, conf, &server_name);
+ if (rc)
+ return rc;
+
if (conf->krb5_s4u2proxy) {
usage = GSS_C_BOTH;
- obtain_server_credentials(r, conf->krb_service_name);
+ obtain_server_credentials(r, conf->krb_service_name, conf->krb_5_keytab);
}
- gss_release_buffer(&minor_status, &token);
-
- major_status = gss_acquire_cred(&minor_status, server_name, GSS_C_INDEFINITE,
- GSS_C_NO_OID_SET, usage,
- server_creds, NULL, NULL);
+
+ if (conf->krb_5_keytab) {
+ e[kv.count].value = apr_pstrcat(r->pool, "FILE:", conf->krb_5_keytab,
+ NULL);
+ if (!e[kv.count].value) {
+ log_rerror(APLOG_MARK, APLOG_ERR, APR_ENOMEM, r,
+ "Out of memory");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ e[kv.count].key = "keytab";
+ ++kv.count;
+ }
+
+ major_status = gss_acquire_cred_from(
+ &minor_status, server_name, GSS_C_INDEFINITE, GSS_C_NO_OID_SET,
+ usage, &kv, server_creds, NULL, NULL);
gss_release_name(&minor_status2, &server_name);
if (GSS_ERROR(major_status)) {
log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -1447,8 +1477,8 @@ cleanup:
}
static int
-obtain_server_credentials(request_rec *r,
- const char *service_name)
+obtain_server_credentials(request_rec *r, const char *service_name,
+ const char *kt_name)
{
krb5_context kcontext = NULL;
krb5_keytab keytab = NULL;
@@ -1555,7 +1585,9 @@ obtain_server_credentials(request_rec *r
}
}
- if ((kerr = krb5_kt_default(kcontext, &keytab))) {
+ if ((kerr = (kt_name ?
+ krb5_kt_resolve(kcontext, kt_name, &keytab) :
+ krb5_kt_default(kcontext, &keytab)))) {
log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
"Unable to get default keytab: %s (%d)",
error_message(kerr), kerr);
@@ -1667,25 +1699,6 @@ authenticate_user_gss(request_rec *r, ke
spnego_oid.length = 6;
spnego_oid.elements = (void *)"\x2b\x06\x01\x05\x05\x02";
- if (conf->krb_5_keytab) {
- char *ktname;
- /* we don't use the ap_* calls here, since the string passed to putenv()
- * will become part of the enviroment and shouldn't be free()ed by apache
- */
- ktname = malloc(strlen("KRB5_KTNAME=") + strlen(conf->krb_5_keytab) + 1);
- if (ktname == NULL) {
- log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "malloc() failed: not enough memory");
- ret = HTTP_INTERNAL_SERVER_ERROR;
- goto end;
- }
- sprintf(ktname, "KRB5_KTNAME=%s", conf->krb_5_keytab);
- putenv(ktname);
-#ifdef HEIMDAL
- /* Seems to be also supported by latest MIT */
- gsskrb5_register_acceptor_identity(conf->krb_5_keytab);
-#endif
- }
-
ret = get_gss_creds(r, conf, &server_creds);
if (ret)
goto end;
--- End Message ---