Skip to content

Commit

Permalink
Fix leaks on error in kadm5 init functions
Browse files Browse the repository at this point in the history 
In the GENERIC_CHECK_HANDLE function, separate out the
version-checking logic so we can call it in the init functions before
allocating resources.

In the client and server library initialization functions, use a
single exit path after argument validation, and share the destruction
code with kadm5_destroy() via a helper.
  • Loading branch information
@greghudson
greghudson committed Jun 28, 2021
1 parent aa9b4a2 commit 552d7b7
Show file tree
Hide file tree
Showing 3 changed files with 145 additions and 259 deletions.
39 changes: 21 additions & 18 deletions src/lib/kadm5/admin_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,32 @@

#define KADM5_SERVER_HANDLE_MAGIC 0x12345800

#define GENERIC_CHECK_HANDLE(handle, old_api_version, new_api_version) \
#define CHECK_VERSIONS(struct_version, api_version, old_api_err, new_api_err) \
{ \
kadm5_server_handle_t srvr = \
(kadm5_server_handle_t) handle; \
\
if (! srvr) \
return KADM5_BAD_SERVER_HANDLE; \
if (srvr->magic_number != KADM5_SERVER_HANDLE_MAGIC) \
return KADM5_BAD_SERVER_HANDLE; \
if ((srvr->struct_version & KADM5_MASK_BITS) != \
KADM5_STRUCT_VERSION_MASK) \
if ((struct_version & KADM5_MASK_BITS) != KADM5_STRUCT_VERSION_MASK) \
return KADM5_BAD_STRUCT_VERSION; \
if (srvr->struct_version < KADM5_STRUCT_VERSION_1) \
if (struct_version < KADM5_STRUCT_VERSION_1) \
return KADM5_OLD_STRUCT_VERSION; \
if (srvr->struct_version > KADM5_STRUCT_VERSION_1) \
if (struct_version > KADM5_STRUCT_VERSION_1) \
return KADM5_NEW_STRUCT_VERSION; \
if ((srvr->api_version & KADM5_MASK_BITS) != \
KADM5_API_VERSION_MASK) \
if ((api_version & KADM5_MASK_BITS) != KADM5_API_VERSION_MASK) \
return KADM5_BAD_API_VERSION; \
if (srvr->api_version < KADM5_API_VERSION_2) \
return old_api_version; \
if (srvr->api_version > KADM5_API_VERSION_4) \
return new_api_version; \
if (api_version < KADM5_API_VERSION_2) \
return old_api_err; \
if (api_version > KADM5_API_VERSION_4) \
return new_api_err; \
}

#define GENERIC_CHECK_HANDLE(handle, old_api_err, new_api_err) \
{ \
kadm5_server_handle_t srvr = handle; \
\
if (srvr == NULL) \
return KADM5_BAD_SERVER_HANDLE; \
if (srvr->magic_number != KADM5_SERVER_HANDLE_MAGIC) \
return KADM5_BAD_SERVER_HANDLE; \
CHECK_VERSIONS(srvr->struct_version, srvr->api_version, \
old_api_err, new_api_err); \
}

/*
Expand Down
174 changes: 65 additions & 109 deletions src/lib/kadm5/clnt/client_init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,43 +138,71 @@ kadm5_init_with_skey(krb5_context context, char *client_name,
server_handle);
}

static kadm5_ret_t
free_handle(kadm5_server_handle_t handle)
{
kadm5_ret_t ret = 0;
OM_uint32 minor_stat;
krb5_ccache ccache;

if (handle == NULL)
return 0;

if (handle->destroy_cache && handle->cache_name != NULL) {
ret = krb5_cc_resolve(handle->context, handle->cache_name, &ccache);
if (!ret)
ret = krb5_cc_destroy(handle->context, ccache);
}
free(handle->cache_name);
(void)gss_release_cred(&minor_stat, &handle->cred);
if (handle->clnt != NULL && handle->clnt->cl_auth != NULL)
AUTH_DESTROY(handle->clnt->cl_auth);
if (handle->clnt != NULL)
clnt_destroy(handle->clnt);
if (handle->client_socket != -1)
close(handle->client_socket);
free(handle->lhandle);
kadm5_free_config_params(handle->context, &handle->params);
free(handle);

return ret;
}

static kadm5_ret_t
init_any(krb5_context context, char *client_name, enum init_type init_type,
char *pass, krb5_ccache ccache_in, char *service_name,
kadm5_config_params *params_in, krb5_ui_4 struct_version,
krb5_ui_4 api_version, char **db_args, void **server_handle)
{
int fd = -1;
OM_uint32 minor_stat;
krb5_boolean iprop_enable;
int port;
rpcprog_t rpc_prog;
rpcvers_t rpc_vers;
krb5_ccache ccache;
krb5_principal client = NULL, server = NULL;
struct timeval timeout;

kadm5_server_handle_t handle;
kadm5_server_handle_t handle = NULL;
kadm5_config_params params_local;

int code = 0;
krb5_error_code code;
generic_ret r = { 0, 0 };

initialize_ovk_error_table();
initialize_ovku_error_table();

if (! server_handle) {
if (server_handle == NULL || client_name == NULL)
return EINVAL;
}

if (! (handle = malloc(sizeof(*handle)))) {
return ENOMEM;
}
memset(handle, 0, sizeof(*handle));
if (! (handle->lhandle = malloc(sizeof(*handle)))) {
free(handle);
return ENOMEM;
}
CHECK_VERSIONS(struct_version, api_version, KADM5_OLD_LIB_API_VERSION,
KADM5_NEW_LIB_API_VERSION);

handle = k5alloc(sizeof(*handle), &code);
if (handle == NULL)
goto cleanup;
handle->lhandle = k5alloc(sizeof(*handle), &code);
if (handle->lhandle == NULL)
goto cleanup;

handle->magic_number = KADM5_SERVER_HANDLE_MAGIC;
handle->struct_version = struct_version;
Expand All @@ -192,33 +220,20 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,

handle->context = context;

if(client_name == NULL) {
free(handle);
return EINVAL;
}

/*
* Verify the version numbers before proceeding; we can't use
* CHECK_HANDLE because not all fields are set yet.
*/
GENERIC_CHECK_HANDLE(handle, KADM5_OLD_LIB_API_VERSION,
KADM5_NEW_LIB_API_VERSION);

memset(&params_local, 0, sizeof(params_local));

if ((code = kadm5_get_config_params(handle->context, 0,
params_in, &handle->params))) {
free(handle);
return(code);
}
code = kadm5_get_config_params(handle->context, 0, params_in,
&handle->params);
if (code)
goto cleanup;

#define REQUIRED_PARAMS (KADM5_CONFIG_REALM | \
KADM5_CONFIG_ADMIN_SERVER | \
KADM5_CONFIG_KADMIND_PORT)

if ((handle->params.mask & REQUIRED_PARAMS) != REQUIRED_PARAMS) {
free(handle);
return KADM5_MISSING_KRB5_CONF_PARAMS;
code = KADM5_MISSING_KRB5_CONF_PARAMS;
goto cleanup;
}

/*
Expand All @@ -228,7 +243,7 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
*/
code = krb5_parse_name(handle->context, client_name, &client);
if (code)
goto error;
goto cleanup;
if (init_type == INIT_SKEY && client->realm.length == 0)
client->type = KRB5_NT_SRV_HST;

Expand All @@ -239,7 +254,7 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
code = get_init_creds(handle, client, init_type, pass, ccache_in,
service_name, handle->params.realm, &server);
if (code)
goto error;
goto cleanup;

/* If the service_name and client_name are iprop-centric, use the iprop
* port and RPC identifiers. */
Expand All @@ -258,15 +273,15 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,

code = connect_to_server(handle->params.admin_server, port, &fd);
if (code)
goto error;
goto cleanup;

handle->clnt = clnttcp_create(NULL, rpc_prog, rpc_vers, &fd, 0, 0);
if (handle->clnt == NULL) {
code = KADM5_RPC_ERROR;
#ifdef DEBUG
clnt_pcreateerror("clnttcp_create");
#endif
goto error;
goto cleanup;
}

/* Set a one-hour timeout. */
Expand All @@ -278,26 +293,23 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
handle->lhandle->clnt = handle->clnt;
handle->lhandle->client_socket = fd;

/* now that handle->clnt is set, we can check the handle */
if ((code = _kadm5_check_handle((void *) handle)))
goto error;

/*
* The RPC connection is open; establish the GSS-API
* authentication context.
*/
code = setup_gss(handle, params_in,
(init_type == INIT_CREDS) ? client : NULL, server);
if (code)
goto error;
goto cleanup;

/*
* Bypass the remainder of the code and return straight away
* if the gss service requested is kiprop
*/
if (iprop_enable) {
code = 0;
*server_handle = (void *) handle;
*server_handle = handle;
handle = NULL;
goto cleanup;
}

Expand All @@ -306,7 +318,7 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
#ifdef DEBUG
clnt_perror(handle->clnt, "init_2 null resp");
#endif
goto error;
goto cleanup;
}
/* Drop down to v3 wire protocol if server does not support v4 */
if (r.code == KADM5_NEW_SERVER_API_VERSION &&
Expand All @@ -315,7 +327,7 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
memset(&r, 0, sizeof(generic_ret));
if (init_2(&handle->api_version, &r, handle->clnt)) {
code = KADM5_RPC_ERROR;
goto error;
goto cleanup;
}
}
/* Drop down to v2 wire protocol if server does not support v3 */
Expand All @@ -325,47 +337,21 @@ init_any(krb5_context context, char *client_name, enum init_type init_type,
memset(&r, 0, sizeof(generic_ret));
if (init_2(&handle->api_version, &r, handle->clnt)) {
code = KADM5_RPC_ERROR;
goto error;
goto cleanup;
}
}
if (r.code) {
code = r.code;
goto error;
goto cleanup;
}

*server_handle = (void *) handle;

goto cleanup;

error:
/*
* Note that it is illegal for this code to execute if "handle"
* has not been allocated and initialized. I.e., don't use "goto
* error" before the block of code at the top of the function
* that allocates and initializes "handle".
*/
if (handle->destroy_cache && handle->cache_name) {
if (krb5_cc_resolve(handle->context,
handle->cache_name, &ccache) == 0)
(void) krb5_cc_destroy (handle->context, ccache);
}
if (handle->cache_name)
free(handle->cache_name);
(void)gss_release_cred(&minor_stat, &handle->cred);
if(handle->clnt && handle->clnt->cl_auth)
AUTH_DESTROY(handle->clnt->cl_auth);
if(handle->clnt)
clnt_destroy(handle->clnt);
if (fd != -1)
close(fd);
free(handle->lhandle);
kadm5_free_config_params(handle->context, &handle->params);
*server_handle = handle;
handle = NULL;

cleanup:
krb5_free_principal(handle->context, client);
krb5_free_principal(handle->context, server);
if (code)
free(handle);
krb5_free_principal(context, client);
krb5_free_principal(context, server);
(void)free_handle(handle);

return code;
}
Expand Down Expand Up @@ -695,38 +681,8 @@ rpc_auth(kadm5_server_handle_t handle, kadm5_config_params *params_in,
kadm5_ret_t
kadm5_destroy(void *server_handle)
{
OM_uint32 minor_stat;
krb5_ccache ccache = NULL;
int code = KADM5_OK;
kadm5_server_handle_t handle =
(kadm5_server_handle_t) server_handle;

CHECK_HANDLE(server_handle);

if (handle->destroy_cache && handle->cache_name) {
if ((code = krb5_cc_resolve(handle->context,
handle->cache_name, &ccache)) == 0)
code = krb5_cc_destroy (handle->context, ccache);
}
if (handle->cache_name)
free(handle->cache_name);
if (handle->cred)
(void)gss_release_cred(&minor_stat, &handle->cred);
if (handle->clnt && handle->clnt->cl_auth)
AUTH_DESTROY(handle->clnt->cl_auth);
if (handle->clnt)
clnt_destroy(handle->clnt);
if (handle->client_socket != -1)
close(handle->client_socket);
if (handle->lhandle)
free (handle->lhandle);

kadm5_free_config_params(handle->context, &handle->params);

handle->magic_number = 0;
free(handle);

return code;
return free_handle(server_handle);
}
/* not supported on client */
kadm5_ret_t kadm5_lock(void *server_handle)
Expand Down

0 comments on commit 552d7b7

Please sign in to comment.