From 260ca0d7f0083ba59bab54595318e50fffa669d8 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Thu, 26 Sep 2013 22:21:35 -0700 Subject: [PATCH] Use the Kerberos context to set the error message 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. --- NEWS | 4 ++++ external/heimdal-strength.c | 21 ++++++++++++------- plugin/api.c | 41 ++++++++++++++++++++++++------------- plugin/api.h | 7 +++---- plugin/heimdal.c | 23 +++++++++++++++------ plugin/mit.c | 30 ++++++++------------------- tests/data/cracklib.json | 8 ++++---- tests/heimdal/external-t | 4 +++- tests/heimdal/plugin-t.c | 3 ++- 9 files changed, 82 insertions(+), 59 deletions(-) diff --git a/NEWS b/NEWS index a8fc32a..d6da648 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,10 @@ krb5-strength 2.0 (unreleased) and MIT. Drop the patch for MIT Kerberos 1.4 (and hence support for versions of MIT Kerberos prior to 1.9). + 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. + CrackLib checks for passwords where a character is a simple increment or decrement of the previous character. In previous versions, the embedded version of CrackLib allowed at most four such occurrences in diff --git a/external/heimdal-strength.c b/external/heimdal-strength.c index c74ffa8..9f7be35 100644 --- a/external/heimdal-strength.c +++ b/external/heimdal-strength.c @@ -60,7 +60,9 @@ read_key(const char *key, char *buffer, size_t length) static void check_password(krb5_context ctx, krb5_pwqual_moddata data) { - char principal[BUFSIZ], password[BUFSIZ], error[BUFSIZ], end[BUFSIZ]; + char principal[BUFSIZ], password[BUFSIZ], end[BUFSIZ]; + krb5_error_code code; + const char *message; read_key("principal", principal, sizeof(principal)); read_key("new-password", password, sizeof(password)); @@ -68,10 +70,14 @@ check_password(krb5_context ctx, krb5_pwqual_moddata data) sysdie("Cannot read end of entry"); if (strcmp(end, "end\n") != 0) die("Malformed end line"); - if (pwcheck_check(ctx, data, password, principal, error, sizeof(error))) - fprintf(stderr, "%s\n", error); - else + code = pwcheck_check(ctx, data, password, principal); + if (code == 0) printf("APPROVED\n"); + else { + message = krb5_get_error_message(ctx, code); + fprintf(stderr, "%s\n", message); + krb5_free_error_message(ctx, message); + } } @@ -96,9 +102,10 @@ main(int argc, char *argv[] UNUSED) /* Initialize Kerberos and the module. */ code = krb5_init_context(&ctx); if (code != 0) - die_krb5(ctx, code, "cannot create Kerberos context"); - if (pwcheck_init(ctx, NULL, &data) != 0) - die("cannot initialize strength checking"); + die_krb5(ctx, code, "Cannot create Kerberos context"); + code = pwcheck_init(ctx, NULL, &data); + if (code != 0) + die_krb5(ctx, code, "Cannot initialize strength checking"); /* Check the password and report results. */ check_password(ctx, data); diff --git a/plugin/api.c b/plugin/api.c index 1a343dd..0f31a6d 100644 --- a/plugin/api.c +++ b/plugin/api.c @@ -117,22 +117,31 @@ pwcheck_init(krb5_context ctx, const char *dictionary, default_string(ctx, "krb5-strength", "password_dictionary", &path); else { path = strdup(dictionary); - if (path == NULL) - return errno; + if (path == NULL) { + oerrno = errno; + krb5_set_error_message(ctx, oerrno, "cannot allocate memory"); + return oerrno; + } } /* If there is no dictionary, abort our setup with an error. */ - if (path == NULL) + if (path == NULL) { + krb5_set_error_message(ctx, KADM5_MISSING_CONF_PARAMS, + "password_dictionary not configured in krb5.conf"); return KADM5_MISSING_CONF_PARAMS; + } /* Sanity-check the dictionary path. */ if (asprintf(&file, "%s.pwd", path) < 0) { oerrno = errno; + krb5_set_error_message(ctx, oerrno, "cannot allocate memory"); free(path); return oerrno; } if (access(file, R_OK) != 0) { oerrno = errno; + krb5_set_error_message(ctx, oerrno, "dictionary %s does not exist", + file); free(path); free(file); return oerrno; @@ -143,6 +152,7 @@ pwcheck_init(krb5_context ctx, const char *dictionary, *data = malloc(sizeof(**data)); if (*data == NULL) { oerrno = errno; + krb5_set_error_message(ctx, oerrno, "cannot allocate memory"); free(path); return oerrno; } @@ -158,14 +168,13 @@ pwcheck_init(krb5_context ctx, const char *dictionary, */ krb5_error_code pwcheck_check(krb5_context ctx UNUSED, krb5_pwqual_moddata data, - const char *password, const char *principal, char *errstr, - int errstrlen) + const char *password, const char *principal) { char *user, *p; const char *q; size_t i, j; char c; - int err; + int oerrno; const char *result; /* @@ -175,14 +184,15 @@ pwcheck_check(krb5_context ctx UNUSED, krb5_pwqual_moddata data, * have to copy the string so that we can manipulate it a bit. */ if (strcasecmp(password, principal) == 0) { - snprintf(errstr, errstrlen, "Password based on username"); + krb5_set_error_message(ctx, KADM5_PASS_Q_GENERIC, + "password based on username"); return KADM5_PASS_Q_GENERIC; } user = strdup(principal); if (user == NULL) { - err = errno; - snprintf(errstr, errstrlen, "Cannot allocate memory"); - return err; + oerrno = errno; + krb5_set_error_message(ctx, oerrno, "cannot allocate memory"); + return oerrno; } for (p = user; p[0] != '\0'; p++) { if (p[0] == '\\' && p[1] != '\0') { @@ -197,7 +207,8 @@ pwcheck_check(krb5_context ctx UNUSED, krb5_pwqual_moddata data, if (strlen(password) == strlen(user)) { if (strcasecmp(password, user) == 0) { free(user); - snprintf(errstr, errstrlen, "Password based on username"); + krb5_set_error_message(ctx, KADM5_PASS_Q_GENERIC, + "password based on username"); return KADM5_PASS_Q_GENERIC; } @@ -209,7 +220,8 @@ pwcheck_check(krb5_context ctx UNUSED, krb5_pwqual_moddata data, } if (strcasecmp(password, user) == 0) { free(user); - snprintf(errstr, errstrlen, "Password based on username"); + krb5_set_error_message(ctx, KADM5_PASS_Q_GENERIC, + "password based on username"); return KADM5_PASS_Q_GENERIC; } } @@ -220,14 +232,15 @@ pwcheck_check(krb5_context ctx UNUSED, krb5_pwqual_moddata data, q++; if (*q == '\0') { free(user); - snprintf(errstr, errstrlen, "Password based on username"); + krb5_set_error_message(ctx, KADM5_PASS_Q_GENERIC, + "password based on username"); return KADM5_PASS_Q_GENERIC; } } free(user); result = FascistCheck(password, data->dictionary); if (result != NULL) { - strlcpy(errstr, result, errstrlen); + krb5_set_error_message(ctx, KADM5_PASS_Q_GENERIC, "%s", result); return KADM5_PASS_Q_GENERIC; } return 0; diff --git a/plugin/api.h b/plugin/api.h index 2ea7eb3..0a7668e 100644 --- a/plugin/api.h +++ b/plugin/api.h @@ -36,12 +36,11 @@ krb5_error_code pwcheck_init(krb5_context, const char *dictionary, krb5_pwqual_moddata *); /* - * Check a password. Returns 0 if okay, non-zero on rejection, and stores - * the rejection method in the provided errstr buffer. + * Check a password. Returns 0 if okay. On error, sets the Kerberos error + * message and returns a Kerberos status code. */ krb5_error_code pwcheck_check(krb5_context, krb5_pwqual_moddata, - const char *password, const char *principal, - char *errstr, int errstrlen); + const char *password, const char *principal); /* Finished checking passwords. Free internal data. */ void pwcheck_close(krb5_context, krb5_pwqual_moddata); diff --git a/plugin/heimdal.c b/plugin/heimdal.c index 4aa550b..d6583d6 100644 --- a/plugin/heimdal.c +++ b/plugin/heimdal.c @@ -71,7 +71,7 @@ heimdal_pwcheck(krb5_context ctx, krb5_principal principal, char *pastring; char *name = NULL; krb5_error_code code; - int result; + const char *error; pastring = malloc(password->length + 1); if (pastring == NULL) { @@ -81,23 +81,34 @@ heimdal_pwcheck(krb5_context ctx, krb5_principal principal, } memcpy(pastring, password->data, password->length); pastring[password->length] = '\0'; - if (pwcheck_init(ctx, NULL, &data) != 0) { - snprintf(message, length, "cannot initialize strength checking"); + code = pwcheck_init(ctx, NULL, &data); + if (code != 0) { + error = krb5_get_error_message(ctx, code); + snprintf(message, length, "cannot initialize strength checking: %s", + error); + krb5_free_error_message(ctx, error); free(pastring); return 1; } code = krb5_unparse_name(ctx, principal, &name); if (code != 0) { - strlcpy(message, "cannot unparse principal name", length); + error = krb5_get_error_message(ctx, code); + snprintf(message, length, "cannot unparse principal name: %s", error); + krb5_free_error_message(ctx, error); free(pastring); pwcheck_close(ctx, data); return 1; } - result = pwcheck_check(ctx, data, pastring, name, message, length); + code = pwcheck_check(ctx, data, pastring, name); + if (code != 0) { + error = krb5_get_error_message(ctx, code); + snprintf(message, length, "%s", error); + krb5_free_error_message(ctx, error); + } krb5_free_unparsed_name(ctx, name); free(pastring); pwcheck_close(ctx, data); - return (result == 0) ? 0 : 1; + return (code == 0) ? 0 : 1; } /* The public symbol that Heimdal looks for. */ diff --git a/plugin/mit.c b/plugin/mit.c index 0ef01b8..5649c34 100644 --- a/plugin/mit.c +++ b/plugin/mit.c @@ -41,18 +41,7 @@ krb5_error_code pwqual_strength_initvt(krb5_context, int, int, static krb5_error_code init(krb5_context ctx, const char *dictionary, krb5_pwqual_moddata *data) { - krb5_error_code code; - - code = pwcheck_init(ctx, dictionary, data); - if (code != 0) { - krb5_set_error_message(ctx, code, "cannot initialize strength" - " checking%s%s: %s", - dictionary == NULL ? "" : " with dictionary", - dictionary == NULL ? "" : dictionary, - strerror(errno)); - return code; - } - return 0; + return pwcheck_init(ctx, dictionary, data); } @@ -66,17 +55,14 @@ check(krb5_context ctx, krb5_pwqual_moddata data, const char *password, const char **languages UNUSED) { char *name = NULL; - krb5_error_code status; - char error[BUFSIZ]; - - status = krb5_unparse_name(ctx, princ, &name); - if (status != 0) - return status; - status = pwcheck_check(ctx, data, password, name, error, sizeof(error)); - if (status != 0) - krb5_set_error_message(ctx, status, "%s", error); + krb5_error_code code; + + code = krb5_unparse_name(ctx, princ, &name); + if (code != 0) + return code; + code = pwcheck_check(ctx, data, password, name); krb5_free_unparsed_name(ctx, name); - return status; + return code; } diff --git a/tests/data/cracklib.json b/tests/data/cracklib.json index e0178c8..be7ec4d 100644 --- a/tests/data/cracklib.json +++ b/tests/data/cracklib.json @@ -87,28 +87,28 @@ "principal": "someuser@EXAMPLE.ORG", "password": "someuser", "code": "KADM5_PASS_Q_GENERIC", - "error": "Password based on username" + "error": "password based on username" }, { "name": "based on principal (reversed)", "principal": "someuser@EXAMPLE.ORG", "password": "resuemos", "code": "KADM5_PASS_Q_GENERIC", - "error": "Password based on username" + "error": "password based on username" }, { "name": "based on principal with digits", "principal": "someuser@EXAMPLE.ORG", "password": "someuser123", "code": "KADM5_PASS_Q_GENERIC", - "error": "Password based on username" + "error": "password based on username" }, { "name": "is full principal", "principal": "test@EXAMPLE.ORG", "password": "test@EXAMPLE.ORG", "code": "KADM5_PASS_Q_GENERIC", - "error": "Password based on username" + "error": "password based on username" }, { "name": "long password complexity", diff --git a/tests/heimdal/external-t b/tests/heimdal/external-t index 0c862a2..66d4761 100755 --- a/tests/heimdal/external-t +++ b/tests/heimdal/external-t @@ -134,12 +134,14 @@ local $ENV{KRB5_CONFIG} = test_file_path('data/krb5.conf'); # Run a test with no Kerberos password dictionary configuration and check that # we get the correct error message. +my $error = 'Cannot initialize strength checking: password_dictionary not' + . ' configured in krb5.conf'; my $test = { name => 'no dictionary configured', principal => 'test@EXAMPLE.COM', password => 'password', status => 1, - error => 'cannot initialize strength checking', + error => $error, }; check_password($test); diff --git a/tests/heimdal/plugin-t.c b/tests/heimdal/plugin-t.c index 7567780..44153fe 100644 --- a/tests/heimdal/plugin-t.c +++ b/tests/heimdal/plugin-t.c @@ -150,7 +150,8 @@ main(void) "test@EXAMPLE.ORG", "password", 1, - "cannot initialize strength checking", + "cannot initialize strength checking: password_dictionary not" + " configured in krb5.conf", }; /* If we're not building with Heimdal, we can't run this test. */ -- 2.39.2