git: 87c06b7d026f - main - MAC/do: Output errors when parsing rules
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 16 Dec 2024 14:46:07 UTC
The branch main has been updated by olce:
URL: https://cgit.FreeBSD.org/src/commit/?id=87c06b7d026f2beeb3c2f695567ef72aa3a427ea
commit 87c06b7d026f2beeb3c2f695567ef72aa3a427ea
Author: Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-08-07 09:25:00 +0000
Commit: Olivier Certner <olce@FreeBSD.org>
CommitDate: 2024-12-16 14:42:39 +0000
MAC/do: Output errors when parsing rules
So that administrators can more easily know what the problem is with the
rules they are trying to set.
The new sysctl 'security.mac.do.print_parse_error' controls whether
trying to set sysctl 'security.mac.do.rules' with invalid rules triggers
printing of the error on the system console.
Setting jail parameters directlty reports an error to the calling
process thanks to the VFS options mechanism used by the jail machinery,
so is not controlled by the new sysctl setting.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47617
---
sys/security/mac_do/mac_do.c | 231 +++++++++++++++++++++++++++++++++++--------
1 file changed, 191 insertions(+), 40 deletions(-)
diff --git a/sys/security/mac_do/mac_do.c b/sys/security/mac_do/mac_do.c
index 92c09d540723..decfb3c756f0 100644
--- a/sys/security/mac_do/mac_do.c
+++ b/sys/security/mac_do/mac_do.c
@@ -23,6 +23,8 @@
#include <sys/ucred.h>
#include <sys/vnode.h>
+#include <machine/stdarg.h>
+
#include <security/mac/mac_policy.h>
static SYSCTL_NODE(_security_mac, OID_AUTO, do,
@@ -32,6 +34,11 @@ static int do_enabled = 1;
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
@@ -52,6 +59,13 @@ static const char *id_type_to_str[] = {
[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 'u_int' in conversions
* required for parsing rules specification strings.
@@ -364,8 +378,32 @@ strtoui_strict(const char *const restrict s, const char **const restrict endptr,
return (0);
}
+
+static void
+make_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
@@ -385,6 +423,7 @@ parse_id_type(const char *const string, id_type_t *const type)
}
*type = IT_INVALID;
+ make_parse_error(parse_error, 0, "No valid type found.");
return (EINVAL);
}
@@ -426,8 +465,10 @@ has_clauses(const id_nb_t nb, const flags_t type_flags)
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;
@@ -439,21 +480,30 @@ parse_target_clause(char *to, struct rule *const rule,
id_type_t type;
int error;
+ MPASS(*parse_error == NULL);
MPASS(to != NULL);
to_type = strsep(&to, "=");
MPASS(to_type != NULL);
to_type += parse_gid_flags(to_type, &is.flags, &gid_flags);
- error = parse_id_type(to_type, &type);
+ error = parse_id_type(to_type, &type, parse_error);
if (error != 0)
goto einval;
- if (type != IT_GID && is.flags != 0)
+ if (type != IT_GID && is.flags != 0) {
+ make_parse_error(parse_error, to_type - start,
+ "Expected type 'gid' after flags, not '%s'.",
+ to_type);
goto einval;
+ }
to_id = strsep(&to, "");
switch (type) {
case IT_GID:
- if (to_id == NULL)
+ if (to_id == NULL) {
+ make_parse_error(parse_error, to_type - start,
+ "No '=' and ID specification after type '%s'.",
+ to_type);
goto einval;
+ }
if (is.flags == 0) {
/* No flags: Dealing with a primary group. */
@@ -473,17 +523,33 @@ parse_target_clause(char *to, struct rule *const rule,
* supplementary groups).
*/
if ((is.flags & MDF_PRIMARY) != 0) {
- if ((*tflags & MDF_HAS_PRIMARY_CLAUSE) != 0)
+ if ((*tflags & MDF_HAS_PRIMARY_CLAUSE) != 0) {
+ make_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) != 0)
+ if ((is.flags & MDF_SUPP_MASK) != MDF_SUPP_ALLOW) {
+ make_parse_error(parse_error,
+ to_id - start,
+ "'any' specified with another "
+ "flag than '+'.");
+ goto einval;
+ }
+ if ((*tflags & MDF_HAS_SUPP_CLAUSE) != 0) {
+ make_parse_error(parse_error,
+ to_id - start,
+ "'any' with flag '+' specified after "
+ "another (supplementary) GID.");
goto einval;
+ }
*tflags |= gid_flags | MDF_ANY_SUPP;
}
goto check_type_and_finish;
@@ -493,18 +559,32 @@ parse_target_clause(char *to, struct rule *const rule,
* category.
*/
if ((is.flags & MDF_PRIMARY) != 0) {
- if ((*tflags & MDF_ANY) != 0)
+ if ((*tflags & MDF_ANY) != 0) {
+ make_parse_error(parse_error,
+ to_id - start,
+ "Some (primary) GID specified after "
+ "'any'.");
goto einval;
+ }
} else if ((*tflags & MDF_ANY_SUPP) != 0 &&
- (is.flags & MDF_SUPP_ALLOW) != 0)
+ (is.flags & MDF_SUPP_ALLOW) != 0) {
+ make_parse_error(parse_error,
+ to_id - start,
+ "Some (supplementary) GID specified after "
+ "'any' with flag '+'.");
goto einval;
+ }
*tflags |= gid_flags;
}
break;
case IT_UID:
- if (to_id == NULL)
+ if (to_id == NULL) {
+ make_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;
@@ -513,8 +593,11 @@ parse_target_clause(char *to, struct rule *const rule,
/* "*" or "any"? */
if (parse_any(to_id)) {
/* There must not be any other clause. */
- if (has_clauses(*nb, *tflags))
+ if (has_clauses(*nb, *tflags)) {
+ make_parse_error(parse_error, to_id - start,
+ "'any' specified after another UID.");
goto einval;
+ }
*tflags |= MDF_ANY;
goto check_type_and_finish;
} else {
@@ -522,22 +605,32 @@ parse_target_clause(char *to, struct rule *const rule,
* Check that we haven't already seen "any" for the same
* category.
*/
- if ((*tflags & MDF_ANY) != 0)
+ if ((*tflags & MDF_ANY) != 0) {
+ make_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) {
+ make_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)) {
+ make_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;
@@ -556,8 +649,12 @@ parse_target_clause(char *to, struct rule *const rule,
if ((*tflags & MDF_CURRENT) != 0) {
/* Duplicate "." <id>. Try to coalesce. */
error = coalesce_id_flags(is.flags, tflags);
- if (error != 0)
+ if (error != 0) {
+ make_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;
@@ -565,8 +662,11 @@ parse_target_clause(char *to, struct rule *const rule,
/* Parse an ID. */
error = strtoui_strict(to_id, &p, 10, &is.id);
- if (error != 0 || *p != '\0')
+ if (error != 0 || *p != '\0') {
+ make_parse_error(parse_error, to_id - start,
+ "Cannot parse a numerical ID (base 10).");
goto einval;
+ }
/* Explicit ID flags. */
if (type == IT_GID && (is.flags & MDF_SUPP_MUST) != 0)
@@ -578,18 +678,22 @@ parse_target_clause(char *to, struct rule *const rule,
* (using sorted arrays).
*/
++*nb;
- if (*nb == 0)
+ if (*nb == 0) {
+ make_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);
}
@@ -620,7 +724,8 @@ id_spec_cmp(const void *const p1, const void *const p2)
*/
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;
@@ -658,8 +763,12 @@ pour_list_into_rule(const id_type_t type, struct id_list *const list,
case IT_GID:
error = coalesce_id_flags(array[idx].flags,
&array[ref_idx].flags);
- if (error != 0)
+ if (error != 0) {
+ make_parse_error(parse_error, 0,
+ "Incompatible flags or duplicate "
+ "GID %u.", id);
return (EINVAL);
+ }
check_type_and_id_flags(type,
array[ref_idx].flags);
break;
@@ -670,6 +779,8 @@ pour_list_into_rule(const id_type_t type, struct id_list *const list,
* of the same UID is an exact redundancy, so
* error out.
*/
+ make_parse_error(parse_error, 0,
+ "Duplicate UID %u.", id);
return (EINVAL);
default:
@@ -697,8 +808,10 @@ pour_list_into_rule(const id_type_t type, struct id_list *const list,
* 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;
@@ -706,7 +819,7 @@ parse_single_rule(char *rule, struct rules *const rules)
struct rule *new;
int error;
- MPASS(rule != NULL);
+ MPASS(*parse_error == NULL);
TAILQ_INIT(&uid_list);
TAILQ_INIT(&gid_list);
@@ -715,7 +828,7 @@ parse_single_rule(char *rule, struct rules *const rules)
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) {
@@ -723,16 +836,23 @@ parse_single_rule(char *rule, struct rules *const rules)
case IT_GID:
break;
default:
+ make_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)) {
+ make_parse_error(parse_error, 0, "No ID specified.");
goto einval;
+ }
error = strtoui_strict(from_id, &p, 10, &new->from_id);
- if (error != 0 || *p != '\0')
+ if (error != 0 || *p != '\0') {
+ make_parse_error(parse_error, from_id - start,
+ "Cannot parse a numerical ID (base 10).");
goto einval;
+ }
/*
* We will now parse the "to" list.
@@ -747,12 +867,17 @@ parse_single_rule(char *rule, struct rules *const rules)
* O(log(n)) instead of linearly.
*/
to_list = strsep(&rule, ",");
- if (to_list == NULL)
+ if (to_list == NULL) {
+ make_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);
@@ -761,7 +886,7 @@ parse_single_rule(char *rule, struct rules *const rules)
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;
}
@@ -777,7 +902,7 @@ parse_single_rule(char *rule, struct rules *const rules)
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;
}
@@ -801,6 +926,7 @@ einval:
free(ie, M_DO);
TAILQ_FOREACH_SAFE(ie, &uid_list, ie_entries, ie_next)
free(ie, M_DO);
+ MPASS(*parse_error != NULL);
return (EINVAL);
}
@@ -811,7 +937,9 @@ einval:
* 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
* "<from>:<target>". The <from> part is of the form "<type>=<id>" where <type>
@@ -825,15 +953,22 @@ einval:
* - "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) {
+ make_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);
@@ -847,8 +982,9 @@ parse_rules(const char *const string, struct rules **const rulesp)
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;
}
@@ -957,12 +1093,13 @@ set_empty_rules(struct prison *const pr)
* 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);
@@ -976,6 +1113,7 @@ mac_do_sysctl_rules(SYSCTL_HANDLER_ARGS)
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);
@@ -987,7 +1125,13 @@ mac_do_sysctl_rules(SYSCTL_HANDLER_ARGS)
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);
@@ -1137,6 +1281,7 @@ mac_do_jail_set(void *obj, void *data)
struct prison *pr = obj;
struct vfsoptlist *opts = data;
char *rules_string;
+ struct parse_error *parse_error;
int error, jsys;
/*
@@ -1182,7 +1327,13 @@ mac_do_jail_set(void *obj, void *data)
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();