From 79d39291e0d43287f9bfb383aa81f19db7c006b7 Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy Date: Wed, 8 Apr 2020 15:00:38 +0300 Subject: [PATCH] CVE-2020-1722: prevent use of too long passwords NIST SP 800-63-3B sets a recommendation to have password length upper bound limited in A.2: https://pages.nist.gov/800-63-3/sp800-63b.html#appA Users should be encouraged to make their passwords as lengthy as they want, within reason. Since the size of a hashed password is independent of its length, there is no reason not to permit the use of lengthy passwords (or pass phrases) if the user wishes. Extremely long passwords (perhaps megabytes in length) could conceivably require excessive processing time to hash, so it is reasonable to have some limit. FreeIPA already applied 256 characters limit for non-random passwords set through ipa-getkeytab tool. The limit was not, however, enforced in other places. MIT Kerberos limits the length of the password to 1024 characters in its tools. However, these tools (kpasswd and 'cpw' command of kadmin) do not differentiate between a password larger than 1024 and a password of 1024 characters. As a result, longer passwords are silently cut off. To prevent silent cut off for user passwords, use limit of 1000 characters. Thus, this patch enforces common limit of 1000 characters everywhere: - LDAP-based password changes - LDAP password change control - LDAP ADD and MOD operations on clear-text userPassword - Keytab setting with ipa-getkeytab - Kerberos password setting and changing Fixes: https://pagure.io/freeipa/issue/8268 Signed-off-by: Alexander Bokovoy Signed-off-by: Rob Crittenden Reviewed-by: Simo Sorce --- client/ipa-getkeytab.c | 19 ++++- client/man/ipa-getkeytab.1 | 2 +- daemons/ipa-kdb/ipa_kdb_passwords.c | 6 ++ .../ipa-slapi-plugins/ipa-pwd-extop/common.c | 9 +++ .../ipa-pwd-extop/ipa_pwd_extop.c | 13 +++ .../ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h | 1 + .../ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 29 ++++++- ipatests/test_integration/test_commands.py | 80 ++++++++++++++++++- util/ipa_krb5.c | 18 +++++ util/ipa_krb5.h | 3 + 10 files changed, 171 insertions(+), 9 deletions(-) diff --git a/client/ipa-getkeytab.c b/client/ipa-getkeytab.c index 612405419e6..04786be9ee1 100644 --- a/client/ipa-getkeytab.c +++ b/client/ipa-getkeytab.c @@ -562,6 +562,11 @@ static int ldap_get_keytab(krb5_context krbctx, bool generate, char *password, * set match=true to enforce that the two entered passwords match. * * To prompt for an existing password provide prompt1 and set match=false. + * + * Implementation details: + * krb5_prompter_posix() does not differentiate between too long entry or + * an entry exactly the size of a buffer. Thus, allocate a bigger buffer + * and do the check for a too long password afterwards. */ static char *ask_password(krb5_context krbctx, char *prompt1, char *prompt2, bool match) @@ -569,8 +574,10 @@ static char *ask_password(krb5_context krbctx, char *prompt1, char *prompt2, krb5_prompt ap_prompts[2]; krb5_data k5d_pw0; krb5_data k5d_pw1; - char pw0[256]; - char pw1[256]; +#define MAX(a,b) (((a)>(b))?(a):(b)) +#define PWD_BUFFER_SIZE MAX((IPAPWD_PASSWORD_MAX_LEN + 2), 1024) + char pw0[PWD_BUFFER_SIZE]; + char pw1[PWD_BUFFER_SIZE]; char *password; int num_prompts = match ? 2:1; @@ -593,7 +600,12 @@ static char *ask_password(krb5_context krbctx, char *prompt1, char *prompt2, num_prompts, ap_prompts); if (match && (strcmp(pw0, pw1))) { - fprintf(stderr, _("Passwords do not match!")); + fprintf(stderr, _("Passwords do not match!\n")); + return NULL; + } + + if (k5d_pw0.length > IPAPWD_PASSWORD_MAX_LEN) { + fprintf(stderr, "%s\n", ipapwd_password_max_len_errmsg); return NULL; } @@ -998,6 +1010,7 @@ int main(int argc, const char *argv[]) } fprintf(stderr, _("Failed to create key material\n")); + free_keys_contents(krbctx, &keys); exit(8); } diff --git a/client/man/ipa-getkeytab.1 b/client/man/ipa-getkeytab.1 index f06fcd91032..a4bcd88714b 100644 --- a/client/man/ipa-getkeytab.1 +++ b/client/man/ipa-getkeytab.1 @@ -93,7 +93,7 @@ AES\-256 CTS mode with 192\-bit SHA\-384 HMAC ArcFour with HMAC/md5 .TP \fB\-P, \-\-password\fR -Use this password for the key instead of one randomly generated. +Use this password for the key instead of one randomly generated. The length of the password is limited by 1024 characters. Note that MIT Kerberos also limits passwords entered through kpasswd and kadmin commands to the same length. .TP \fB\-D, \-\-binddn\fR The LDAP DN to bind as when retrieving a keytab without Kerberos credentials. Generally used with the \fB\-w\fR or \fB\-W\fR options. diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c index a3d4fe2436d..9362f4305d9 100644 --- a/daemons/ipa-kdb/ipa_kdb_passwords.c +++ b/daemons/ipa-kdb/ipa_kdb_passwords.c @@ -80,6 +80,12 @@ static krb5_error_code ipadb_check_pw_policy(krb5_context context, return EINVAL; } + if (strlen(passwd) > IPAPWD_PASSWORD_MAX_LEN) { + krb5_set_error_message(context, E2BIG, "%s", + ipapwd_password_max_len_errmsg); + return E2BIG; + } + ied->passwd = strdup(passwd); if (!ied->passwd) { return ENOMEM; diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c index ba5c54e58e9..716b7133305 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c @@ -1087,3 +1087,12 @@ void free_ipapwd_krbcfg(struct ipapwd_krbcfg **cfg) *cfg = NULL; }; +int ipapwd_check_max_pwd_len(size_t len, char **errMesg) { + if (len > IPAPWD_PASSWORD_MAX_LEN) { + LOG("%s\n", ipapwd_password_max_len_errmsg); + *errMesg = ipapwd_password_max_len_errmsg; + return LDAP_CONSTRAINT_VIOLATION; + } + return 0; +} + diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c index bae8e74000c..bb87527c768 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c @@ -334,6 +334,11 @@ static int ipapwd_chpwop(Slapi_PBlock *pb, struct ipapwd_krbcfg *krbcfg) goto free_and_return; } + rc = ipapwd_check_max_pwd_len(strlen(newPasswd), &errMesg); + if (rc) { + goto free_and_return; + } + if (oldPasswd == NULL || *oldPasswd == '\0') { /* If user is authenticated, they already gave their password during the bind operation (or used sasl or client cert auth or OS creds) */ @@ -1684,6 +1689,14 @@ static int ipapwd_getkeytab(Slapi_PBlock *pb, struct ipapwd_krbcfg *krbcfg) } else { + if (password != NULL) { + /* if password was passed-in, check its length */ + rc = ipapwd_check_max_pwd_len(strlen(password), &err_msg); + if (rc) { + goto free_and_return; + } + } + /* check if we are allowed to *write* keys */ acl_ok = is_allowed_to_access_attr(pb, bind_dn, target_entry, WRITEKEYS_OP_CHECK, NULL, diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h index 31c76b3f1a3..5a49fa7e6c7 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h @@ -133,6 +133,7 @@ int ipapwd_set_extradata(const char *dn, time_t unixtime); void ipapwd_free_slapi_value_array(Slapi_Value ***svals); void free_ipapwd_krbcfg(struct ipapwd_krbcfg **cfg); +int ipapwd_check_max_pwd_len(size_t len, char **errMesg); /* from encoding.c */ struct ipapwd_keyset { diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c index 8590ac98ea6..f4a90569363 100644 --- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c +++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c @@ -278,6 +278,10 @@ static int ipapwd_pre_add(Slapi_PBlock *pb) rc = LDAP_CONSTRAINT_VIOLATION; slapi_ch_free_string(&userpw); } else { + rc = ipapwd_check_max_pwd_len(strlen(userpw_clear), &errMesg); + if (rc) { + goto done; + } userpw = slapi_ch_strdup(userpw_clear); } @@ -561,6 +565,11 @@ static int ipapwd_pre_mod(Slapi_PBlock *pb) goto done; } bv = lmod->mod_bvalues[0]; + + rc = ipapwd_check_max_pwd_len(bv->bv_len, &errMesg); + if (rc) { + goto done; + } slapi_ch_free_string(&unhashedpw); unhashedpw = slapi_ch_malloc(bv->bv_len+1); if (!unhashedpw) { @@ -783,7 +792,12 @@ static int ipapwd_pre_mod(Slapi_PBlock *pb) if (! unhashedpw && (gen_krb_keys || is_smb || is_ipant)) { if ((userpw != NULL) && ('{' == userpw[0])) { if (0 == strncasecmp(userpw, "{CLEAR}", strlen("{CLEAR}"))) { - unhashedpw = slapi_ch_strdup(&userpw[strlen("{CLEAR}")]); + const char *userpw_clear = &userpw[strlen("{CLEAR}")]; + rc = ipapwd_check_max_pwd_len(strlen(userpw_clear), &errMesg); + if (rc) { + goto done; + } + unhashedpw = slapi_ch_strdup(userpw_clear); if (NULL == unhashedpw) { LOG_OOM(); rc = LDAP_OPERATIONS_ERROR; @@ -1419,6 +1433,8 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb) time_t expire_time; char *principal_expire = NULL; struct tm expire_tm; + int rc = LDAP_INVALID_CREDENTIALS; + char *errMesg = NULL; /* get BIND parameters */ ret |= slapi_pblock_get(pb, SLAPI_BIND_TARGET_SDN, &target_sdn); @@ -1480,8 +1496,14 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb) goto invalid_creds; /* Ensure that there is a password. */ - if (credentials->bv_len == 0) + if (credentials->bv_len == 0) { goto invalid_creds; + } else { + rc = ipapwd_check_max_pwd_len(credentials->bv_len, &errMesg); + if (rc) { + goto invalid_creds; + } + } /* Authenticate the user. */ ret = ipapwd_authenticate(dn, entry, credentials); @@ -1505,8 +1527,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb) invalid_creds: slapi_entry_free(entry); slapi_sdn_free(&sdn); - slapi_send_ldap_result(pb, LDAP_INVALID_CREDENTIALS, - NULL, NULL, 0, NULL); + slapi_send_ldap_result(pb, rc, NULL, errMesg, 0, NULL); return 1; } diff --git a/ipatests/test_integration/test_commands.py b/ipatests/test_integration/test_commands.py index c92cad9471c..cdad544f517 100644 --- a/ipatests/test_integration/test_commands.py +++ b/ipatests/test_integration/test_commands.py @@ -35,7 +35,7 @@ from ipaplatform.tasks import tasks as platform_tasks from ipatests.create_external_ca import ExternalCA from ipatests.test_ipalib.test_x509 import good_pkcs7, badcert -from ipapython.ipautil import realm_to_suffix +from ipapython.ipautil import realm_to_suffix, ipa_generate_password logger = logging.getLogger(__name__) @@ -445,6 +445,84 @@ def test_dm_change_user_pwd_history_issue7181(self, pwpolicy_global): except CalledProcessError: pytest.fail("Password change failed when it should not") + def test_huge_password(self): + user = 'toolonguser' + hostname = 'toolong.{}'.format(self.master.domain.name) + huge_password = ipa_generate_password(min_len=1536) + original_passwd = 'Secret123' + master = self.master + base_dn = str(master.domain.basedn) + + # Create a user with a password that is too long + tasks.kinit_admin(master) + add_password_stdin_text = "{pwd}\n{pwd}".format(pwd=huge_password) + result = master.run_command(['ipa', 'user-add', user, + '--first', user, + '--last', user, + '--password'], + stdin_text=add_password_stdin_text, + raiseonerr=False) + assert result.returncode != 0 + + # Try again with a normal password + add_password_stdin_text = "{pwd}\n{pwd}".format(pwd=original_passwd) + master.run_command(['ipa', 'user-add', user, + '--first', user, + '--last', user, + '--password'], + stdin_text=add_password_stdin_text) + + # kinit as that user in order to modify the pwd + user_kinit_stdin_text = "{old}\n%{new}\n%{new}\n".format( + old=original_passwd, + new=original_passwd) + master.run_command(['kinit', user], stdin_text=user_kinit_stdin_text) + # sleep 1 sec (krblastpwdchange and krbpasswordexpiration have at most + # a 1s precision) + time.sleep(1) + # perform ldapmodify on userpassword as dir mgr + entry_ldif = textwrap.dedent(""" + dn: uid={user},cn=users,cn=accounts,{base_dn} + changetype: modify + replace: userpassword + userpassword: {new_passwd} + """).format( + user=user, + base_dn=base_dn, + new_passwd=huge_password) + + result = tasks.ldapmodify_dm(master, entry_ldif, raiseonerr=False) + assert result.returncode != 0 + + # ask_password in ipa-getkeytab will complain about too long password + keytab_file = os.path.join(self.master.config.test_dir, + 'user.keytab') + password_stdin_text = "{pwd}\n{pwd}".format(pwd=huge_password) + result = self.master.run_command(['ipa-getkeytab', + '-p', user, + '-P', + '-k', keytab_file, + '-s', self.master.hostname], + stdin_text=password_stdin_text, + raiseonerr=False) + assert result.returncode != 0 + assert "clear-text password is too long" in result.stderr_text + + # Create a host with a user-set OTP that is too long + tasks.kinit_admin(master) + result = master.run_command(['ipa', 'host-add', '--force', + hostname, + '--password', huge_password], + raiseonerr=False) + assert result.returncode != 0 + + # Try again with a valid password + result = master.run_command(['ipa', 'host-add', '--force', + hostname, + '--password', original_passwd], + raiseonerr=False) + assert result.returncode == 0 + def test_change_selinuxusermaporder(self): """ An update file meant to ensure a more sane default was diff --git a/util/ipa_krb5.c b/util/ipa_krb5.c index c09c3daa505..1ba6d25eecb 100644 --- a/util/ipa_krb5.c +++ b/util/ipa_krb5.c @@ -31,6 +31,13 @@ #include "ipa_krb5.h" +#define TOSTR(x) STR(x) +#define STR(x) #x +const char *ipapwd_password_max_len_errmsg = \ + "clear-text password is too long (max " \ + TOSTR(IPAPWD_PASSWORD_MAX_LEN) \ + " chars)!"; + /* Salt types */ #define KRB5P_SALT_SIZE 16 @@ -125,6 +132,13 @@ krb5_error_code ipa_krb5_generate_key_data(krb5_context krbctx, int num_keys; int i; + if ((pwd.data != NULL) && (pwd.length > IPAPWD_PASSWORD_MAX_LEN)) { + kerr = E2BIG; + krb5_set_error_message(krbctx, kerr, "%s", + ipapwd_password_max_len_errmsg); + return kerr; + } + num_keys = num_encsalts; keys = calloc(num_keys, sizeof(krb5_key_data)); if (!keys) { @@ -970,6 +984,10 @@ int create_keys(krb5_context krbctx, if (password) { key_password.data = password; key_password.length = strlen(password); + if (key_password.length > IPAPWD_PASSWORD_MAX_LEN) { + *err_msg = _("Password is too long!\n"); + return 0; + } realm = krb5_princ_realm(krbctx, princ); } diff --git a/util/ipa_krb5.h b/util/ipa_krb5.h index c514580d07e..e01f1e8748e 100644 --- a/util/ipa_krb5.h +++ b/util/ipa_krb5.h @@ -31,6 +31,9 @@ struct keys_container { #define KEYTAB_RET_OID "2.16.840.1.113730.3.8.10.2" #define KEYTAB_GET_OID "2.16.840.1.113730.3.8.10.5" +#define IPAPWD_PASSWORD_MAX_LEN 1000 +extern const char *ipapwd_password_max_len_errmsg; + int krb5_klog_syslog(int, const char *, ...); void