From 6613538b475e7ce4dfc0c3c749ff21a1134744d6 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Wed, 2 Oct 2013 21:05:56 -0700 Subject: [PATCH] Add additional checks for passwords based on principals The check for passwords based on the principal now check for passwords formed by reversing or adding numbers before and after each separate component of the principal. This will catch passwords based on the realm or components of the realm, which will often catch passwords based on the name of the local institution. --- NEWS | 6 + plugin/principal.c | 194 ++++++++++++++++++++-------- tests/data/passwords/principal.json | 56 ++++++++ 3 files changed, 202 insertions(+), 54 deletions(-) diff --git a/NEWS b/NEWS index b171725..35c70d0 100644 --- a/NEWS +++ b/NEWS @@ -46,6 +46,12 @@ krb5-strength 2.0 (unreleased) This mode is mostly useful for testing, since such simple checking can more easily be done via less complex password strength configurations. + The check for passwords based on the principal now check for passwords + formed by reversing or adding numbers before and after each separate + component of the principal. This will catch passwords based on the + realm or components of the realm, which will often catch passwords + based on the name of the local institution. + The plugin now sets the Kerberos error message in the context to pass error information, resulting in higher-quality error reporting in the MIT Kerberos plugin. diff --git a/plugin/principal.c b/plugin/principal.c index 5d396a4..55b6d13 100644 --- a/plugin/principal.c +++ b/plugin/principal.c @@ -24,80 +24,166 @@ /* - * Check whether the password is based in some way on the principal. Returns - * 0 if it is not and some non-zero error code if it appears to be. + * Given a string taken from the principal, check if the password matches that + * string or is that string with leading or trailing digits added. If so, + * sets the Kerberos error and returns a non-zero error code. Otherwise, + * returns 0. */ -krb5_error_code -strength_check_principal(krb5_context ctx, krb5_pwqual_moddata data UNUSED, - const char *principal, const char *password) +static krb5_error_code +check_component(krb5_context ctx, const char *component, const char *password) { - char *user, *p; - const char *q; - size_t i, j; + char *copy; + size_t i, j, complength, passlength; char c; - /* - * We get the principal (in krb5_unparse_name format) and we want to be - * sure that the password doesn't match the username, the username - * reversed, or the username with trailing digits. We therefore have to - * copy the string so that we can manipulate it a bit. - */ - if (strcasecmp(password, principal) == 0) + /* Check if the password is a simple match for the component. */ + if (strcasecmp(component, password) == 0) return strength_error_generic(ctx, ERROR_USERNAME); - user = strdup(principal); - if (user == NULL) - return strength_error_system(ctx, "cannot allocate memory"); - /* Strip the realm off of the principal. */ - for (p = user; p[0] != '\0'; p++) { - if (p[0] == '\\' && p[1] != '\0') { - p++; - continue; + /* + * If the length of the password matches the length of the component, + * check for a reversed match. + */ + complength = strlen(component); + passlength = strlen(password); + if (complength == passlength) { + copy = strdup(component); + if (copy == NULL) + return strength_error_system(ctx, "cannot allocate memory"); + for (i = 0, j = complength - 1; i < j; i++, j--) { + c = copy[i]; + copy[i] = copy[j]; + copy[j] = c; } - if (p[0] == '@') { - p[0] = '\0'; - break; + if (strcasecmp(copy, password) == 0) { + free(copy); + return strength_error_generic(ctx, ERROR_USERNAME); } + free(copy); } /* - * If the length of the password matches the length of the local portion - * of the principal, check for exact matches or reversed matches. + * We've checked everything we care about unless the password is longer + * than the component. */ - if (strlen(password) == strlen(user)) { - if (strcasecmp(password, user) == 0) { - free(user); - return strength_error_generic(ctx, ERROR_USERNAME); - } + if (passlength <= complength) + return 0; - /* Check against the reversed username. */ - for (i = 0, j = strlen(user) - 1; i < j; i++, j--) { - c = user[i]; - user[i] = user[j]; - user[j] = c; - } - if (strcasecmp(password, user) == 0) { - free(user); - return strength_error_generic(ctx, ERROR_USERNAME); - } + /* + * Check whether the user just added leading or trailing digits to the + * component of the principal to form the password. + */ + for (i = 0; i <= passlength - complength; i++) { + if (strncasecmp(password + i, component, complength) != 0) + continue; + + /* + * For this to be a match, all characters from 0 to i - 1 must be + * digits, and all characters from strlen(component) + i to + * strlen(password) - 1 must be digits. + */ + for (j = 0; j < i; j++) + if (!isdigit((unsigned char) password[j])) + return 0; + for (j = complength + i; j < passlength; j++) + if (!isdigit((unsigned char) password[j])) + return 0; + + /* The password was formed by adding digits to this component. */ + return strength_error_generic(ctx, ERROR_USERNAME); } + /* No similarity to component detected. */ + return 0; +} + + +/* + * Returns true if a given character is a separator character for forming + * components, and false otherwise. + */ +static bool +is_separator(unsigned char c) +{ + if (c == '-' || c == '_') + return false; + if (isalnum(c)) + return false; + return true; +} + + +/* + * Check whether the password is based in some way on the principal. We do + * this by scanning the principal (in string form) and checking both each + * component of that password (defined as the alphanumeric, hyphen, and + * underscore bits between other characters) and the remaining principal from + * that point forward (to catch, for example, the entire realm). Returns 0 if + * it is not and some non-zero error code if it appears to be. + */ +krb5_error_code +strength_check_principal(krb5_context ctx, krb5_pwqual_moddata data UNUSED, + const char *principal, const char *password) +{ + krb5_error_code code; + char *copy, *start; + size_t i, length; + + /* Sanity check. */ + if (principal == NULL) + return 0; + + /* Start with checking the entire principal. */ + code = check_component(ctx, principal, password); + if (code != 0) + return code; + + /* + * Make a copy of the principal and scan forward past any leading + * separators. + */ + length = strlen(principal); + copy = strdup(principal); + if (copy == NULL) + return strength_error_system(ctx, "cannot allocate memory"); + i = 0; + while (copy[i] != '\0' && is_separator(copy[i])) + i++; + /* - * If the length is greater, check whether the user just added trailing - * digits to the local portion of the principal to form the password. + * Now loop for each component. At the start of each loop, check against + * the component formed by the rest of the principal string. */ - if (strlen(password) > strlen(user)) - if (strncasecmp(password, user, strlen(user)) == 0) { - q = password + strlen(user); - while (isdigit((unsigned char) *q)) - q++; - if (*q == '\0') { - free(user); - return strength_error_generic(ctx, ERROR_USERNAME); + do { + if (i != 0) { + code = check_component(ctx, copy + i, password); + if (code != 0) { + free(copy); + return code; } } + /* Set the component start and then scan for a separator. */ + start = copy + i; + while (i < length && !is_separator(copy[i])) + i++; + + /* At end of string or a separator. Truncate the component. */ + copy[i] = '\0'; + + /* Check the current component. */ + code = check_component(ctx, start, password); + if (code != 0) { + free(copy); + return code; + } + + /* Scan forward past any more separators. */ + while (i < length && is_separator(copy[i])) + i++; + } while (i < length); + /* Password does not appear to be based on the principal. */ - free(user); + free(copy); return 0; } diff --git a/tests/data/passwords/principal.json b/tests/data/passwords/principal.json index 98dddfa..c82ecf9 100644 --- a/tests/data/passwords/principal.json +++ b/tests/data/passwords/principal.json @@ -26,5 +26,61 @@ "password": "test@EXAMPLE.ORG", "code": "KADM5_PASS_Q_GENERIC", "error": "password based on username" + }, + { + "name": "principal with leading digits", + "principal": "someuser@EXAMPLE.ORG", + "password": "123someuser", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "principal with leading and trailing digits", + "principal": "someuser@EXAMPLE.ORG", + "password": "1someuser2", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is realm (lowercase)", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "newexample", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is realm (lowercase) with digits", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "newexample123", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is realm (lowercase) with leading digits", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "123newexample", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is realm reversed", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "ELPMAXEWEN", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is second realm with digits", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "ORG1791520", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" + }, + { + "name": "is whole realm (mixed case)", + "principal": "someuser@NEWEXAMPLE.ORG", + "password": "NewExample.Org", + "code": "KADM5_PASS_Q_GENERIC", + "error": "password based on username" } ] -- 2.39.2