dsdb: Allow special chars like "@" in samAccountName when generating the salt

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14874
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14881

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>

Autobuild-User(master): Stefan Metzmacher <metze@samba.org>
Autobuild-Date(master): Wed Oct 20 12:54:54 UTC 2021 on sn-devel-184

(cherry picked from commit 5eeb441b771a1ffe1ba1c69b72e8795f525a58ed)

6 files changed, 195 insertions, 66 deletions

diff --git a/auth/credentials/credentials_krb5.c b/auth/credentials/credentials_krb5.c
index d7b1c430841..2338d9f114b 100644
--- a/auth/credentials/credentials_krb5.c
+++ b/auth/credentials/credentials_krb5.c
@@ -1200,12 +1200,12 @@ _PUBLIC_ int cli_credentials_get_keytab(struct cli_credentials *cred,
 		break;
 	}
 
-	ret = smb_krb5_salt_principal(realm,
-				      username, /* sAMAccountName */
-				      upn, /* userPrincipalName */
-				      uac_flags,
-				      mem_ctx,
-				      &salt_principal);
+	ret = smb_krb5_salt_principal_str(realm,
+					  username, /* sAMAccountName */
+					  upn, /* userPrincipalName */
+					  uac_flags,
+					  mem_ctx,
+					  &salt_principal);
 	if (ret) {
 		talloc_free(mem_ctx);
 		return ret;
diff --git a/lib/krb5_wrap/krb5_samba.c b/lib/krb5_wrap/krb5_samba.c
index 20ce86c708d..63a6e951f80 100644
--- a/lib/krb5_wrap/krb5_samba.c
+++ b/lib/krb5_wrap/krb5_samba.c
@@ -456,19 +456,20 @@ int smb_krb5_get_pw_salt(krb5_context context,
  *
  * @see smb_krb5_salt_principal2data
  */
-int smb_krb5_salt_principal(const char *realm,
+int smb_krb5_salt_principal(krb5_context krb5_ctx,
+			    const char *realm,
 			    const char *sAMAccountName,
 			    const char *userPrincipalName,
 			    uint32_t uac_flags,
-			    TALLOC_CTX *mem_ctx,
-			    char **_salt_principal)
+			    krb5_principal *salt_princ)
 {
 	TALLOC_CTX *frame = talloc_stackframe();
 	char *upper_realm = NULL;
 	const char *principal = NULL;
 	int principal_len = 0;
+	krb5_error_code krb5_ret;
 
-	*_salt_principal = NULL;
+	*salt_princ = NULL;
 
 	if (sAMAccountName == NULL) {
 		TALLOC_FREE(frame);
@@ -512,7 +513,6 @@ int smb_krb5_salt_principal(const char *realm,
 	 */
 	if (uac_flags & UF_TRUST_ACCOUNT_MASK) {
 		int computer_len = 0;
-		char *tmp = NULL;
 
 		computer_len = strlen(sAMAccountName);
 		if (sAMAccountName[computer_len-1] == '$') {
@@ -520,54 +520,103 @@ int smb_krb5_salt_principal(const char *realm,
 		}
 
 		if (uac_flags & UF_INTERDOMAIN_TRUST_ACCOUNT) {
-			principal = talloc_asprintf(frame, "krbtgt/%*.*s",
-						    computer_len, computer_len,
-						    sAMAccountName);
-			if (principal == NULL) {
+			const char *krbtgt = "krbtgt";
+			krb5_ret = krb5_build_principal_ext(krb5_ctx,
+							    salt_princ,
+							    strlen(upper_realm),
+							    upper_realm,
+							    strlen(krbtgt),
+							    krbtgt,
+							    computer_len,
+							    sAMAccountName,
+							    0);
+			if (krb5_ret != 0) {
 				TALLOC_FREE(frame);
-				return ENOMEM;
+				return krb5_ret;
 			}
 		} else {
-
-			tmp = talloc_asprintf(frame, "host/%*.*s.%s",
-					      computer_len, computer_len,
-					      sAMAccountName, realm);
+			const char *host = "host";
+			char *tmp = NULL;
+			char *tmp_lower = NULL;
+
+			tmp = talloc_asprintf(frame, "%*.*s.%s",
+					      computer_len,
+					      computer_len,
+					      sAMAccountName,
+					      realm);
 			if (tmp == NULL) {
 				TALLOC_FREE(frame);
 				return ENOMEM;
 			}
 
-			principal = strlower_talloc(frame, tmp);
-			TALLOC_FREE(tmp);
-			if (principal == NULL) {
+			tmp_lower = strlower_talloc(frame, tmp);
+			if (tmp_lower == NULL) {
 				TALLOC_FREE(frame);
 				return ENOMEM;
 			}
-		}
 
-		principal_len = strlen(principal);
+			krb5_ret = krb5_build_principal_ext(krb5_ctx,
+							    salt_princ,
+							    strlen(upper_realm),
+							    upper_realm,
+							    strlen(host),
+							    host,
+							    strlen(tmp_lower),
+							    tmp_lower,
+							    0);
+			if (krb5_ret != 0) {
+				TALLOC_FREE(frame);
+				return krb5_ret;
+			}
+		}
 
 	} else if (userPrincipalName != NULL) {
-		char *p;
+		/*
+		 * We parse the name not only to allow an easy
+		 * replacement of the realm (no matter the realm in
+		 * the UPN, the salt comes from the upper-case real
+		 * realm, but also to correctly provide a salt when
+		 * the UPN is host/foo.bar
+		 *
+		 * This can fail for a UPN of the form foo@bar@REALM
+		 * (which is accepted by windows) however.
+		 */
+		krb5_ret = krb5_parse_name(krb5_ctx,
+					   userPrincipalName,
+					   salt_princ);
 
-		principal = userPrincipalName;
-		p = strchr(principal, '@');
-		if (p != NULL) {
-			principal_len = PTR_DIFF(p, principal);
-		} else {
-			principal_len = strlen(principal);
+		if (krb5_ret != 0) {
+			TALLOC_FREE(frame);
+			return krb5_ret;
+		}
+
+		/*
+		 * No matter what realm (including none) in the UPN,
+		 * the realm is replaced with our upper-case realm
+		 */
+		smb_krb5_principal_set_realm(krb5_ctx,
+					     *salt_princ,
+					     upper_realm);
+		if (krb5_ret != 0) {
+			krb5_free_principal(krb5_ctx, *salt_princ);
+			TALLOC_FREE(frame);
+			return krb5_ret;
 		}
 	} else {
 		principal = sAMAccountName;
 		principal_len = strlen(principal);
-	}
 
-	*_salt_principal = talloc_asprintf(mem_ctx, "%*.*s@%s",
-					   principal_len, principal_len,
-					   principal, upper_realm);
-	if (*_salt_principal == NULL) {
-		TALLOC_FREE(frame);
-		return ENOMEM;
+		krb5_ret = krb5_build_principal_ext(krb5_ctx,
+						    salt_princ,
+						    strlen(upper_realm),
+						    upper_realm,
+						    principal_len,
+						    principal,
+						    0);
+		if (krb5_ret != 0) {
+			TALLOC_FREE(frame);
+			return krb5_ret;
+		}
 	}
 
 	TALLOC_FREE(frame);
@@ -575,6 +624,83 @@ int smb_krb5_salt_principal(const char *realm,
 }
 
 /**
+ * @brief This constructs the salt principal used by active directory
+ *
+ * Most Kerberos encryption types require a salt in order to
+ * calculate the long term private key for user/computer object
+ * based on a password.
+ *
+ * The returned _salt_principal is a string in forms like this:
+ * - host/somehost.example.com@EXAMPLE.COM
+ * - SomeAccount@EXAMPLE.COM
+ * - SomePrincipal@EXAMPLE.COM
+ *
+ * This is not the form that's used as salt, it's just
+ * the human readable form. It needs to be converted by
+ * smb_krb5_salt_principal2data().
+ *
+ * @param[in]  realm              The realm the user/computer is added too.
+ *
+ * @param[in]  sAMAccountName     The sAMAccountName attribute of the object.
+ *
+ * @param[in]  userPrincipalName  The userPrincipalName attribute of the object
+ *                                or NULL is not available.
+ *
+ * @param[in]  uac_flags          UF_ACCOUNT_TYPE_MASKed userAccountControl field
+ *
+ * @param[in]  mem_ctx            The TALLOC_CTX to allocate _salt_principal.
+ *
+ * @param[out]  _salt_principal   The resulting principal as string.
+ *
+ * @retval 0 Success; otherwise - Kerberos error codes
+ *
+ * @see smb_krb5_salt_principal2data
+ */
+int smb_krb5_salt_principal_str(const char *realm,
+				const char *sAMAccountName,
+				const char *userPrincipalName,
+				uint32_t uac_flags,
+				TALLOC_CTX *mem_ctx,
+				char **_salt_principal_str)
+{
+	krb5_principal salt_principal = NULL;
+	char *salt_principal_malloc;
+	krb5_context krb5_ctx;
+	krb5_error_code krb5_ret
+		= smb_krb5_init_context_common(&krb5_ctx);
+	if (krb5_ret != 0) {
+		DBG_ERR("kerberos init context failed (%s)\n",
+			error_message(krb5_ret));
+		return krb5_ret;
+	}
+
+	krb5_ret = smb_krb5_salt_principal(krb5_ctx,
+					   realm,
+					   sAMAccountName,
+					   userPrincipalName,
+					   uac_flags,
+					   &salt_principal);
+
+	krb5_ret = krb5_unparse_name(krb5_ctx, salt_principal,
+				     &salt_principal_malloc);
+	if (krb5_ret != 0) {
+		krb5_free_principal(krb5_ctx, salt_principal);
+		DBG_ERR("kerberos unparse of salt principal failed (%s)\n",
+			error_message(krb5_ret));
+		return krb5_ret;
+	}
+	krb5_free_principal(krb5_ctx, salt_principal);
+	*_salt_principal_str
+		= talloc_strdup(mem_ctx, salt_principal_malloc);
+	krb5_free_unparsed_name(krb5_ctx, salt_principal_malloc);
+
+	if (*_salt_principal_str == NULL) {
+		return ENOMEM;
+	}
+	return 0;
+}
+
+/**
  * @brief Converts the salt principal string into the salt data blob
  *
  * This function takes a salt_principal as string in forms like this:
diff --git a/lib/krb5_wrap/krb5_samba.h b/lib/krb5_wrap/krb5_samba.h
index ca9a893e4f7..56a2a975278 100644
--- a/lib/krb5_wrap/krb5_samba.h
+++ b/lib/krb5_wrap/krb5_samba.h
@@ -350,12 +350,19 @@ krb5_error_code ms_suptypes_to_ietf_enctypes(TALLOC_CTX *mem_ctx,
 int smb_krb5_get_pw_salt(krb5_context context,
 			 krb5_const_principal host_princ,
 			 krb5_data *psalt);
-int smb_krb5_salt_principal(const char *realm,
+int smb_krb5_salt_principal(krb5_context krb5_ctx,
+			    const char *realm,
 			    const char *sAMAccountName,
 			    const char *userPrincipalName,
 			    uint32_t uac_flags,
-			    TALLOC_CTX *mem_ctx,
-			    char **_salt_principal);
+			    krb5_principal *salt_princ);
+
+int smb_krb5_salt_principal_str(const char *realm,
+				const char *sAMAccountName,
+				const char *userPrincipalName,
+				uint32_t uac_flags,
+				TALLOC_CTX *mem_ctx,
+				char **_salt_principal);
 int smb_krb5_salt_principal2data(krb5_context context,
 				 const char *salt_principal,
 				 TALLOC_CTX *mem_ctx,
diff --git a/selftest/knownfail.d/kdc-salt b/selftest/knownfail.d/kdc-salt
index 1a4ecd44624..a671e4d93eb 100644
--- a/selftest/knownfail.d/kdc-salt
+++ b/selftest/knownfail.d/kdc-salt
@@ -1,12 +1 @@
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_case_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_case_user
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_end_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_end_no_dollar_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_end_user
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_start_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_start_user
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_at_user
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_double_at_mac
-^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_double_at_user
 ^samba.tests.krb5.salt_tests.samba.tests.krb5.salt_tests.SaltTests.test_salt_upn_at_realm_user
diff --git a/source3/passdb/machine_account_secrets.c b/source3/passdb/machine_account_secrets.c
index d81f79c705b..1964eb5a448 100644
--- a/source3/passdb/machine_account_secrets.c
+++ b/source3/passdb/machine_account_secrets.c
@@ -1574,11 +1574,11 @@ NTSTATUS secrets_store_JoinCtx(const struct libnet_JoinCtx *r)
 	if (info->salt_principal == NULL && r->out.domain_is_ad) {
 		char *p = NULL;
 
-		ret = smb_krb5_salt_principal(info->domain_info.dns_domain.string,
-					      info->account_name,
-					      NULL /* userPrincipalName */,
-					      UF_WORKSTATION_TRUST_ACCOUNT,
-					      info, &p);
+		ret = smb_krb5_salt_principal_str(info->domain_info.dns_domain.string,
+						  info->account_name,
+						  NULL /* userPrincipalName */,
+						  UF_WORKSTATION_TRUST_ACCOUNT,
+						  info, &p);
 		if (ret != 0) {
 			status = krb5_to_nt_status(ret);
 			DBG_ERR("smb_krb5_salt_principal() failed "
diff --git a/source4/dsdb/samdb/ldb_modules/password_hash.c b/source4/dsdb/samdb/ldb_modules/password_hash.c
index 5bdd23c13e9..9f38a31c8dd 100644
--- a/source4/dsdb/samdb/ldb_modules/password_hash.c
+++ b/source4/dsdb/samdb/ldb_modules/password_hash.c
@@ -688,8 +688,8 @@ static int setup_kerberos_keys(struct setup_password_fields_io *io)
 {
 	struct ldb_context *ldb;
 	krb5_error_code krb5_ret;
-	char *salt_principal = NULL;
-	char *salt_data = NULL;
+	krb5_principal salt_principal = NULL;
+	krb5_data salt_data;
 	krb5_data salt;
 	krb5_keyblock key;
 	krb5_data cleartext_data;
@@ -700,11 +700,11 @@ static int setup_kerberos_keys(struct setup_password_fields_io *io)
 	cleartext_data.length = io->n.cleartext_utf8->length;
 
 	uac_flags = io->u.userAccountControl & UF_ACCOUNT_TYPE_MASK;
-	krb5_ret = smb_krb5_salt_principal(io->ac->status->domain_data.realm,
+	krb5_ret = smb_krb5_salt_principal(io->smb_krb5_context->krb5_context,
+					   io->ac->status->domain_data.realm,
 					   io->u.sAMAccountName,
 					   io->u.user_principal_name,
 					   uac_flags,
-					   io->ac,
 					   &salt_principal);
 	if (krb5_ret) {
 		ldb_asprintf_errstring(ldb,
@@ -718,8 +718,10 @@ static int setup_kerberos_keys(struct setup_password_fields_io *io)
 	/*
 	 * create salt from salt_principal
 	 */
-	krb5_ret = smb_krb5_salt_principal2data(io->smb_krb5_context->krb5_context,
-						salt_principal, io->ac, &salt_data);
+	krb5_ret = smb_krb5_get_pw_salt(io->smb_krb5_context->krb5_context,
+					salt_principal, &salt_data);
+
+	krb5_free_principal(io->smb_krb5_context->krb5_context, salt_principal);
 	if (krb5_ret) {
 		ldb_asprintf_errstring(ldb,
 				       "setup_kerberos_keys: "
@@ -728,12 +730,17 @@ static int setup_kerberos_keys(struct setup_password_fields_io *io)
 								  krb5_ret, io->ac));
 		return LDB_ERR_OPERATIONS_ERROR;
 	}
-	io->g.salt = salt_data;
 
 	/* now use the talloced copy of the salt */
-	salt.data	= discard_const(io->g.salt);
+	salt.data	= talloc_strndup(io->ac,
+					 (char *)salt_data.data,
+					 salt_data.length);
+	io->g.salt      = salt.data;
 	salt.length	= strlen(io->g.salt);
 
+	smb_krb5_free_data_contents(io->smb_krb5_context->krb5_context,
+				    &salt_data);
+
 	/*
 	 * create ENCTYPE_AES256_CTS_HMAC_SHA1_96 key out of
 	 * the salt and the cleartext password