From 0cef2b5bbc7c1f4e4ad7a1b5cb727e1e00988d9e Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 14:32:09 -0400 Subject: [PATCH 01/12] Silence gcc on truncation of debug messages Also improve the DEBUG by reporting that messages were truncated. Signed-off-by: Simo Sorce Reviewed-by: Robbie Harwood --- tests/t_utils.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/t_utils.h b/tests/t_utils.h index 04e2d941a..58e378f11 100644 --- a/tests/t_utils.h +++ b/tests/t_utils.h @@ -15,9 +15,11 @@ #define discard_const(ptr) ((void *)((uintptr_t)(ptr))) #define DEBUG(...) do { \ - char msg[4096]; \ - snprintf(msg, 4096, __VA_ARGS__); \ - fprintf(stderr, "%s[%s:%d]: %s", argv[0], __FUNCTION__, __LINE__, msg); \ + char _msg[4096]; \ + int _snret; \ + _snret = snprintf(_msg, 4096, __VA_ARGS__); \ + fprintf(stderr, "%s[%s:%d]: %s", argv[0], __FUNCTION__, __LINE__, _msg); \ + if (_snret >= 4096) fprintf(stderr, " [TRUNCATED]\n"); \ fflush(stderr); \ } while(0); From fe52166eceb02907ee4ea2b3bf6fb18dce295b29 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 17:01:12 -0400 Subject: [PATCH 02/12] Work around incorrect gcc restrict warning on asprintf Signed-off-by: Simo Sorce [rharwood@redhat.com: edit commit message] Reviewed-by: Robbie Harwood --- src/gp_creds.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/gp_creds.c b/src/gp_creds.c index 04c84f3bc..424cf3521 100644 --- a/src/gp_creds.c +++ b/src/gp_creds.c @@ -775,6 +775,7 @@ static uint32_t get_impersonator_fallback(uint32_t *min, gss_cred_id_t cred, uint32_t ret_maj = 0; uint32_t ret_min = 0; char *memcache = NULL; + char **ptr = &memcache; krb5_context context = NULL; krb5_ccache ccache = NULL; krb5_data config; @@ -791,7 +792,7 @@ static uint32_t get_impersonator_fallback(uint32_t *min, gss_cred_id_t cred, gss_key_value_element_desc ccelement = { "ccache", NULL }; gss_key_value_set_desc cred_store = { 1, &ccelement }; - err = asprintf(&memcache, "MEMORY:cred_allowed_%p", &memcache); + err = asprintf(&memcache, "MEMORY:cred_allowed_%p", ptr); if (err == -1) { memcache = NULL; ret_min = ENOMEM; @@ -991,6 +992,7 @@ uint32_t gp_count_tickets(uint32_t *min, gss_cred_id_t cred, uint32_t *ccsum) uint32_t ret_maj = 0; uint32_t ret_min = 0; char *memcache = NULL; + char **ptr = &memcache; krb5_context context = NULL; krb5_ccache ccache = NULL; krb5_cc_cursor cursor = NULL; @@ -1008,7 +1010,7 @@ uint32_t gp_count_tickets(uint32_t *min, gss_cred_id_t cred, uint32_t *ccsum) gss_key_value_element_desc ccelement = { "ccache", NULL }; gss_key_value_set_desc cred_store = { 1, &ccelement }; - err = asprintf(&memcache, "MEMORY:cred_allowed_%p", &memcache); + err = asprintf(&memcache, "MEMORY:cred_allowed_%p", ptr); if (err == -1) { memcache = NULL; ret_min = ENOMEM; From 182983129c1cbd312b04202480e81652159b34ff Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Wed, 26 Aug 2020 17:54:21 -0400 Subject: [PATCH 03/12] Add testlib method to wait for gssproxy reconfiguration Add a more robust method to wait until gssproxy has reconfigured. This should work better when tests ar run on slow machines or through tools like valgrind which can considerably slow down the tooling. Also increase timeouts to 30 seconds for said slower machines. Signed-off-by: Simo Sorce [rharwood@redhat.com: commit message tweaks] Reviewed-by: Robbie Harwood --- src/gssproxy.c | 16 ++++++++++++++++ tests/runtests.py | 9 ++------- tests/t_impersonate.py | 6 ++---- tests/t_multi_key.py | 6 ++---- tests/t_program.py | 9 +++------ tests/t_reloading.py | 15 +++++---------- tests/testlib.py | 39 +++++++++++++++++++++++++++++++++++++-- 7 files changed, 67 insertions(+), 33 deletions(-) mode change 100644 => 100755 tests/t_program.py diff --git a/src/gssproxy.c b/src/gssproxy.c index 82b6de522..b681b5a54 100644 --- a/src/gssproxy.c +++ b/src/gssproxy.c @@ -150,6 +150,11 @@ static void hup_handler(verto_ctx *vctx, verto_ev *ev UNUSED) return; } +static void init_event(verto_ctx *vctx UNUSED, verto_ev *ev UNUSED) +{ + GPDEBUG("Initialization complete.\n"); +} + int main(int argc, const char *argv[]) { int opt; @@ -291,6 +296,17 @@ int main(int argc, const char *argv[]) goto cleanup; } + /* Schedule an event to run as soon as the event loop is started + * This is useful in debug to know that all initialization is done. + * Might be used in future to schdule startup one offs that do not + * need to be done synchronously */ + ev = verto_add_timeout(vctx, VERTO_EV_FLAG_NONE, init_event, 1); + if (!ev) { + fprintf(stderr, "Failed to register init_event with verto!\n"); + ret = EXIT_FAILURE; + goto cleanup; + } + verto_run(vctx); verto_free(vctx); diff --git a/tests/runtests.py b/tests/runtests.py index b9a2bb644..81a1de354 100755 --- a/tests/runtests.py +++ b/tests/runtests.py @@ -32,7 +32,7 @@ def parse_args(): "--args") parser.add_argument('--debug-num', default=-1, type=int, help="Specify the testcase number to debug") - parser.add_argument('--timeout', default=15, type=int, + parser.add_argument('--timeout', default=30, type=int, help="Specify test case timeout limit") parser.add_argument('--valgrind-cmd', default="valgrind " + "--track-origins=yes", @@ -83,15 +83,10 @@ def runtests_main(testfiles): if 'TERM' in os.environ: gssapienv['TERM'] = os.environ['TERM'] - gssproxylog = os.path.join(testdir, 'gssproxy.log') - - logfile = open(gssproxylog, "a") - gssproxyenv = keysenv gssproxyenv['KRB5_TRACE'] = os.path.join(testdir, 'gssproxy.trace') - gproc, gpsocket = setup_gssproxy(testdir, logfile, gssproxyenv) - time.sleep(5) #Give time to gssproxy to fully start up + gproc, gpsocket = setup_gssproxy(testdir, gssproxyenv) processes['GSS-Proxy(%d)' % gproc.pid] = gproc gssapienv['GSSPROXY_SOCKET'] = gpsocket diff --git a/tests/t_impersonate.py b/tests/t_impersonate.py index db0fe9e8c..c9f88efdf 100755 --- a/tests/t_impersonate.py +++ b/tests/t_impersonate.py @@ -56,8 +56,7 @@ def run(testdir, env, conf): keysenv = conf["keysenv"].copy() keysenv['KRB5_KTNAME'] = os.path.join(testdir, PROXY_KTNAME) update_gssproxy_conf(testdir, keysenv, IMPERSONATE_CONF_TEMPLATE) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, conf['gpid']) rets = [] @@ -119,8 +118,7 @@ def run(testdir, env, conf): # Reset back gssproxy conf update_gssproxy_conf(testdir, keysenv, GSSPROXY_CONF_TEMPLATE) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, conf['gpid']) e = [r for r in rets if r != 0] if len(e) > 0: diff --git a/tests/t_multi_key.py b/tests/t_multi_key.py index c08930d7e..e3ce0afd1 100755 --- a/tests/t_multi_key.py +++ b/tests/t_multi_key.py @@ -33,8 +33,7 @@ def run(testdir, env, conf): p1env['client_name'] = MULTI_UPN p1env['KRB5_KTNAME'] = os.path.join(testdir, MULTI_KTNAME) update_gssproxy_conf(testdir, p1env, GSSPROXY_MULTI_TEMPLATE) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, conf['gpid']) r1 = run_basic_test(testdir, env, conf) print("Testing multiple keys Keytab with second principal", @@ -48,8 +47,7 @@ def run(testdir, env, conf): p2env['client_name'] = MULTI_SVC p2env['KRB5_KTNAME'] = os.path.join(testdir, MULTI_KTNAME) update_gssproxy_conf(testdir, p2env, GSSPROXY_MULTI_TEMPLATE) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, conf['gpid']) r2 = run_basic_test(testdir, env, conf) if r1 != 0: diff --git a/tests/t_program.py b/tests/t_program.py old mode 100644 new mode 100755 index 37b0c6080..0c947e010 --- a/tests/t_program.py +++ b/tests/t_program.py @@ -30,8 +30,7 @@ def run(testdir, env, conf): sys.stderr.write(" ") conf["prefix"] = prefix + "_1" update_gssproxy_conf(testdir, conf["keysenv"], GSSPROXY_PROGRAM) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) + gssproxy_reload(testdir, conf['gpid']) retval |= run_acquire_test(testdir, env, conf) print("Testing negative program name matching...", file=sys.stderr) @@ -39,14 +38,12 @@ def run(testdir, env, conf): conf["prefix"] = prefix + "_2" bad_progdir = GSSPROXY_PROGRAM.replace("${PROGDIR}", "//bad/path") update_gssproxy_conf(testdir, conf["keysenv"], bad_progdir) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) + gssproxy_reload(testdir, conf['gpid']) retval |= run_acquire_test(testdir, env, conf, expected_failure=True) # be a good citizen and clean up after ourselves update_gssproxy_conf(testdir, conf["keysenv"], GSSPROXY_CONF_TEMPLATE) - os.kill(conf["gpid"], signal.SIGHUP) - time.sleep(1) + gssproxy_reload(testdir, conf['gpid']) print_return(retval, -1, "Program", False) return retval diff --git a/tests/t_reloading.py b/tests/t_reloading.py index e49f6839d..88bfcd856 100755 --- a/tests/t_reloading.py +++ b/tests/t_reloading.py @@ -14,8 +14,7 @@ def run(testdir, env, basicconf): print("Testing basic SIGHUP with no change", file=sys.stderr) sys.stderr.write(" ") basicconf['prefix'] += prefix + "_1" - os.kill(basicconf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, basicconf['gpid']) r = run_basic_test(testdir, env, basicconf) rets.append(r) @@ -23,8 +22,7 @@ def run(testdir, env, basicconf): sys.stderr.write(" ") basicconf['prefix'] = prefix + "_2" update_gssproxy_conf(testdir, keysenv, GSSPROXY_CONF_MINIMAL_TEMPLATE) - os.kill(basicconf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, basicconf['gpid']) r = run_basic_test(testdir, env, basicconf, True) rets.append(r) @@ -32,8 +30,7 @@ def run(testdir, env, basicconf): sys.stderr.write(" ") basicconf['prefix'] = prefix + "_3" update_gssproxy_conf(testdir, keysenv, GSSPROXY_CONF_TEMPLATE) - os.kill(basicconf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, basicconf['gpid']) r = run_basic_test(testdir, env, basicconf) rets.append(r) @@ -42,16 +39,14 @@ def run(testdir, env, basicconf): basicconf['prefix'] = prefix + "_4" update_gssproxy_conf(testdir, keysenv, GSSPROXY_CONF_SOCKET_TEMPLATE) env['GSSPROXY_SOCKET'] += "2" - os.kill(basicconf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, basicconf['gpid']) r = run_basic_test(testdir, env, basicconf) rets.append(r) # restore old configuration env['GSSPROXY_SOCKET'] = env['GSSPROXY_SOCKET'][:-1] update_gssproxy_conf(testdir, keysenv, GSSPROXY_CONF_TEMPLATE) - os.kill(basicconf["gpid"], signal.SIGHUP) - time.sleep(1) #Let gssproxy reload everything + gssproxy_reload(testdir, basicconf['gpid']) e = [r for r in rets if r != 0] if len(e) > 0: diff --git a/tests/testlib.py b/tests/testlib.py index b722e5ff1..247b2bc61 100755 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -11,7 +11,7 @@ import sys import time -testcase_wait = 15 +testcase_wait = 30 cmd_index = 0 debug_all = False @@ -717,9 +717,12 @@ def update_gssproxy_conf(testdir, env, template): with open(conf, 'w+') as f: f.write(text) -def setup_gssproxy(testdir, logfile, env): +def setup_gssproxy(testdir, env): global debug_gssproxy, valgrind_cmd + gssproxylog = os.path.join(testdir, 'gssproxy.log') + logfile = open(gssproxylog, "a") + gssproxy = os.path.join(testdir, 'gssproxy') if os.path.exists(gssproxy): shutil.rmtree(gssproxy) @@ -750,4 +753,36 @@ def setup_gssproxy(testdir, logfile, env): print("Attach and start debugging, then press enter to continue.") input() + gssproxy_reload(testdir, gproc.pid, { + 'signal': 0 , 'key': 'Initialization complete' }) + return gproc, socket + +def gssproxy_reload(testdir, pid, action=None): + + if action is None: + action = dict() + sig = action.get('signal', signal.SIGHUP) + key = action.get('key', 'New config loaded successfully') + + gssproxylog = os.path.join(testdir, 'gssproxy.log') + logsize = os.stat(gssproxylog).st_size + + # Send signal ... + os.kill(pid, sig) + + if key == '': + return + + # ... and Wait for the reload to happen + with open(gssproxylog, 'r') as logfile: + logfile.seek(logsize) + # Try for 30 seconds max in increments of 100ms + for retries in range(0, 300): + lines = logfile.readlines() + for line in lines: + if key in line: + return + time.sleep(0.1) + + raise Exception("timed out while waiting for gssproxy to reload") From a3f13b30ef3c90ff7344c3913f6e26e55b82451f Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 11:34:45 -0400 Subject: [PATCH 04/12] Expand use of global static mechs to conform to SPI GSSAPI requires some specific APIs to return "static" OIDs that the user does not have to free. The krb5 mechglue in fact requires mechanisms to also honor this or the mech oid will be irretrievably leaked in some cases. To accomodate this, expand use of global mechs structure we already allocate for the gss_inidicate_mechs case so we can return "static" OIDs from calls like ISC and ASC. Signed-off-by: Simo Sorce [rharwood@redhat.com: commit message fixups] Reviewed-by: Robbie Harwood --- src/client/gpm_accept_sec_context.c | 22 ++++++------------- src/client/gpm_common.c | 1 - src/client/gpm_indicate_mechs.c | 34 +++++++++++++++++++++++++++++ src/client/gpm_init_sec_context.c | 19 +++++----------- src/client/gssapi_gpm.h | 3 +++ src/mechglue/gss_plugin.c | 5 +++++ 6 files changed, 55 insertions(+), 29 deletions(-) diff --git a/src/client/gpm_accept_sec_context.c b/src/client/gpm_accept_sec_context.c index ef5e79c17..ab20b03df 100644 --- a/src/client/gpm_accept_sec_context.c +++ b/src/client/gpm_accept_sec_context.c @@ -21,7 +21,6 @@ OM_uint32 gpm_accept_sec_context(OM_uint32 *minor_status, gssx_res_accept_sec_context *res = &ures.accept_sec_context; gssx_ctx *ctx = NULL; gssx_name *name = NULL; - gss_OID_desc *mech = NULL; gss_buffer_t outbuf = NULL; uint32_t ret_maj; int ret; @@ -70,15 +69,6 @@ OM_uint32 gpm_accept_sec_context(OM_uint32 *minor_status, goto done; } - if (mech_type) { - if (res->status.mech.octet_string_len) { - ret = gp_conv_gssx_to_oid_alloc(&res->status.mech, &mech); - if (ret) { - goto done; - } - } - } - ctx = res->context_handle; /* we are stealing the delegated creds on success, so we do not want * it to be freed by xdr_free */ @@ -101,8 +91,14 @@ OM_uint32 gpm_accept_sec_context(OM_uint32 *minor_status, } if (mech_type) { - *mech_type = mech; + gss_OID_desc mech; + gp_conv_gssx_to_oid(&res->status.mech, &mech); + ret = gpm_mech_to_static(&mech, mech_type); + if (ret) { + goto done; + } } + if (src_name) { *src_name = name; } @@ -145,10 +141,6 @@ OM_uint32 gpm_accept_sec_context(OM_uint32 *minor_status, xdr_free((xdrproc_t)xdr_gssx_name, (char *)name); free(name); } - if (mech) { - free(mech->elements); - free(mech); - } if (outbuf) { free(outbuf->value); free(outbuf); diff --git a/src/client/gpm_common.c b/src/client/gpm_common.c index 786a77b21..820243efe 100644 --- a/src/client/gpm_common.c +++ b/src/client/gpm_common.c @@ -799,4 +799,3 @@ void gpm_free_xdrs(int proc, union gp_rpc_arg *arg, union gp_rpc_res *res) xdr_free(gpm_xdr_set[proc].arg_fn, (char *)arg); xdr_free(gpm_xdr_set[proc].res_fn, (char *)res); } - diff --git a/src/client/gpm_indicate_mechs.c b/src/client/gpm_indicate_mechs.c index b019a9628..86c7de365 100644 --- a/src/client/gpm_indicate_mechs.c +++ b/src/client/gpm_indicate_mechs.c @@ -300,6 +300,40 @@ static int gpmint_init_global_mechs(void) return 0; } +/* GSSAPI requires some APIs to return "static" mechs that callers do not need + * to free. So match a radom mech and return from our global "static" array */ +int gpm_mech_to_static(gss_OID mech_type, gss_OID *mech_static) +{ + int ret; + + ret = gpmint_init_global_mechs(); + if (ret) { + return ret; + } + + *mech_static = GSS_C_NO_OID; + for (size_t i = 0; i < global_mechs.mech_set->count; i++) { + if (gpm_equal_oids(&global_mechs.mech_set->elements[i], mech_type)) { + *mech_static = &global_mechs.mech_set->elements[i]; + return 0; + } + } + /* TODO: potentially in future add the mech to the list if missing */ + return ENOENT; +} + +bool gpm_mech_is_static(gss_OID mech_type) +{ + if (global_mechs.mech_set) { + for (size_t i = 0; i < global_mechs.mech_set->count; i++) { + if (&global_mechs.mech_set->elements[i] == mech_type) { + return true; + } + } + } + return false; +} + OM_uint32 gpm_indicate_mechs(OM_uint32 *minor_status, gss_OID_set *mech_set) { uint32_t ret_min; diff --git a/src/client/gpm_init_sec_context.c b/src/client/gpm_init_sec_context.c index bea201050..b84ff946f 100644 --- a/src/client/gpm_init_sec_context.c +++ b/src/client/gpm_init_sec_context.c @@ -43,7 +43,6 @@ OM_uint32 gpm_init_sec_context(OM_uint32 *minor_status, gssx_arg_init_sec_context *arg = &uarg.init_sec_context; gssx_res_init_sec_context *res = &ures.init_sec_context; gssx_ctx *ctx = NULL; - gss_OID_desc *mech = NULL; gss_buffer_t outbuf = NULL; uint32_t ret_maj = GSS_S_COMPLETE; uint32_t ret_min = 0; @@ -100,11 +99,12 @@ OM_uint32 gpm_init_sec_context(OM_uint32 *minor_status, /* return values */ if (actual_mech_type) { - if (res->status.mech.octet_string_len) { - ret = gp_conv_gssx_to_oid_alloc(&res->status.mech, &mech); - if (ret) { - goto done; - } + gss_OID_desc mech; + gp_conv_gssx_to_oid(&res->status.mech, &mech); + ret = gpm_mech_to_static(&mech, actual_mech_type); + if (ret) { + gpm_save_internal_status(ret, gp_strerror(ret)); + goto done; } } @@ -151,9 +151,6 @@ OM_uint32 gpm_init_sec_context(OM_uint32 *minor_status, gpm_free_xdrs(GSSX_INIT_SEC_CONTEXT, &uarg, &ures); if (ret_maj == GSS_S_COMPLETE || ret_maj == GSS_S_CONTINUE_NEEDED) { - if (actual_mech_type) { - *actual_mech_type = mech; - } if (outbuf) { *output_token = *outbuf; free(outbuf); @@ -170,10 +167,6 @@ OM_uint32 gpm_init_sec_context(OM_uint32 *minor_status, free(ctx); ctx = NULL; } - if (mech) { - free(mech->elements); - free(mech); - } if (outbuf) { free(outbuf->value); free(outbuf); diff --git a/src/client/gssapi_gpm.h b/src/client/gssapi_gpm.h index 61124e029..b7ba04b1f 100644 --- a/src/client/gssapi_gpm.h +++ b/src/client/gssapi_gpm.h @@ -27,6 +27,9 @@ void gpm_display_status_init_once(void); void gpm_save_status(gssx_status *status); void gpm_save_internal_status(uint32_t err, char *err_str); +int gpm_mech_to_static(gss_OID mech_type, gss_OID *mech_static); +bool gpm_mech_is_static(gss_OID mech_type); + OM_uint32 gpm_display_status(OM_uint32 *minor_status, OM_uint32 status_value, int status_type, diff --git a/src/mechglue/gss_plugin.c b/src/mechglue/gss_plugin.c index 9ce3e15c4..8f401e9a5 100644 --- a/src/mechglue/gss_plugin.c +++ b/src/mechglue/gss_plugin.c @@ -376,6 +376,11 @@ OM_uint32 gssi_internal_release_oid(OM_uint32 *minor_status, gss_OID *oid) item = gpp_next_special_oids(item); } + if (gpm_mech_is_static(*oid)) { + *oid = GSS_C_NO_OID; + return GSS_S_COMPLETE; + } + /* none matched, it's not ours */ return GSS_S_CONTINUE_NEEDED; } From 447d5352c2a81e219ccf04348a87b2ff25b7de15 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 12:32:06 -0400 Subject: [PATCH 05/12] Initialize interposed mech list without allocation While we had already fixed the leak here in main, the code performed unnecessary extra work, so just replacethe whole lot with a function that does not do any extra allocation or copy. Signed-off-by: Simo Sorce [rharwood@redhat.com: commit message] Reviewed-by: Robbie Harwood --- src/mechglue/gss_plugin.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/mechglue/gss_plugin.c b/src/mechglue/gss_plugin.c index 8f401e9a5..5767f4da4 100644 --- a/src/mechglue/gss_plugin.c +++ b/src/mechglue/gss_plugin.c @@ -65,6 +65,8 @@ enum gpp_behavior gpp_get_behavior(void) return behavior; } +static void gpp_init_special_available_mechs(const gss_OID_set mechs); + /* 2.16.840.1.113730.3.8.15.1 */ const gss_OID_desc gssproxy_mech_interposer = { .length = 11, @@ -76,7 +78,6 @@ gss_OID_set gss_mech_interposer(gss_OID mech_type) gss_OID_set interposed_mechs; OM_uint32 maj, min; char *envval; - gss_OID_set special_mechs; /* avoid looping in the gssproxy daemon by avoiding to interpose * any mechanism */ @@ -119,8 +120,7 @@ gss_OID_set gss_mech_interposer(gss_OID mech_type) } /* while there also initiaize special_mechs */ - special_mechs = gpp_special_available_mechs(interposed_mechs); - (void)gss_release_oid_set(&min, &special_mechs); + gpp_init_special_available_mechs(interposed_mechs); done: if (maj != 0) { @@ -307,13 +307,13 @@ gss_OID_set gpp_special_available_mechs(const gss_OID_set mechs) gss_OID n; uint32_t maj, min; - item = gpp_get_special_oids(); - maj = gss_create_empty_oid_set(&min, &amechs); if (maj) { return GSS_C_NO_OID_SET; } for (size_t i = 0; i < mechs->count; i++) { + item = gpp_get_special_oids(); + while (item) { if (gpp_is_special_oid(&mechs->elements[i])) { maj = gss_add_oid_set_member(&min, @@ -354,6 +354,27 @@ gss_OID_set gpp_special_available_mechs(const gss_OID_set mechs) return amechs; } +static void gpp_init_special_available_mechs(const gss_OID_set mechs) +{ + struct gpp_special_oid_list *item; + + for (size_t i = 0; i < mechs->count; i++) { + item = gpp_get_special_oids(); + + while (item) { + if (gpp_is_special_oid(&mechs->elements[i]) || + gpp_special_equal(&item->special_oid, &mechs->elements[i])) { + break; + } + item = gpp_next_special_oids(item); + } + if (item == NULL) { + /* not found, add to static list */ + (void)gpp_new_special_mech(&mechs->elements[i]); + } + } +} + OM_uint32 gssi_internal_release_oid(OM_uint32 *minor_status, gss_OID *oid) { struct gpp_special_oid_list *item = NULL; From e6811347c23b6c62d9f1869da089ab9900f97a84 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 12:44:45 -0400 Subject: [PATCH 06/12] Make sure to free also the remote ctx struct The xdr_free() call only frees the contents and not the containing structure itself. Signed-off-by: Simo Sorce Reviewed-by: Robbie Harwood --- src/client/gpm_release_handle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/client/gpm_release_handle.c b/src/client/gpm_release_handle.c index 8f49ee919..2f707811a 100644 --- a/src/client/gpm_release_handle.c +++ b/src/client/gpm_release_handle.c @@ -106,5 +106,7 @@ OM_uint32 gpm_delete_sec_context(OM_uint32 *minor_status, gpm_free_xdrs(GSSX_RELEASE_HANDLE, &uarg, &ures); done: xdr_free((xdrproc_t)xdr_gssx_ctx, (char *)r); + free(r); + *context_handle = NULL; return ret; } From a2ffd1230fd572d7fa9099af2365dfb7ac394d07 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 13:20:49 -0400 Subject: [PATCH 07/12] Use the correct function to free unused creds Signed-off-by: Simo Sorce Reviewed-by: Robbie Harwood --- src/mechglue/gpp_creds.c | 2 +- src/mechglue/gpp_init_sec_context.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mechglue/gpp_creds.c b/src/mechglue/gpp_creds.c index e87da8203..338fadd96 100644 --- a/src/mechglue/gpp_creds.c +++ b/src/mechglue/gpp_creds.c @@ -895,7 +895,7 @@ OM_uint32 gssi_import_cred_by_mech(OM_uint32 *minor_status, if (maj == GSS_S_COMPLETE) { *cred_handle = (gss_cred_id_t)cred; } else { - free(cred); + (void)gpp_cred_handle_free(&min, cred); } (void)gss_release_buffer(&min, &wrap_token); return maj; diff --git a/src/mechglue/gpp_init_sec_context.c b/src/mechglue/gpp_init_sec_context.c index 94d9b01f2..bb878dfeb 100644 --- a/src/mechglue/gpp_init_sec_context.c +++ b/src/mechglue/gpp_init_sec_context.c @@ -215,7 +215,7 @@ OM_uint32 gssi_init_sec_context(OM_uint32 *minor_status, *context_handle = (gss_ctx_id_t)ctx_handle; if (claimant_cred_handle == GSS_C_NO_CREDENTIAL) { - free(cred_handle); + (void)gpp_cred_handle_free(&min, cred_handle); } return maj; } From dc56c86f1dcb1ae4dbc35facf5f50fb21c9d5049 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 13:23:49 -0400 Subject: [PATCH 08/12] Fix leaks in our test suite itself These are mostly laziness in freeing since the programs are short-lived. Signed-off-by: Simo Sorce [rharwood@redhat.com: rewrote commit message] Reviewed-by: Robbie Harwood --- tests/interposetest.c | 22 +++++++++++++++------- tests/t_impersonate.c | 11 ++++++++--- tests/t_init.c | 2 ++ tests/t_setcredopt.c | 8 ++++++-- 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/tests/interposetest.c b/tests/interposetest.c index a00904fb4..0cdd473f0 100644 --- a/tests/interposetest.c +++ b/tests/interposetest.c @@ -71,6 +71,8 @@ static int gptest_inq_context(gss_ctx_id_t ctx) DEBUG("Context validity: %d sec.\n", time_rec); done: + (void)gss_release_name(&min, &src_name); + (void)gss_release_name(&min, &targ_name); (void)gss_release_buffer(&min, &sname); (void)gss_release_buffer(&min, &tname); (void)gss_release_buffer(&min, &mechstr); @@ -274,7 +276,7 @@ void run_client(struct aproc *data) gp_log_failure(GSS_C_NO_OID, ret_maj, ret_min); goto done; } - fprintf(stdout, "Client, RECV: [%s]\n", buffer); + fprintf(stdout, "Client, RECV: [%*s]\n", buflen, buffer); /* test gss_wrap_iov_length */ @@ -837,19 +839,22 @@ int main(int argc, const char *main_argv[]) if (opt_version) { puts(VERSION""DISTRO_VERSION""PRERELEASE_VERSION); - return 0; + ret = 0; + goto done; } if (opt_target == NULL) { fprintf(stderr, "Missing target!\n"); poptPrintUsage(pc, stderr, 0); - return 1; + ret = 1; + goto done; } if (!opt_all) { - return run_cli_srv_test(PROXY_LOCAL_ONLY, - PROXY_LOCAL_ONLY, - opt_target); + ret = run_cli_srv_test(PROXY_LOCAL_ONLY, + PROXY_LOCAL_ONLY, + opt_target); + goto done; } for (i=0; i<4; i++) { @@ -861,10 +866,13 @@ int main(int argc, const char *main_argv[]) lookup_gssproxy_behavior(k), ret ? "failed" : "succeeded"); if (ret) { - return ret; + goto done; } } } +done: + poptFreeContext(pc); + free(opt_target); return ret; } diff --git a/tests/t_impersonate.c b/tests/t_impersonate.c index 8ca6e9c0b..e7b0bc2b8 100644 --- a/tests/t_impersonate.c +++ b/tests/t_impersonate.c @@ -12,9 +12,9 @@ int main(int argc, const char *argv[]) gss_ctx_id_t accept_ctx = GSS_C_NO_CONTEXT; gss_buffer_desc in_token = GSS_C_EMPTY_BUFFER; gss_buffer_desc out_token = GSS_C_EMPTY_BUFFER; - gss_name_t user_name; - gss_name_t proxy_name; - gss_name_t target_name; + gss_name_t user_name = GSS_C_NO_NAME; + gss_name_t proxy_name = GSS_C_NO_NAME; + gss_name_t target_name = GSS_C_NO_NAME; gss_OID_set_desc oid_set = { 1, discard_const(gss_mech_krb5) }; uint32_t ret_maj; uint32_t ret_min; @@ -207,9 +207,14 @@ int main(int argc, const char *argv[]) ret = 0; done: + gss_release_name(&ret_min, &user_name); + gss_release_name(&ret_min, &proxy_name); + gss_release_name(&ret_min, &target_name); gss_release_buffer(&ret_min, &in_token); gss_release_buffer(&ret_min, &out_token); gss_release_cred(&ret_min, &impersonator_cred_handle); gss_release_cred(&ret_min, &cred_handle); + gss_delete_sec_context(&ret_min, &accept_ctx, GSS_C_NO_BUFFER); + gss_delete_sec_context(&ret_min, &init_ctx, GSS_C_NO_BUFFER); return ret; } diff --git a/tests/t_init.c b/tests/t_init.c index 02407cea0..76bd4c158 100644 --- a/tests/t_init.c +++ b/tests/t_init.c @@ -82,6 +82,8 @@ int main(int argc, const char *argv[]) goto done; } + gss_release_buffer(&ret_min, &out_token); + ret = t_recv_buffer(STDIN_FD, buffer, &buflen); if (ret != 0) { DEBUG("Failed to read token from STDIN\n"); diff --git a/tests/t_setcredopt.c b/tests/t_setcredopt.c index 13994741f..bc5e13ff8 100644 --- a/tests/t_setcredopt.c +++ b/tests/t_setcredopt.c @@ -12,8 +12,8 @@ int main(int argc, const char *argv[]) gss_ctx_id_t accept_ctx = GSS_C_NO_CONTEXT; gss_buffer_desc in_token = GSS_C_EMPTY_BUFFER; gss_buffer_desc out_token = GSS_C_EMPTY_BUFFER; - gss_name_t user_name; - gss_name_t target_name; + gss_name_t user_name = GSS_C_NO_NAME; + gss_name_t target_name = GSS_C_NO_NAME; gss_OID_set_desc oid_set = { 1, discard_const(gss_mech_krb5) }; uint32_t ret_maj; uint32_t ret_min; @@ -160,8 +160,12 @@ int main(int argc, const char *argv[]) ret = 0; done: + gss_release_name(&ret_min, &user_name); + gss_release_name(&ret_min, &target_name); gss_release_buffer(&ret_min, &in_token); gss_release_buffer(&ret_min, &out_token); gss_release_cred(&ret_min, &cred_handle); + gss_delete_sec_context(&ret_min, &init_ctx, GSS_C_NO_BUFFER); + gss_delete_sec_context(&ret_min, &accept_ctx, GSS_C_NO_BUFFER); return ret; } From fe9e3c29caab90daf19028fb31ff28622d8708a9 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 15:35:40 -0400 Subject: [PATCH 09/12] Always free ciphertext data in gp_encrypt_buffer Signed-off-by: Simo Sorce [rharwood@redhat.com: rewrote commit message] Reviewed-by: Robbie Harwood --- src/gp_export.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/gp_export.c b/src/gp_export.c index a5681c01b..fb2f81b37 100644 --- a/src/gp_export.c +++ b/src/gp_export.c @@ -308,10 +308,9 @@ static int gp_encrypt_buffer(krb5_context context, krb5_keyblock *key, ret = gp_conv_octet_string(enc_handle.ciphertext.length, enc_handle.ciphertext.data, out); - if (ret) { - free(enc_handle.ciphertext.data); - goto done; - } + /* the conversion function copies the data, so free our copy + * unconditionally, or we leak */ + free(enc_handle.ciphertext.data); done: free(padded); From 6ea8391257e687dfb3981b634c06cf7a55008eb0 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 17:01:39 -0400 Subject: [PATCH 10/12] Return static oids for naming functions gss_display_name and gss_inquire_name reteurn "static" oids, that are generally not freed by callers, so make sure to match and return actual static OIDs exported by GSSAPI. Also remove gpm_equal_oids() and use the library provided gss_oid_equal function instead. Signed-off-by: Simo Sorce Reviewed-by: Robbie Harwood --- src/client/gpm_import_and_canon_name.c | 28 ++++++++++++++++++++++++-- src/client/gpm_indicate_mechs.c | 24 +++++----------------- src/client/gssapi_gpm.h | 1 + 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/client/gpm_import_and_canon_name.c b/src/client/gpm_import_and_canon_name.c index 70149a33f..88b8d7c8b 100644 --- a/src/client/gpm_import_and_canon_name.c +++ b/src/client/gpm_import_and_canon_name.c @@ -2,6 +2,26 @@ #include "gssapi_gpm.h" +static int gpm_name_oid_to_static(gss_OID name_type, gss_OID *name_static) +{ +#define ret_static(b) \ + if (gss_oid_equal(name_type, b)) { \ + *name_static = b; \ + return 0; \ + } + ret_static(GSS_C_NT_USER_NAME); + ret_static(GSS_C_NT_MACHINE_UID_NAME); + ret_static(GSS_C_NT_STRING_UID_NAME); + ret_static(GSS_C_NT_HOSTBASED_SERVICE_X); + ret_static(GSS_C_NT_HOSTBASED_SERVICE); + ret_static(GSS_C_NT_ANONYMOUS); + ret_static(GSS_C_NT_EXPORT_NAME); + ret_static(GSS_C_NT_COMPOSITE_EXPORT); + ret_static(GSS_KRB5_NT_PRINCIPAL_NAME); + ret_static(gss_nt_krb5_name); + return ENOENT; +} + OM_uint32 gpm_display_name(OM_uint32 *minor_status, gssx_name *in_name, gss_buffer_t output_name_buffer, @@ -57,7 +77,9 @@ OM_uint32 gpm_display_name(OM_uint32 *minor_status, } if (output_name_type) { - ret = gp_conv_gssx_to_oid_alloc(&in_name->name_type, output_name_type); + gss_OID_desc oid; + gp_conv_gssx_to_oid(&in_name->name_type, &oid); + ret = gpm_name_oid_to_static(&oid, output_name_type); if (ret) { gss_release_buffer(&discard, output_name_buffer); ret_min = ret; @@ -285,7 +307,9 @@ OM_uint32 gpm_inquire_name(OM_uint32 *minor_status, } if (MN_mech != NULL) { - ret = gp_conv_gssx_to_oid_alloc(&name->name_type, MN_mech); + gss_OID_desc oid; + gp_conv_gssx_to_oid(&name->name_type, &oid); + ret = gpm_name_oid_to_static(&oid, MN_mech); if (ret) { *minor_status = ret; return GSS_S_FAILURE; diff --git a/src/client/gpm_indicate_mechs.c b/src/client/gpm_indicate_mechs.c index 86c7de365..4041dcd65 100644 --- a/src/client/gpm_indicate_mechs.c +++ b/src/client/gpm_indicate_mechs.c @@ -95,20 +95,6 @@ static uint32_t gpm_copy_gss_buffer(uint32_t *minor_status, return GSS_S_COMPLETE; } -static bool gpm_equal_oids(gss_const_OID a, gss_const_OID b) -{ - int ret; - - if (a->length == b->length) { - ret = memcmp(a->elements, b->elements, a->length); - if (ret == 0) { - return true; - } - } - - return false; -} - static void gpmint_indicate_mechs(void) { union gp_rpc_arg uarg; @@ -313,7 +299,7 @@ int gpm_mech_to_static(gss_OID mech_type, gss_OID *mech_static) *mech_static = GSS_C_NO_OID; for (size_t i = 0; i < global_mechs.mech_set->count; i++) { - if (gpm_equal_oids(&global_mechs.mech_set->elements[i], mech_type)) { + if (gss_oid_equal(&global_mechs.mech_set->elements[i], mech_type)) { *mech_static = &global_mechs.mech_set->elements[i]; return 0; } @@ -383,7 +369,7 @@ OM_uint32 gpm_inquire_names_for_mech(OM_uint32 *minor_status, } for (unsigned i = 0; i < global_mechs.info_len; i++) { - if (!gpm_equal_oids(global_mechs.info[i].mech, mech_type)) { + if (!gss_oid_equal(global_mechs.info[i].mech, mech_type)) { continue; } ret_maj = gpm_copy_gss_OID_set(&ret_min, @@ -481,7 +467,7 @@ OM_uint32 gpm_inquire_attrs_for_mech(OM_uint32 *minor_status, } for (unsigned i = 0; i < global_mechs.info_len; i++) { - if (!gpm_equal_oids(global_mechs.info[i].mech, mech)) { + if (!gss_oid_equal(global_mechs.info[i].mech, mech)) { continue; } @@ -540,7 +526,7 @@ OM_uint32 gpm_inquire_saslname_for_mech(OM_uint32 *minor_status, } for (unsigned i = 0; i < global_mechs.info_len; i++) { - if (!gpm_equal_oids(global_mechs.info[i].mech, desired_mech)) { + if (!gss_oid_equal(global_mechs.info[i].mech, desired_mech)) { continue; } ret_maj = gpm_copy_gss_buffer(&ret_min, @@ -598,7 +584,7 @@ OM_uint32 gpm_display_mech_attr(OM_uint32 *minor_status, } for (unsigned i = 0; i < global_mechs.desc_len; i++) { - if (!gpm_equal_oids(global_mechs.desc[i].attr, mech_attr)) { + if (!gss_oid_equal(global_mechs.desc[i].attr, mech_attr)) { continue; } ret_maj = gpm_copy_gss_buffer(&ret_min, diff --git a/src/client/gssapi_gpm.h b/src/client/gssapi_gpm.h index b7ba04b1f..bdf12e1e2 100644 --- a/src/client/gssapi_gpm.h +++ b/src/client/gssapi_gpm.h @@ -10,6 +10,7 @@ #include #include #include +#include #include "rpcgen/gp_rpc.h" #include "rpcgen/gss_proxy.h" #include "src/gp_common.h" From c0561c078bc22b9523ac25f515ad85b735c26a92 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 17:20:44 -0400 Subject: [PATCH 11/12] Avoid unnecessary allocation in gpm_inquire_mechs_for_name() Signed-off-by: Simo Sorce [rharwood@redhat.com: clarified commit message] Reviewed-by: Robbie Harwood --- src/client/gpm_indicate_mechs.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/client/gpm_indicate_mechs.c b/src/client/gpm_indicate_mechs.c index 4041dcd65..73fadf0af 100644 --- a/src/client/gpm_indicate_mechs.c +++ b/src/client/gpm_indicate_mechs.c @@ -390,7 +390,7 @@ OM_uint32 gpm_inquire_mechs_for_name(OM_uint32 *minor_status, uint32_t ret_min; uint32_t ret_maj; uint32_t discard; - gss_OID name_type = GSS_C_NO_OID; + gss_OID_desc name_type; int present; if (!minor_status) { @@ -407,19 +407,14 @@ OM_uint32 gpm_inquire_mechs_for_name(OM_uint32 *minor_status, return GSS_S_FAILURE; } - ret_min = gp_conv_gssx_to_oid_alloc(&input_name->name_type, &name_type); - if (ret_min) { - ret_maj = GSS_S_FAILURE; - goto done; - } - ret_maj = gss_create_empty_oid_set(&ret_min, mech_types); if (ret_maj) { goto done; } + gp_conv_gssx_to_oid(&input_name->name_type, &name_type); for (unsigned i = 0; i < global_mechs.info_len; i++) { - ret_maj = gss_test_oid_set_member(&ret_min, name_type, + ret_maj = gss_test_oid_set_member(&ret_min, &name_type, global_mechs.info[i].name_types, &present); if (ret_maj) { @@ -437,7 +432,6 @@ OM_uint32 gpm_inquire_mechs_for_name(OM_uint32 *minor_status, } done: - gss_release_oid(&discard, &name_type); if (ret_maj) { gss_release_oid_set(&discard, mech_types); *minor_status = ret_min; From 502e448b3b126bf828ed871496dd7520d5075564 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 27 Aug 2020 17:21:03 -0400 Subject: [PATCH 12/12] Use static OIDs in gss_inquire_context() As per other functions gssapi expect a static OID here. Signed-off-by: Simo Sorce [rharwood@redhat.com: commit message fixup] Reviewed-by: Robbie Harwood --- src/client/gpm_inquire_context.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client/gpm_inquire_context.c b/src/client/gpm_inquire_context.c index 8c683fe1e..5800a8d21 100644 --- a/src/client/gpm_inquire_context.c +++ b/src/client/gpm_inquire_context.c @@ -51,7 +51,9 @@ OM_uint32 gpm_inquire_context(OM_uint32 *minor_status, } if (mech_type) { - ret = gp_conv_gssx_to_oid_alloc(&context_handle->mech, mech_type); + gss_OID_desc mech; + gp_conv_gssx_to_oid(&context_handle->mech, &mech); + ret = gpm_mech_to_static(&mech, mech_type); if (ret) { if (src_name) { (void)gpm_release_name(&tmp_min, src_name);