From bfc821a147ffef63c893d67d7ff30fc4e42cf985 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 1 May 2018 11:10:24 +1200 Subject: [PATCH 1/2] auth: keytab invalidation test chgtdcpass should add a new DC password and delete the old ones but the bug exposed by this test causes the tool to remove only a single record from the old entries, leaving the old passwords functional. Since the tool is used by administrators who may have disclosed their domain join password and want to invalidate it, this is a security concern. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13415 Signed-off-by: Aaron Haslett --- selftest/knownfail.d/keytab | 1 + selftest/tests.py | 2 + source4/auth/tests/kerberos.c | 104 ++++++++++++++++++++++++++++++++++++++++++ source4/auth/wscript_build | 6 +++ 4 files changed, 113 insertions(+) create mode 100644 selftest/knownfail.d/keytab create mode 100644 source4/auth/tests/kerberos.c diff --git a/selftest/knownfail.d/keytab b/selftest/knownfail.d/keytab new file mode 100644 index 0000000..6777d98 --- /dev/null +++ b/selftest/knownfail.d/keytab @@ -0,0 +1 @@ +^samba.unittests.kerberos.test_krb5_remove_obsolete_keytab_entries_many diff --git a/selftest/tests.py b/selftest/tests.py index 185ad37..f354bb5 100644 --- a/selftest/tests.py +++ b/selftest/tests.py @@ -187,5 +187,7 @@ plantestsuite("samba.unittests.tldap", "none", [os.path.join(bindir(), "default/source3/test_tldap")]) plantestsuite("samba.unittests.rfc1738", "none", [os.path.join(bindir(), "default/lib/util/test_rfc1738")]) +plantestsuite("samba.unittests.kerberos", "none", + [os.path.join(bindir(), "test_kerberos")]) plantestsuite("samba.unittests.ms_fnmatch", "none", [os.path.join(bindir(), "default/lib/util/test_ms_fnmatch")]) diff --git a/source4/auth/tests/kerberos.c b/source4/auth/tests/kerberos.c new file mode 100644 index 0000000..7a4441f --- /dev/null +++ b/source4/auth/tests/kerberos.c @@ -0,0 +1,104 @@ +#include +#include +#include +#include +#include +#include +#include + +#include "includes.h" +#include "system/kerberos.h" +#include "auth/kerberos/kerberos.h" +#include "auth/credentials/credentials.h" +#include "auth/credentials/credentials_proto.h" +#include "auth/credentials/credentials_krb5.h" +#include "auth/kerberos/kerberos_credentials.h" +#include "auth/kerberos/kerberos_util.h" + +static void internal_obsolete_keytab_test(int num_principals, int num_kvnos, + krb5_kvno kvno, const char *kt_name) +{ + krb5_context krb5_ctx; + krb5_keytab keytab; + krb5_keytab_entry kt_entry; + krb5_kt_cursor cursor; + krb5_error_code code; + + int i,j; + char princ_name[6] = "user0"; + bool found_previous; + const char *error_str; + + TALLOC_CTX *tmp_ctx = talloc_new(NULL); + krb5_principal *principals = talloc_zero_array(tmp_ctx, + krb5_principal, + num_principals); + krb5_init_context(&krb5_ctx); + krb5_kt_resolve(krb5_ctx, kt_name, &keytab); + ZERO_STRUCT(kt_entry); + + for(i=0; iname.name_string.val)); + assert_int_equal(kt_entry.vno, j+1); + } + } + + smb_krb5_remove_obsolete_keytab_entries(tmp_ctx, krb5_ctx, keytab, + num_principals, principals, + kvno, &found_previous, + &error_str); + + code = krb5_kt_start_seq_get(krb5_ctx, keytab, &cursor); + assert_int_equal(code, 0); + for(i=0; iname.name_string.val)); + assert_int_equal(kt_entry.vno, kvno-1); + } + code = krb5_kt_next_entry(krb5_ctx, keytab, &kt_entry, &cursor); + assert_int_not_equal(code, 0); +} + +static void test_krb5_remove_obsolete_keytab_entries_many(void **state) +{ + internal_obsolete_keytab_test(5, 4, (krb5_kvno)5, "MEMORY:LOL2"); +} + +static void test_krb5_remove_obsolete_keytab_entries_one(void **state) +{ + internal_obsolete_keytab_test(1, 2, (krb5_kvno)3, "MEMORY:LOL"); +} + +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_krb5_remove_obsolete_keytab_entries_one), + cmocka_unit_test(test_krb5_remove_obsolete_keytab_entries_many), + }; + + samba_start_debugger(); + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/source4/auth/wscript_build b/source4/auth/wscript_build index f750861..d3452d2 100644 --- a/source4/auth/wscript_build +++ b/source4/auth/wscript_build @@ -42,6 +42,12 @@ bld.SAMBA_SUBSYSTEM('auth4_sam', deps='' ) +bld.SAMBA_BINARY('test_kerberos', + source='tests/kerberos.c', + deps='cmocka authkrb5 krb5samba com_err CREDENTIALS_KRB5', + local_include=False, + install=False + ) for env in bld.gen_python_environments(): pytalloc_util = bld.pyembed_libname('pytalloc-util') -- 2.7.4 From 4f8e4fae87aa2b2586cf12a97287799f83a04842 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 1 May 2018 11:10:50 +1200 Subject: [PATCH 2/2] auth: keytab invalidation fix chgtdcpass should add a new DC password and delete the old ones but the bug exposed by this test causes the tool to remove only a single record from the old entries, leaving the old passwords functional. Since the tool is used by administrators who may have disclosed their domain join password and want to invalidate it, this is a security concern. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13415 Signed-off-by: Aaron Haslett --- selftest/knownfail.d/keytab | 1 - source4/auth/kerberos/kerberos_util.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/keytab diff --git a/selftest/knownfail.d/keytab b/selftest/knownfail.d/keytab deleted file mode 100644 index 6777d98..0000000 --- a/selftest/knownfail.d/keytab +++ /dev/null @@ -1 +0,0 @@ -^samba.unittests.kerberos.test_krb5_remove_obsolete_keytab_entries_many diff --git a/source4/auth/kerberos/kerberos_util.c b/source4/auth/kerberos/kerberos_util.c index 618da62..50bf8fe 100644 --- a/source4/auth/kerberos/kerberos_util.c +++ b/source4/auth/kerberos/kerberos_util.c @@ -633,7 +633,7 @@ krb5_error_code smb_krb5_remove_obsolete_keytab_entries(TALLOC_CTX *mem_ctx, krb5_kt_free_entry(context, &entry); /* Make sure we do not double free */ ZERO_STRUCT(entry); - } while (code != 0); + } while (code == 0); krb5_kt_end_seq_get(context, keytab, &cursor); -- 2.7.4