Re: test patch for auditctl inter-field comparisons on euid/uid, egid/gid

Steve Grubb
2012-02-28T23:07:33Z

On Wednesday, December 14, 2011 08:18:30 PM Peter Moody wrote:
> > Testing has revealed a couple of bugs in this. Fixing these and
> > testing some more now.
>
> Ok, this should be the last one. I was apparently a little more tired
> riding the shuttle home than I thought I was. The patch last night was
> missing a break and a number of the AUDIT_COMPARE_* fields were plain
> wrong.
>
> This one tests good to me though.

This patch + some changes and additions has been applied.
https://fedorahosted.org/audit/changeset/642

If I could go back in time, I would have suggested using a bitmap for left and
right side so we could avoid the big switch/case constructs. But it works fine
the way it is. :)

-Steve

> Signed-off-by: Peter Moody <pmoody@google.com>
> ---
> trunk/auparse/typetab.h | 2 +
> trunk/lib/errormsg.h | 7 +-
> trunk/lib/fieldtab.h | 3 +
> trunk/lib/libaudit.c | 303
> +++++++++++++++++++++++++++++++++++++++++++ trunk/lib/libaudit.h |
> 2 +
> trunk/src/auditctl.c | 19 +++-
> trunk/src/ausearch-report.c | 1 +
> 7 files changed, 334 insertions(+), 3 deletions(-)
>
> diff --git a/trunk/auparse/typetab.h b/trunk/auparse/typetab.h
> index 746573c..cf13f4d 100644
> --- a/trunk/auparse/typetab.h
> +++ b/trunk/auparse/typetab.h
> @@ -32,6 +32,8 @@ _S(AUPARSE_TYPE_UID, "iuid" )
> _S(AUPARSE_TYPE_UID, "id" )
> _S(AUPARSE_TYPE_UID, "inode_uid" )
> _S(AUPARSE_TYPE_UID, "sauid" )
> +_S(AUPARSE_TYPE_UID, "obj_uid" )
> +_S(AUPARSE_TYPE_UID, "obj_gid" )
> _S(AUPARSE_TYPE_GID, "gid" )
> _S(AUPARSE_TYPE_GID, "egid" )
> _S(AUPARSE_TYPE_GID, "sgid" )
> diff --git a/trunk/lib/errormsg.h b/trunk/lib/errormsg.h
> index fd6b1c0..a7bc61d 100644
> --- a/trunk/lib/errormsg.h
> +++ b/trunk/lib/errormsg.h
> @@ -54,5 +54,10 @@ static const struct msg_tab err_msgtab[] = {
> { -19, 0, "Key field needs a watch or syscall given prior to it"
> }, { -20, 2, "-F missing value after operation for" },
> { -21, 2, "-F value should be number for" },
> - { -22, 2, "-F missing field name before operator for" }
> + { -22, 2, "-F missing field name before operator for" },
> + { -23, 2, "-C missng operation for "},
> + { -24, 2, "-C missing field name before operator for" },
> + { -25, 2, "-C missing value after operation for "},
> + { -26, 2, "-C unknown field:" },
> + { -27, 2, "-C unknown right hand value for comparison with:" },
> };
> diff --git a/trunk/lib/fieldtab.h b/trunk/lib/fieldtab.h
> index ad95814..c0432cc 100644
> --- a/trunk/lib/fieldtab.h
> +++ b/trunk/lib/fieldtab.h
> @@ -55,6 +55,8 @@ _S(AUDIT_WATCH, "path" )
> _S(AUDIT_PERM, "perm" )
> _S(AUDIT_DIR, "dir" )
> _S(AUDIT_FILETYPE, "filetype" )
> +_S(AUDIT_OBJ_UID, "obj_uid" )
> +_S(AUDIT_OBJ_GID, "obj_gid" )
>
> _S(AUDIT_ARG0, "a0" )
> _S(AUDIT_ARG1, "a1" )
> @@ -63,3 +65,4 @@ _S(AUDIT_ARG3, "a3" )
>
> _S(AUDIT_FILTERKEY, "key" )
>
> +_S(AUDIT_FIELD_COMPARE, "field_compare" )
> diff --git a/trunk/lib/libaudit.c b/trunk/lib/libaudit.c
> index 9a5070c..b179b5c 100644
> --- a/trunk/lib/libaudit.c
> +++ b/trunk/lib/libaudit.c
> @@ -783,6 +783,307 @@ int audit_rule_syscallbyname_data(struct
> audit_rule_data *rule,
> }
> hidden_def(audit_rule_syscallbyname_data)
>
> +int audit_rule_interfield_fieldpair_data(struct audit_rule_data **rulep,
> + const char *pair,
> + int flags) {
> + const char *f = pair;
> + char *v;
> + int op;
> + int field1, field2;
> + int vlen;
> + int offset;
> + struct audit_rule_data *rule = *rulep;
> +
> + if (f == NULL)
> + return -1;
> +
> + /* look for 2-char operators first
> + then look for 1-char operators afterwards
> + when found, null out the bytes under the operators to split
> + and set value pointer just past operator bytes
> + */
> + if ( (v = strstr(pair, "!=")) ) {
> + *v++ = '\0';
> + *v++ = '\0';
> + op = AUDIT_NOT_EQUAL;
> + } else if ( (v = strstr(pair, "=")) ) {
> + *v++ = '\0';
> + op = AUDIT_EQUAL;
> + } else {
> + return -13;
> + }
> +
> + if (v == NULL)
> + return -23;
> +
> + if (*f == 0)
> + return -24;
> +
> + if (*v == 0)
> + return -25;
> +
> + if ((field1 = audit_name_to_field(f)) < 0)
> + return -26;
> +
> + if ((field2 = audit_name_to_field(v)) < 0)
> + return -27;
> +
> + /* Exclude filter can be used only with MSGTYPE field */
> + if (flags == AUDIT_FILTER_EXCLUDE && field1 != AUDIT_MSGTYPE)
> + return -12;
> +
> + // It should always be AUDIT_FIELD_COMPARE
> + rule->fields[rule->field_count] = AUDIT_FIELD_COMPARE;
> + rule->fieldflags[rule->field_count] = op;
> + /* oh god, so many permutations */
> + switch (field1)
> + {
> + /* UID comparison */
> + case AUDIT_EUID:
> + switch(field2) {
> + case AUDIT_LOGINUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_EUID;
> + break;
> + case AUDIT_FSUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EUID_TO_FSUID;
> + break;
> + case AUDIT_OBJ_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_EUID_TO_OBJ_UID;
> + break;
> + case AUDIT_SUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EUID_TO_SUID;
> + break;
> + case AUDIT_UID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_EUID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_FSUID:
> + switch(field2) {
> + case AUDIT_LOGINUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_FSUID;
> + break;
> + case AUDIT_EUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EUID_TO_FSUID;
> + break;
> + case AUDIT_OBJ_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_FSUID_TO_OBJ_UID;
> + break;
> + case AUDIT_SUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_SUID_TO_FSUID;
> + break;
> + case AUDIT_UID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_FSUID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_LOGINUID:
> + switch(field2) {
> + case AUDIT_EUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_EUID;
> + break;
> + case AUDIT_FSUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_FSUID;
> + break;
> + case AUDIT_OBJ_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_AUID_TO_OBJ_UID;
> + break;
> + case AUDIT_SUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_SUID;
> + break;
> + case AUDIT_UID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_AUID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_SUID:
> + switch(field2) {
> + case AUDIT_LOGINUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_AUID_TO_SUID;
> + break;
> + case AUDIT_EUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EUID_TO_SUID;
> + break;
> + case AUDIT_FSUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_SUID_TO_FSUID;
> + break;
> + case AUDIT_OBJ_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_SUID_TO_OBJ_UID;
> + break;
> + case AUDIT_UID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_SUID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_OBJ_UID:
> + switch(field2) {
> + case AUDIT_LOGINUID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_AUID_TO_OBJ_UID;
> + break;
> + case AUDIT_EUID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_EUID_TO_OBJ_UID;
> + break;
> + case AUDIT_FSUID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_FSUID_TO_OBJ_UID;
> + break;
> + case AUDIT_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_UID_TO_OBJ_UID;
> + break;
> + case AUDIT_SUID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_SUID_TO_OBJ_UID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_UID:
> + switch(field2) {
> + case AUDIT_LOGINUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_AUID;
> + break;
> + case AUDIT_EUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_EUID;
> + break;
> + case AUDIT_FSUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_FSUID;
> + break;
> + case AUDIT_OBJ_UID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_UID_TO_OBJ_UID;
> + break;
> + case AUDIT_SUID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_UID_TO_SUID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> +
> + /* GID comparisons */
> + case AUDIT_EGID:
> + switch(field2) {
> + case AUDIT_FSGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EGID_TO_FSGID;
> + break;
> + case AUDIT_GID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_EGID;
> + break;
> + case AUDIT_OBJ_GID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_EGID_TO_OBJ_GID;
> + break;
> + case AUDIT_SGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EGID_TO_SGID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_FSGID:
> + switch(field2) {
> + case AUDIT_SGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_SGID_TO_FSGID;
> + break;
> + case AUDIT_GID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_FSGID;
> + break;
> + case AUDIT_OBJ_GID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_FSGID_TO_OBJ_GID;
> + break;
> + case AUDIT_EGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EGID_TO_FSGID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_GID:
> + switch(field2) {
> + case AUDIT_EGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_EGID;
> + break;
> + case AUDIT_FSGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_FSGID;
> + break;
> + case AUDIT_OBJ_GID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_GID_TO_OBJ_GID;
> + break;
> + case AUDIT_SGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_SGID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_OBJ_GID:
> + switch(field2) {
> + case AUDIT_EGID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_EGID_TO_OBJ_GID;
> + break;
> + case AUDIT_FSGID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_FSGID_TO_OBJ_GID;
> + break;
> + case AUDIT_GID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_GID_TO_OBJ_GID;
> + break;
> + case AUDIT_SGID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_SGID_TO_OBJ_GID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + case AUDIT_SGID:
> + switch(field2) {
> + case AUDIT_FSGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_SGID_TO_FSGID;
> + break;
> + case AUDIT_GID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_GID_TO_SGID;
> + break;
> + case AUDIT_OBJ_GID:
> + rule->values[rule->field_count] =
AUDIT_COMPARE_SGID_TO_OBJ_GID;
> + break;
> + case AUDIT_EGID:
> + rule->values[rule->field_count] = AUDIT_COMPARE_EGID_TO_SGID;
> + break;
> + default:
> + return -1;
> + }
> + break;
> + /* fallthrough */
> + default:
> + if (field1 == AUDIT_INODE) {
> + if (!(op == AUDIT_NOT_EQUAL ||
> + op == AUDIT_EQUAL))
> + return -13;
> + }
> +
> + if (field1 == AUDIT_PPID && !(flags == AUDIT_FILTER_EXIT
> + || flags == AUDIT_FILTER_ENTRY))
> + return -17;
> +
> + if (!isdigit((char)*(v)))
> + return -21;
> +
> + if (field1 == AUDIT_INODE)
> + rule->values[rule->field_count] =
> + strtoul(v, NULL, 0);
> + else
> + rule->values[rule->field_count] =
> + strtol(v, NULL, 0);
> + break;
> + }
> + rule->field_count++;
> + return 0;
> +}
> +
> int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char
> *pair, int flags)
> {
> @@ -857,6 +1158,8 @@ int audit_rule_fieldpair_data(struct
> audit_rule_data **rulep, const char *pair,
> case AUDIT_SUID:
> case AUDIT_FSUID:
> case AUDIT_LOGINUID:
> + case AUDIT_OBJ_UID:
> + case AUDIT_OBJ_GID:
> // Do positive & negative separate for 32 bit systems
> vlen = strlen(v);
> if (isdigit((char)*(v)))
> diff --git a/trunk/lib/libaudit.h b/trunk/lib/libaudit.h
> index 8feaa39..911bce4 100644
> --- a/trunk/lib/libaudit.h
> +++ b/trunk/lib/libaudit.h
> @@ -428,6 +428,8 @@ extern int audit_rule_syscallbyname_data(struct
> audit_rule_data *rule,
> * adding new fields */
> extern int audit_rule_fieldpair_data(struct audit_rule_data **rulep,
> const char *pair, int flags);
> +extern int audit_rule_interfield_fieldpair_data(struct audit_rule_data
> **rulep, + const char *pair, int flags);
> extern void audit_rule_free_data(struct audit_rule_data *rule);
>
> #ifdef __cplusplus
> diff --git a/trunk/src/auditctl.c b/trunk/src/auditctl.c
> index 34b7935..d7ec998 100644
> --- a/trunk/src/auditctl.c
> +++ b/trunk/src/auditctl.c
> @@ -482,7 +482,7 @@ static int setopt(int count, int lineno, char *vars[])
> keylen = AUDIT_MAX_KEY_LEN;
>
> while ((retval >= 0) && (c = getopt(count, vars,
> - "hislDvte:f:r:b:a:A:d:S:F:m:R:w:W:k:p:q:")) != EOF) {
> + "hislDvtC:e:f:r:b:a:A:d:S:F:m:R:w:W:k:p:q:")) != EOF) {
> int flags = AUDIT_FILTER_UNSET;
> rc = 10; // Init to something impossible to see if unused.
> switch (c) {
> @@ -731,7 +731,6 @@ static int setopt(int count, int lineno, char *vars[])
> retval = -1;
> break;
> }
> -
> rc = audit_rule_fieldpair_data(&rule_new,optarg,flags);
> if (rc != 0) {
> audit_number_to_errmsg(rc, optarg);
> @@ -743,6 +742,22 @@ static int setopt(int count, int lineno, char *vars[])
> }
>
> break;
> + case 'C':
> + if (add != AUDIT_FILTER_UNSET)
> + flags = add & AUDIT_FILTER_MASK;
> + else if (del != AUDIT_FILTER_UNSET)
> + flags = del & AUDIT_FILTER_MASK;
> +
> + rc = audit_rule_interfield_fieldpair_data(&rule_new, optarg, flags);
> + if (rc != 0) {
> + audit_number_to_errmsg(rc, optarg);
> + retval = -1;
> + } else {
> + if (rule_new->fields[rule_new->field_count - 1] ==
> + AUDIT_PERM)
> + audit_permadded = 1;
> + }
> + break;
> case 'm':
> if (count > 3) {
> fprintf(stderr,
> diff --git a/trunk/src/ausearch-report.c b/trunk/src/ausearch-report.c
> index d50c732..62e1ae0 100644
> --- a/trunk/src/ausearch-report.c
> +++ b/trunk/src/ausearch-report.c
> @@ -333,6 +333,7 @@ static struct nv_pair typetab[] = {
> {T_UID, "id"},
> {T_UID, "inode_uid"},
> {T_UID, "sauid"},
> + {T_UID, "obj_uid"},
> {T_GID, "gid"},
> {T_GID, "egid"},
> {T_GID, "sgid"},