diff --git a/sys/security/mac_do/mac_do.c b/sys/security/mac_do/mac_do.c --- a/sys/security/mac_do/mac_do.c +++ b/sys/security/mac_do/mac_do.c @@ -24,6 +24,8 @@ #include #include +#include + #include static SYSCTL_NODE(_security_mac, OID_AUTO, do, @@ -33,6 +35,11 @@ SYSCTL_INT(_security_mac_do, OID_AUTO, enabled, CTLFLAG_RWTUN, &do_enabled, 0, "Enforce do policy"); +static int print_parse_error = 1; +SYSCTL_INT(_security_mac_do, OID_AUTO, print_parse_error, CTLFLAG_RWTUN, + &print_parse_error, 0, "Print parse errors on setting rules " + "(via sysctl(8))."); + static MALLOC_DEFINE(M_DO, "do_rule", "Rules for mac_do"); #define MAC_RULE_STRING_LEN 1024 @@ -53,6 +60,13 @@ [IT_ANY] = "*", }; +#define PARSE_ERROR_SIZE 256 + +struct parse_error { + size_t pos; + char msg[PARSE_ERROR_SIZE]; +}; + /* * We assume that 'uid_t' and 'gid_t' are aliases to 'int' in conversions * required for parsing rules specification strings. @@ -351,8 +365,32 @@ return ((int)l); } + +static void +alloc_parse_error(struct parse_error **const parse_error, const size_t pos, + const char *const fmt, ...) +{ + struct parse_error *const err = malloc(sizeof(*err), M_DO, M_WAITOK); + va_list ap; + + err->pos = pos; + va_start(ap, fmt); + vsnprintf(err->msg, PARSE_ERROR_SIZE, fmt, ap); + va_end(ap); + + MPASS(*parse_error == NULL); + *parse_error = err; +} + +static void +free_parse_error(struct parse_error *const parse_error) +{ + free(parse_error, M_DO); +} + static int -parse_id_type(const char *const string, id_type_t *const type) +parse_id_type(const char *const string, id_type_t *const type, + struct parse_error **const parse_error) { /* * Special case for "any", as the canonical form for IT_ANY in @@ -372,6 +410,7 @@ } *type = IT_INVALID; + alloc_parse_error(parse_error, 0, "No valid type found."); return (EINVAL); } @@ -389,8 +428,10 @@ static int parse_target_clause(char *to, struct rule *const rule, - struct id_list *const uid_list, struct id_list *const gid_list) + struct id_list *const uid_list, struct id_list *const gid_list, + struct parse_error **const parse_error) { + const char *const start = to; char *to_type, *to_id; const char *p; struct id_list *list; @@ -402,18 +443,23 @@ id_type_t type; int error; + MPASS(*parse_error == NULL); + MPASS(to != NULL); to_type = strsep(&to, "="); - if (to_type == NULL) - goto einval; - error = parse_id_type(to_type, &type); + MPASS(to_type != NULL); + error = parse_id_type(to_type, &type, parse_error); if (error != 0) goto einval; to_id = strsep(&to, ""); switch (type) { case IT_GID: - if (to_id == NULL) + if (to_id == NULL) { + alloc_parse_error(parse_error, to_type - start, + "No '=' and ID specification after type '%s'.", + to_type); goto einval; + } /* * This is easily wrapped into a loop for when/if we allow @@ -453,17 +499,33 @@ * supplementary groups). */ if (is.flags & MDF_PRIMARY) { - if (*tflags & MDF_HAS_PRIMARY_CLAUSE) + if (*tflags & MDF_HAS_PRIMARY_CLAUSE) { + alloc_parse_error(parse_error, + to_id - start, + "'any' specified after another " + "(primary) GID."); goto einval; + } *tflags |= gid_flags | MDF_ANY; } else { /* * If a supplementary group flag was present, it * must be MDF_SUPP_ALLOW ("+"). */ - if ((is.flags & MDF_SUPP_MASK) != MDF_SUPP_ALLOW || - (*tflags & MDF_HAS_SUPP_CLAUSE)) + if ((is.flags & MDF_SUPP_MASK) != MDF_SUPP_ALLOW) { + alloc_parse_error(parse_error, + to_id - start, + "'any' specified with another " + "flag than '+'."); goto einval; + } + if (*tflags & MDF_HAS_SUPP_CLAUSE) { + alloc_parse_error(parse_error, + to_id - start, + "'+any' specified after another " + "(supplementary) GID."); + goto einval; + } *tflags |= gid_flags | MDF_ANY_SUPP; } goto check_type_and_finish; @@ -473,17 +535,32 @@ * category. */ if ((is.flags & MDF_PRIMARY)) { - if (*tflags & MDF_ANY) + if (*tflags & MDF_ANY) { + alloc_parse_error(parse_error, + to_id - start, + "Some (primary) GID specified after " + "'any'."); goto einval; - } else if (*tflags & MDF_ANY_SUPP) + } + } else if (*tflags & MDF_ANY_SUPP && + is.flags & MDF_SUPP_ALLOW) { + alloc_parse_error(parse_error, + to_id - start, + "Some (supplementary) GID specified after " + "'+any'."); goto einval; + } *tflags |= gid_flags; } break; case IT_UID: - if (to_id == NULL) + if (to_id == NULL) { + alloc_parse_error(parse_error, to_type - start, + "No '=' and ID specification after type '%s'.", + to_type); goto einval; + } list = uid_list; nb = &rule->uids_nb; @@ -492,8 +569,11 @@ /* "*" or "any"? */ if (parse_any(to_id)) { /* There must not be any other clause. */ - if (has_clauses(*nb, *tflags)) + if (has_clauses(*nb, *tflags)) { + alloc_parse_error(parse_error, to_id - start, + "'any' specified after another UID."); goto einval; + } *tflags |= MDF_ANY; goto check_type_and_finish; } else { @@ -501,22 +581,32 @@ * Check that we haven't already seen "any" for the same * category. */ - if (*tflags & MDF_ANY) + if (*tflags & MDF_ANY) { + alloc_parse_error(parse_error, to_id - start, + "Some UID specified after 'any'."); goto einval; + } } break; case IT_ANY: /* No ID allowed. */ - if (to_id != NULL) + if (to_id != NULL) { + alloc_parse_error(parse_error, to_type - start, + "No '=' and ID allowed after type '%s'.", to_type); goto einval; + } /* * We can't have IT_ANY after any other IT_*, it must be the * only one. */ if (has_clauses(rule->uids_nb, rule->uid_flags) || - has_clauses(rule->gids_nb, rule->gid_flags)) + has_clauses(rule->gids_nb, rule->gid_flags)) { + alloc_parse_error(parse_error, to_type - start, + "Target clause of type '%s' coming after another " + "clause (must be alone).", to_type); goto einval; + } rule->uid_flags |= MDF_ANY; rule->gid_flags |= MDF_ANY | MDF_ANY_SUPP | MDF_HAS_PRIMARY_CLAUSE | MDF_HAS_SUPP_CLAUSE; @@ -535,8 +625,12 @@ if (*tflags & MDF_CURRENT) { /* Duplicate "." . Try to coalesce. */ error = coalesce_id_flags(is.flags, tflags); - if (error != 0) + if (error != 0) { + alloc_parse_error(parse_error, to_id - start, + "Incompatible flags with prior clause " + "with same target."); goto einval; + } } else *tflags |= MDF_CURRENT | is.flags; goto check_type_and_finish; @@ -544,8 +638,11 @@ /* Parse an ID. */ is.id = strtonni(to_id, &p, 10); - if (is.id < 0 || *p != '\0') + if (is.id < 0 || *p != '\0') { + alloc_parse_error(parse_error, to_id - start, + "Cannot parse a numerical ID (base 10, no sign)."); goto einval; + } /* * We check for duplicate IDs and coalesce their 'struct id_spec' only @@ -553,18 +650,22 @@ * (using sorted arrays). */ ++*nb; - if (*nb == 0) + if (*nb == 0) { + alloc_parse_error(parse_error, 0, + "Too many target clauses of type '%s'.", to_type); return (EOVERFLOW); + } ie = malloc(sizeof(*ie), M_DO, M_WAITOK); ie->spec = is; TAILQ_INSERT_TAIL(list, ie, ie_entries); check_type_and_id_spec(type, &is); -finish: - return (0); check_type_and_finish: check_type_and_type_flags(type, *tflags); +finish: return (0); einval: + /* We must have built a parse error on error. */ + MPASS(*parse_error != NULL); return (EINVAL); } @@ -595,7 +696,8 @@ */ static int pour_list_into_rule(const id_type_t type, struct id_list *const list, - struct id_spec *const array, id_nb_t *const nb) + struct id_spec *const array, id_nb_t *const nb, + struct parse_error **const parse_error) { struct id_elem *ie, *ie_next; size_t idx = 0; @@ -625,8 +727,12 @@ coalesce_id_flags(array[idx].flags, &array[ref_idx].flags); - if (error != 0) + if (error != 0) { + alloc_parse_error(parse_error, 0, + "Incompatible or duplicate flags " + "for ID %d.", id); return (EINVAL); + } check_type_and_id_flags(type, array[ref_idx].flags); } @@ -657,8 +763,10 @@ * explained in functions checking privileges below. */ static int -parse_single_rule(char *rule, struct rules *const rules) +parse_single_rule(char *rule, struct rules *const rules, + struct parse_error **const parse_error) { + const char *const start = rule; const char *from_type, *from_id, *p; char *to_list; struct id_list uid_list, gid_list; @@ -666,7 +774,7 @@ struct rule *new; int error; - MPASS(rule != NULL); + MPASS(*parse_error == NULL); TAILQ_INIT(&uid_list); TAILQ_INIT(&gid_list); @@ -675,7 +783,7 @@ from_type = strsep(&rule, "="); MPASS(from_type != NULL); /* Because 'rule' was not NULL. */ - error = parse_id_type(from_type, &new->from_type); + error = parse_id_type(from_type, &new->from_type, parse_error); if (error != 0) goto einval; switch (new->from_type) { @@ -683,16 +791,23 @@ case IT_GID: break; default: + alloc_parse_error(parse_error, 0, "Type '%s' not allowed in " + "the \"from\" part of rules."); goto einval; } from_id = strsep(&rule, ":"); - if (is_null_or_empty(from_id)) + if (is_null_or_empty(from_id)) { + alloc_parse_error(parse_error, 0, "No ID specified."); goto einval; + } new->from_id = strtonni(from_id, &p, 10); - if (new->from_id < 0 || *p != '\0') + if (new->from_id < 0 || *p != '\0') { + alloc_parse_error(parse_error, from_id - start, + "Cannot parse a numerical ID (base 10, no sign)."); goto einval; + } /* * We will now parse the "to" list. @@ -707,12 +822,17 @@ * O(log(n)) instead of linearly. */ to_list = strsep(&rule, ","); - if (to_list == NULL) + if (to_list == NULL) { + alloc_parse_error(parse_error, 0, "No target list."); goto einval; + } do { - error = parse_target_clause(to_list, new, &uid_list, &gid_list); - if (error != 0) + error = parse_target_clause(to_list, new, &uid_list, &gid_list, + parse_error); + if (error != 0) { + (*parse_error)->pos += to_list - start; goto einval; + } to_list = strsep(&rule, ","); } while (to_list != NULL); @@ -721,7 +841,7 @@ new->uids = malloc(sizeof(*new->uids) * new->uids_nb, M_DO, M_WAITOK); error = pour_list_into_rule(IT_UID, &uid_list, new->uids, - &new->uids_nb); + &new->uids_nb, parse_error); if (error != 0) goto einval; } @@ -737,7 +857,7 @@ new->gids = malloc(sizeof(*new->gids) * new->gids_nb, M_DO, M_WAITOK); error = pour_list_into_rule(IT_GID, &gid_list, new->gids, - &new->gids_nb); + &new->gids_nb, parse_error); if (error != 0) goto einval; } @@ -761,6 +881,7 @@ free(ie, M_DO); TAILQ_FOREACH_SAFE(ie, &uid_list, ie_entries, ie_next) free(ie, M_DO); + MPASS(*parse_error != NULL); return (EINVAL); } @@ -771,7 +892,9 @@ * representing the rules. On error, the returned value is non-zero and * '*rulesp' is unchanged. If 'string' has length greater or equal to * MAC_RULE_STRING_LEN, ENAMETOOLONG is returned. If it is not in the expected - * format, EINVAL is returned. + * format, EINVAL is returned. If an error is returned, '*parse_error' is set + * to point to a 'struct parse_error' giving an error message for the problem, + * else '*parse_error' is set to NULL. * * Expected format: A semi-colon-separated list of rules of the form * ":". The part is of the form "=" where @@ -785,15 +908,22 @@ * - "gid=1010:gid=1011,gid=1012,gid=1013" */ static int -parse_rules(const char *const string, struct rules **const rulesp) +parse_rules(const char *const string, struct rules **const rulesp, + struct parse_error **const parse_error) { const size_t len = strlen(string); char *copy, *p, *rule; struct rules *rules; int error = 0; - if (len >= MAC_RULE_STRING_LEN) + *parse_error = NULL; + + if (len >= MAC_RULE_STRING_LEN) { + alloc_parse_error(parse_error, 0, + "Rule specification string is too long (%zu, max %zu)", + len, MAC_RULE_STRING_LEN - 1); return (ENAMETOOLONG); + } rules = alloc_rules(); bcopy(string, rules->string, len + 1); @@ -807,8 +937,9 @@ while ((rule = strsep(&p, ";")) != NULL) { if (rule[0] == '\0') continue; - error = parse_single_rule(rule, rules); + error = parse_single_rule(rule, rules, parse_error); if (error != 0) { + (*parse_error)->pos += rule - copy; toast_rules(rules); goto out; } @@ -917,12 +1048,13 @@ * Returns the same error code as parse_rules() (which see). */ static int -parse_and_set_rules(struct prison *const pr, const char *rules_string) +parse_and_set_rules(struct prison *const pr, const char *rules_string, + struct parse_error **const parse_error) { struct rules *rules; int error; - error = parse_rules(rules_string, &rules); + error = parse_rules(rules_string, &rules, parse_error); if (error != 0) return (error); set_rules(pr, rules); @@ -936,6 +1068,7 @@ struct prison *const td_pr = req->td->td_ucred->cr_prison; struct prison *pr; struct rules *rules; + struct parse_error *parse_error; int error; rules = find_rules(td_pr, &pr); @@ -947,7 +1080,13 @@ goto out; /* Set our prison's rules, not that of the jail we inherited from. */ - error = parse_and_set_rules(td_pr, buf); + error = parse_and_set_rules(td_pr, buf, &parse_error); + if (error != 0) { + if (print_parse_error) + printf("MAC/do: Parse error at index %zu: %s\n", + parse_error->pos, parse_error->msg); + free_parse_error(parse_error); + } out: free(buf, M_DO); return (error); @@ -1090,6 +1229,7 @@ struct prison *pr = obj; struct vfsoptlist *opts = data; char *rules_string; + struct parse_error *parse_error; int error, jsys; /* @@ -1135,7 +1275,13 @@ break; case JAIL_SYS_DISABLE: case JAIL_SYS_NEW: - error = parse_and_set_rules(pr, rules_string); + error = parse_and_set_rules(pr, rules_string, &parse_error); + if (error != 0) { + vfs_opterror(opts, + "MAC/do: Parse error at index %zu: %s\n", + parse_error->pos, parse_error->msg); + free_parse_error(parse_error); + } break; default: __assert_unreachable();