From 89bae59b49b7bc0ccbfa7aab59397651b4558f4d Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Thu, 1 Dec 2011 13:12:45 -0600 Subject: [PATCH] Fix error clobbering bug and code review comments --- appl/dbutils/bsearch.c | 34 ++++------------ lib/krb5/aname_to_localname.c | 62 +++++++++++++++++------------ lib/krb5/test_alname.c | 75 ++++++++++++++++++++++------------- tests/kdc/check-an2ln.in | 12 ++++++ 4 files changed, 104 insertions(+), 79 deletions(-) diff --git a/appl/dbutils/bsearch.c b/appl/dbutils/bsearch.c index 34aa6bae1..b6750994d 100644 --- a/appl/dbutils/bsearch.c +++ b/appl/dbutils/bsearch.c @@ -66,29 +66,9 @@ struct getargs args[] = { static int num_args = sizeof(args) / sizeof(args[0]); static void -usage(const char *progname, int status) +usage(int status) { - arg_printusage(args, num_args, progname, "\n" -"\tThis program does a binary search of the given file for the\n" -"\tgiven keys. Two binary search algorithms are implemented\n" -"\twhole-file and block-wise.\n\n" -"\tIf keys are not given as arguments keys are read from stdin.\n\n" -"\tExit status will be 1 for errors, 2 if any keys are not found,\n" -"\tand 0 if all keys are found.\n\n" -"\tOptions:\n" -"\t\t-K \tPrint keys\n" -"\t\t-V \tDon't print values\n" -"\t\t-b size\tUse block-wise search with give blocksize\n" -"\t\t-m size\tRead DB in if its size is less than given\n" -"\t\t-v \tVerbose (includes count of reads and comparisons)\n" -"\t\t-h \tPrint usage message and exit\n" -"\tIf blocksize is not given, empty, or zero then the\n" -"\tfilesystem's block size (st_blksize) will be used.\n" -"\tBlock sizes should be powers of two, and larger than 256.\n" -"\tIf the max file size is not given or empty then the max\n" -"\tfile size for non-block-wise search will be 1MB.\n" -"\tKeys from stdin must not be longer than 1023 bytes.\n\n" - ); + arg_printusage(args, num_args, NULL, "file [key ...]"); exit(status); } @@ -99,7 +79,6 @@ int main(int argc, char **argv) { char keybuf[1024]; - char *progname = argv[0]; char *fname; char *key = keybuf; char *value; @@ -116,8 +95,9 @@ main(int argc, char **argv) int blockwise; int ret = 0; + setprogname(argv[0]); if (getarg(args, num_args, argc, argv, &optidx)) - usage(progname, 1); + usage(1); if (version_flag) { print_version(NULL); @@ -125,7 +105,7 @@ main(int argc, char **argv) } if (help_flag) - usage(progname, 0); + usage(0); if (block_size_int != 0 && block_size_int < 512) { fprintf(stderr, "Invalid block size: too small\n"); @@ -147,14 +127,14 @@ main(int argc, char **argv) block_size = block_size_int; } if (max_size_int < 0) - usage(progname, 1); + usage(1); max_size = max_size_int; argc -= optind; argv += optind; if (argc == 0) - usage(progname, 1); + usage(1); fname = argv[0]; argc--; diff --git a/lib/krb5/aname_to_localname.c b/lib/krb5/aname_to_localname.c index 6defbea53..e4af81084 100644 --- a/lib/krb5/aname_to_localname.c +++ b/lib/krb5/aname_to_localname.c @@ -186,7 +186,7 @@ an2ln_plugin(krb5_context context, const char *rule, krb5_const_principal aname, ret = KRB5_CONFIG_NOTENUFSPACE; heim_release(ctx.luser); - return 0; + return ret; } static void @@ -287,12 +287,23 @@ an2ln_local_names(krb5_context context, */ static krb5_error_code an2ln_default(krb5_context context, - int root_princs_ok, + char *rule, krb5_const_principal aname, size_t lnsize, char *lname) { krb5_error_code ret; const char *res; + int root_princs_ok; + + if (strcmp(rule, "NONE") == 0) + return KRB5_NO_LOCALNAME; + + if (strcmp(rule, "DEFAULT") == 0) + root_princs_ok = 0; + else if (strcmp(rule, "HEIMDAL_DEFAULT") == 0) + root_princs_ok = 1; + else + return KRB5_PLUGIN_NO_HANDLE; if (!princ_realm_is_default(context, aname)) return KRB5_PLUGIN_NO_HANDLE; @@ -303,7 +314,7 @@ an2ln_default(krb5_context context, * component is the username. */ res = aname->name.name_string.val[0]; - } else if (aname->name.name_string.len == 2 && + } else if (root_princs_ok && aname->name.name_string.len == 2 && strcmp (aname->name.name_string.val[1], "root") == 0) { /* * Two-component principal names in default realm where the @@ -372,41 +383,42 @@ krb5_aname_to_localname(krb5_context context, "auth_to_local", NULL); if (!rules) { /* Heimdal's default rule */ - ret = an2ln_default(context, 1, aname, lnsize, lname); + ret = an2ln_default(context, "HEIMDAL_DEFAULT", aname, lnsize, lname); if (ret == KRB5_PLUGIN_NO_HANDLE) return KRB5_NO_LOCALNAME; return ret; } - /* MIT rules */ + /* + * MIT rules. + * + * Note that RULEs and DBs only have white-list functionality, + * thus RULEs and DBs that we don't understand we simply ignore. + * + * This means that plugins that implement black-lists are + * dangerous: if a black-list plugin isn't found, the black-list + * won't be enforced. But black-lists are dangerous anyways. + */ for (ret = KRB5_PLUGIN_NO_HANDLE, i = 0; rules[i]; i++) { rule = rules[i]; - if (!*rule || strcmp(rule, "NONE") == 0) - break; - else if (strcmp(rule, "HEIMDAL_DEFAULT") == 0) - ret = an2ln_default(context, 1, aname, lnsize, lname); - else if (strcmp(rule, "DEFAULT") == 0) - ret = an2ln_default(context, 0, aname, lnsize, lname); - else - /* Let the plugins handle DBs and RULEs and anything else*/ + + /* Try NONE, DEFAULT, and HEIMDAL_DEFAULT rules */ + ret = an2ln_default(context, rule, aname, lnsize, lname); + if (ret == KRB5_PLUGIN_NO_HANDLE) + /* Try DB, RULE, ... plugins */ ret = an2ln_plugin(context, rule, aname, lnsize, lname); - if (ret == 0 && lnsize && lname[0]) - break; - /* - * Note that RULEs and DBs only have white-list functionality, - * thus RULEs and DBs that we don't understand we simply ignore. - * - * This means that plugins that implement black-lists are - * dangerous: if a black-list plugin isn't found, the black-list - * won't be enforced. But black-lists are dangerous anyways. - */ - if (ret != KRB5_PLUGIN_NO_HANDLE) + if (ret == 0 && lnsize && !lname[0]) + continue; /* Success but no lname?! lies! */ + else if (ret != KRB5_PLUGIN_NO_HANDLE) break; } - if (ret == KRB5_PLUGIN_NO_HANDLE) + if (ret == KRB5_PLUGIN_NO_HANDLE) { + if (lnsize) + lname[0] = '\0'; ret = KRB5_NO_LOCALNAME; + } krb5_config_free_strings(rules); return ret; diff --git a/lib/krb5/test_alname.c b/lib/krb5/test_alname.c index 79f8ef273..a367b0aba 100644 --- a/lib/krb5/test_alname.c +++ b/lib/krb5/test_alname.c @@ -34,13 +34,33 @@ #include #include +char localname[1024]; +static size_t lname_size = sizeof (localname); +static int lname_size_arg = 0; +static int simple_flag = 0; +static int verbose_flag = 0; +static int version_flag = 0; +static int help_flag = 0; + +static struct getargs args[] = { + {"lname-size", 0, arg_integer, &lname_size_arg, + "set localname size (0 means use default, must be 0..1023)", "integer" }, + {"simple", 0, arg_flag, &simple_flag, /* Used for scripting */ + "map the given principal and print the resulting localname", NULL }, + {"verbose", 0, arg_flag, &verbose_flag, + "print the actual principal name as well as the localname", NULL }, + {"version", 0, arg_flag, &version_flag, + "print version", NULL }, + {"help", 0, arg_flag, &help_flag, + NULL, NULL } +}; + static void test_alname(krb5_context context, krb5_const_realm realm, const char *user, const char *inst, const char *localuser, int ok) { krb5_principal p; - char localname[1024]; krb5_error_code ret; char *princ; @@ -52,7 +72,7 @@ test_alname(krb5_context context, krb5_const_realm realm, if (ret) krb5_err(context, 1, ret, "krb5_unparse_name"); - ret = krb5_aname_to_localname(context, p, sizeof(localname), localname); + ret = krb5_aname_to_localname(context, p, lname_size, localname); krb5_free_principal(context, p); if (ret) { if (!ok) { @@ -76,22 +96,6 @@ test_alname(krb5_context context, krb5_const_realm realm, } -static int simple_flag = 0; -static int verbose_flag = 0; -static int version_flag = 0; -static int help_flag = 0; - -static struct getargs args[] = { - {"simple", 0, arg_flag, &simple_flag, /* Used for scripting */ - "map the given principal and print the resulting localname", NULL }, - {"verbose", 0, arg_flag, &verbose_flag, - "print the actual principal name as well as the localname", NULL }, - {"version", 0, arg_flag, &version_flag, - "print version", NULL }, - {"help", 0, arg_flag, &help_flag, - NULL, NULL } -}; - static void usage (int ret) { @@ -133,9 +137,9 @@ main(int argc, char **argv) if (simple_flag) { krb5_principal princ; - char localname[1024]; char *unparsed; krb5_error_code ret; + int status = 0; /* Map then print the result and exit */ if (argc != 1) @@ -149,23 +153,40 @@ main(int argc, char **argv) if (ret) krb5_err(context, 1, ret, "krb5_unparse_name"); - ret = krb5_aname_to_localname(context, princ, sizeof(localname), - localname); + if (lname_size_arg > 0 && lname_size_arg < 1024) + lname_size = lname_size_arg; + else if (lname_size_arg != 0) + errx(1, "local name size must be between 0 and 1023 (inclusive)"); + + ret = krb5_aname_to_localname(context, princ, lname_size, localname); if (ret == KRB5_NO_LOCALNAME) { if (verbose_flag) fprintf(stderr, "No mapping obtained for %s\n", unparsed); exit(1); } - if (ret == KRB5_PLUGIN_NO_HANDLE) { + switch (ret) { + case KRB5_PLUGIN_NO_HANDLE: fprintf(stderr, "Error: KRB5_PLUGIN_NO_HANDLE leaked!\n"); - exit(2); + status = 2; + break; + case KRB5_CONFIG_NOTENUFSPACE: + fprintf(stderr, "Error: lname-size (%lu) too small\n", + (long unsigned)lname_size); + status = 3; + break; + case 0: + if (verbose_flag) + printf("%s ", unparsed); + printf("%s\n", localname); + break; + default: + krb5_err(context, 4, ret, "krb5_aname_to_localname"); + break; } - if (verbose_flag) - printf("%s ", unparsed); free(unparsed); krb5_free_principal(context, princ); - printf("%s\n", localname); - exit(0); + krb5_free_context(context); + exit(status); } if (argc != 1) diff --git a/tests/kdc/check-an2ln.in b/tests/kdc/check-an2ln.in index d4a6bc295..d10d0a8d4 100644 --- a/tests/kdc/check-an2ln.in +++ b/tests/kdc/check-an2ln.in @@ -43,6 +43,9 @@ test_alname="${test_alname} --simple" rm -f localname check_localname() { + if test "$2" -ne 0; then + exec 2> /dev/null + fi ${test_alname} "$1" > localname status=$? if test $status -ne "$2"; then @@ -99,6 +102,15 @@ check_localname bar/mapped1@${R4} 1 check_localname foo/notmapped1@${R4} 1 check_localname bar/notmapped1@${R4} 1 +echo "Checking for overflow" +test_alname="${test_alname} --simple --lname-size=1" +check_localname mapped1@${R} 3 +check_localname mapped2@${R} 3 +check_localname mapped1@${R2} 3 +check_localname mapped2@${R2} 3 +check_localname mapped1@${R3} 3 +check_localname mapped2@${R3} 3 + rm -f messages.log exit 0