[PATCH: Kerberos keytab bug test and fix]
Andrew Bartlett
abartlet at samba.org
Tue May 15 03:09:20 UTC 2018
On Tue, 2018-05-15 at 14:17 +1200, Aaron Haslett via samba-technical
wrote:
> Clearing the keytab with chgtdcpass doesn't work. This test and fix
> solves the problem.
>
> CI tests here:
>
> https://gitlab.com/catalyst-samba/samba/pipelines/22019771
I've improved the all-important whitespace, some coding style issues
and commit messages.
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
https://gitlab.com/catalyst-samba/samba/pipelines/22020990
https://gitlab.com/catalyst-samba/samba/commits/aaron-kbtest
Can I get a second team reviewer please?
Thanks,
Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
-------------- next part --------------
From 776c0d185e66782dde63fd484b22bce6428508df Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
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 <aaronhaslett at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
selftest/knownfail.d/keytab | 1 +
selftest/tests.py | 2 +
source4/auth/tests/kerberos.c | 107 ++++++++++++++++++++++++++++++++++++++++++
source4/auth/wscript_build | 6 +++
4 files changed, 116 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 00000000000..6777d98ff28
--- /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 185ad37fd4a..f354bb57ef5 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 00000000000..703c8067908
--- /dev/null
+++ b/source4/auth/tests/kerberos.c
@@ -0,0 +1,107 @@
+#include <time.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdint.h>
+#include <cmocka.h>
+
+#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";
+ char expect_princ_name[23] = "user0 at samba.example.com";
+ 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; i<num_principals; i++) {
+ princ_name[4] = (char)i+48;
+ smb_krb5_make_principal(krb5_ctx, &(principals[i]),
+ "samba.example.com", princ_name, NULL);
+ kt_entry.principal = principals[i];
+ for (j=0; j<num_kvnos; j++) {
+ kt_entry.vno = j+1;
+ krb5_kt_add_entry(krb5_ctx, keytab, &kt_entry);
+ }
+ }
+
+ code = krb5_kt_start_seq_get(krb5_ctx, keytab, &cursor);
+ assert_int_equal(code, 0);
+ for (i=0; i<num_principals; i++) {
+ expect_princ_name[4] = (char)i+48;
+ for (j=0; j<num_kvnos; j++) {
+ char *unparsed_name;
+ code = krb5_kt_next_entry(krb5_ctx, keytab,
+ &kt_entry, &cursor);
+ assert_int_equal(code, 0);
+ assert_int_equal(kt_entry.vno, j+1);
+ krb5_unparse_name(krb5_ctx, kt_entry.principal,
+ &unparsed_name);
+ assert_string_equal(expect_princ_name, unparsed_name);
+ }
+ }
+
+ 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; i<num_principals; i++) {
+ char *unparsed_name;
+ expect_princ_name[4] = (char)i+48;
+ code = krb5_kt_next_entry(krb5_ctx, keytab, &kt_entry, &cursor);
+ assert_int_equal(code, 0);
+ assert_int_equal(kt_entry.vno, kvno-1);
+ krb5_unparse_name(krb5_ctx, kt_entry.principal, &unparsed_name);
+ assert_string_equal(expect_princ_name, unparsed_name);
+ }
+ 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),
+ };
+
+ 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 f7508619cd7..d3452d2ca92 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.11.0
From 00886f6b9347f33c906218a3a7c49965d2261482 Mon Sep 17 00:00:00 2001
From: Aaron Haslett <aaronhaslett at catalyst.net.nz>
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 <aaronhaslett at catalyst.net.nz>
---
selftest/knownfail.d/keytab | 1 -
source4/auth/kerberos/kerberos_util.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/selftest/knownfail.d/keytab b/selftest/knownfail.d/keytab
index 6777d98ff28..e69de29bb2d 100644
--- a/selftest/knownfail.d/keytab
+++ b/selftest/knownfail.d/keytab
@@ -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 618da626652..50bf8feec96 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.11.0
More information about the samba-technical
mailing list