debian/changelog

+samba (2:4.9.5+dfsg-4) unstable; urgency=high
+
+  * This is a security release in order to address the following defect:
+    - CVE-2018-16860 Heimdal KDC: Reject PA-S4U2Self with unkeyed checksum
+
+ -- Mathieu Parent <sathieu@debian.org>  Wed, 08 May 2019 21:53:16 +0200
+
 samba (2:4.9.5+dfsg-3) unstable; urgency=high
 
   * This is a security release in order to address the following defects:

debian/patches/series

@@ -10,3 +10,4 @@ nsswitch-Add-try_authtok-option-to-pam_winbind.patch
 smbd.service-Run-update-apparmor-samba-profile-befor.patch
 CVE-2019-3880-v4-9-02.patch
 CVE-2019-3870-v4-9-04.patch
+CVE-2018-16860-v4-9-06.patch

source4/heimdal/kdc/krb5tgs.c

@@ -1925,6 +1925,13 @@ server_lookup:
 		goto out;
 	    }
 
+	    if (!krb5_checksum_is_keyed(context, self.cksum.cksumtype)) {
+		free_PA_S4U2Self(&self);
+		kdc_log(context, config, 0, "Reject PA-S4U2Self with unkeyed checksum");
+		ret = KRB5KRB_AP_ERR_INAPP_CKSUM;
+		goto out;
+	    }
+
 	    ret = _krb5_s4u2self_to_checksumdata(context, &self, &datack);
 	    if (ret)
 		goto out;

source4/torture/krb5/kdc-canon-heimdal.c

@@ -43,7 +43,9 @@
 #define TEST_UPN              0x0000040
 #define TEST_S4U2SELF         0x0000080
 #define TEST_REMOVEDOLLAR     0x0000100
-#define TEST_ALL              0x00001FF
+#define TEST_AS_REQ_SPN       0x0000200 /* not used */
+#define TEST_MITM_S4U2SELF    0x0000400
+#define TEST_ALL              0x00007FF
 
 struct test_data {
 	const char *test_name;
@@ -61,6 +63,7 @@ struct test_data {
 	bool upn;
 	bool other_upn_suffix;
 	bool s4u2self;
+	bool mitm_s4u2self;
 	bool removedollar;
 	const char *krb5_service;
 	const char *krb5_hostname;
@@ -209,6 +212,67 @@ static bool test_accept_ticket(struct torture_context *tctx,
 	return true;
 }
 
+krb5_error_code
+_krb5_s4u2self_to_checksumdata(krb5_context context,
+			       const PA_S4U2Self *self,
+			       krb5_data *data);
+
+/* Helper function to modify the principal in PA_FOR_USER padata */
+static bool change_for_user_principal(struct torture_krb5_context *test_context,
+				      krb5_data *modified_send_buf)
+{
+	PA_DATA *for_user;
+	int i = 0;
+	size_t used;
+	krb5_error_code ret;
+	PA_S4U2Self self, mod_self;
+	krb5_data cksum_data;
+	krb5_principal admin;
+	heim_octet_string orig_padata_value;
+	krb5_context k5_ctx = test_context->smb_krb5_context->krb5_context;
+
+	for_user = krb5_find_padata(test_context->tgs_req.padata->val,
+				    test_context->tgs_req.padata->len, KRB5_PADATA_FOR_USER, &i);
+	torture_assert(test_context->tctx, for_user != NULL, "No PA_FOR_USER in s4u2self request");
+	orig_padata_value = for_user->padata_value;
+
+	torture_assert_int_equal(test_context->tctx,
+				 krb5_make_principal(k5_ctx, &admin, test_context->test_data->realm,
+						     "Administrator", NULL),
+				 0, "krb5_make_principal() failed");
+	torture_assert_int_equal(test_context->tctx,
+				 decode_PA_S4U2Self(for_user->padata_value.data,
+						    for_user->padata_value.length, &self, NULL),
+				 0, "decode_PA_S4U2Self() failed");
+	mod_self = self;
+	mod_self.name = admin->name;
+
+	torture_assert_int_equal(test_context->tctx,
+				 _krb5_s4u2self_to_checksumdata(k5_ctx, &mod_self, &cksum_data),
+				 0, "_krb5_s4u2self_to_checksumdata() failed");
+	torture_assert_int_equal(test_context->tctx,
+				 krb5_create_checksum(k5_ctx, NULL, KRB5_KU_OTHER_CKSUM,
+						      CKSUMTYPE_CRC32, cksum_data.data,
+						      cksum_data.length, &mod_self.cksum),
+				 0, "krb5_create_checksum() failed");
+
+	ASN1_MALLOC_ENCODE(PA_S4U2Self, for_user->padata_value.data, for_user->padata_value.length,
+			   &mod_self, &used, ret);
+	torture_assert(test_context->tctx, ret == 0, "Failed to encode PA_S4U2Self ASN1 struct");
+	ASN1_MALLOC_ENCODE(TGS_REQ, modified_send_buf->data, modified_send_buf->length,
+			   &test_context->tgs_req, &used, ret);
+	torture_assert(test_context->tctx, ret == 0, "Failed to encode TGS_REQ ASN1 struct");
+
+	free(for_user->padata_value.data);
+	for_user->padata_value = orig_padata_value;
+
+	free_PA_S4U2Self(&self);
+	krb5_data_free(&cksum_data);
+	free_Checksum(&mod_self.cksum);
+
+	return true;
+}
+
 /*
  * TEST_AS_REQ and TEST_AS_REQ_SELF - SEND
  *
@@ -598,7 +662,12 @@ static bool torture_krb5_pre_send_tgs_req_canon_test(struct torture_krb5_context
 
 	}
 
+	if (test_context->test_data->mitm_s4u2self) {
+		torture_assert(test_context->tctx, change_for_user_principal(test_context, modified_send_buf),
+			       "Failed to modify PA_FOR_USER principal name");
+	} else {
 		*modified_send_buf = *send_buf;
+	}
 
 	return true;
 }
@@ -617,6 +686,7 @@ static bool torture_krb5_post_recv_tgs_req_canon_test(struct torture_krb5_contex
 {
 	KRB_ERROR error;
 	size_t used;
+	krb5_error_code expected_error;
 
 	/*
 	 * If this account did not have a servicePrincipalName, then
@@ -627,9 +697,13 @@ static bool torture_krb5_post_recv_tgs_req_canon_test(struct torture_krb5_contex
 		torture_assert_int_equal(test_context->tctx,
 					 error.pvno, 5,
 					 "Got wrong error.pvno");
+		expected_error = KRB5KDC_ERR_S_PRINCIPAL_UNKNOWN - KRB5KDC_ERR_NONE;
+		if (error.error_code != expected_error && test_context->test_data->mitm_s4u2self) {
+			expected_error = KRB5KRB_AP_ERR_INAPP_CKSUM - KRB5KDC_ERR_NONE;
+		}
 		torture_assert_int_equal(test_context->tctx,
 					 error.error_code,
-					 KRB5KDC_ERR_S_PRINCIPAL_UNKNOWN - KRB5KDC_ERR_NONE,
+					 expected_error,
 					 "Got wrong error.error_code");
 	} else {
 		torture_assert_int_equal(test_context->tctx,
@@ -672,6 +746,8 @@ static bool torture_krb5_post_recv_tgs_req_canon_test(struct torture_krb5_contex
 		torture_assert_int_equal(test_context->tctx,
 					 *test_context->tgs_rep.ticket.enc_part.kvno & 0xFFFF0000,
 					 0, "Unexpecedly got a RODC number in the KVNO, should just be principal KVNO");
+		torture_assert(test_context->tctx, test_context->test_data->mitm_s4u2self == false,
+			       "KDC accepted PA_S4U2Self with unkeyed checksum!");
 		free_TGS_REP(&test_context->tgs_rep);
 	}
 	torture_assert(test_context->tctx, test_context->packet_count == 0, "too many packets");
@@ -1845,7 +1921,23 @@ static bool torture_krb5_as_req_canon(struct torture_context *tctx, const void *
 		 */
 		if (torture_setting_bool(tctx, "expect_machine_account", false)
 		    && (test_data->enterprise || test_data->upn == false)) {
+
+			if (test_data->mitm_s4u2self) {
+				torture_assert_int_equal(tctx, k5ret, KRB5KRB_AP_ERR_INAPP_CKSUM,
+							 assertion_message);
+				/* Done testing mitm-s4u2self */
+				return true;
+			}
+
 			torture_assert_int_equal(tctx, k5ret, 0, assertion_message);
+
+			/* Check that the impersonate principal is not being canonicalized by the KDC. */
+			if (test_data->s4u2self) {
+				torture_assert(tctx, krb5_principal_compare(k5_context, server_creds->client,
+									    principal),
+					       "TGS-REP cname does not match requested client principal");
+			}
+
 			torture_assert_int_equal(tctx, krb5_cc_store_cred(k5_context,
 									  ccache, server_creds),
 						 0, "krb5_cc_store_cred failed");
@@ -2222,11 +2314,25 @@ struct torture_suite *torture_krb5_canon(TALLOC_CTX *mem_ctx)
 					     (i & TEST_NETBIOS_REALM) ? "netbios-realm" : "krb5-realm",
 					     (i & TEST_WIN2K) ? "win2k" : "no-win2k",
 					     (i & TEST_UPN) ? "upn" : "no-upn",
-					     (i & TEST_S4U2SELF) ? "s4u2self" : "normal",
+					     (i & TEST_S4U2SELF) ? (i & TEST_MITM_S4U2SELF) ? "mitm-s4u2self" : "s4u2self" : "normal",
 					     (i & TEST_REMOVEDOLLAR) ? "removedollar" : "keepdollar");
 
 		struct test_data *test_data = talloc_zero(suite, struct test_data);
 
+		if (i & TEST_MITM_S4U2SELF) {
+			if (!(i & TEST_S4U2SELF)) {
+				continue;
+			}
+		}
+
+		/*
+		 * Due to backport: this flag is not used until Samba
+		 * 4.10
+		 */
+		if (i & TEST_AS_REQ_SPN) {
+			continue;
+		}
+
 		test_data->test_name = name;
 		test_data->real_realm
 			= strupper_talloc(test_data,
@@ -2246,6 +2352,7 @@ struct torture_suite *torture_krb5_canon(TALLOC_CTX *mem_ctx)
 		test_data->win2k = (i & TEST_WIN2K) != 0;
 		test_data->upn = (i & TEST_UPN) != 0;
 		test_data->s4u2self = (i & TEST_S4U2SELF) != 0;
+		test_data->mitm_s4u2self = (i & TEST_MITM_S4U2SELF) != 0;
 		test_data->removedollar = (i & TEST_REMOVEDOLLAR) != 0;
 		torture_suite_add_simple_tcase_const(suite, name, torture_krb5_as_req_canon,
 						     test_data);