Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
On 2018-04-18 20:24, Paul Moore wrote: > On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote: > > Implement container ID filtering using the AUDIT_CONTAINERID field name > > to send an 8-character string representing a u64 since the value field > > is only u32. > > > > Sending it as two u32 was considered, but gathering and comparing two > > fields was more complex. > > My only worry here is that you aren't really sending a string in the > ASCII sense, you are sending an 8 byte buffer (that better be NUL > terminated) that happens to be an unsigned 64-bit integer. To be > clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and > the code is okay with that, I just want us to pause for a minute and > make sure that is an okay thing to do long term. I already went through that process and warned of it 7 weeks ago. As already noted, That was preferable to two seperate u32 fields that depend on each other making comparisons more complicated. Using two seperate fields to configure the rule could be gated for validity, then the result stored in a special rule field, but I wasn't keen about that approach. > > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER. > > > > This requires support from userspace to be useful. > > See: https://github.com/linux-audit/audit-userspace/issues/40 > > Signed-off-by: Richard Guy Briggs > > --- > > include/linux/audit.h | 1 + > > include/uapi/linux/audit.h | 5 - > > kernel/audit.h | 1 + > > kernel/auditfilter.c | 47 > > ++ > > kernel/auditsc.c | 3 +++ > > 5 files changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 3acbe9d..f10ca1b 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -76,6 +76,7 @@ struct audit_field { > > u32 type; > > union { > > u32 val; > > + u64 val64; > > kuid_t uid; > > kgid_t gid; > > struct { > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index e83ccbd..8443a8f 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -262,6 +262,7 @@ > > #define AUDIT_LOGINUID_SET 24 > > #define AUDIT_SESSIONID25 /* Session ID */ > > #define AUDIT_FSTYPE 26 /* FileSystem Type */ > > +#define AUDIT_CONTAINERID 27 /* Container ID */ > > > > /* These are ONLY useful when checking > > * at syscall exit time (AUDIT_AT_EXIT). */ > > @@ -342,6 +343,7 @@ enum { > > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x0010 > > #define AUDIT_FEATURE_BITMAP_LOST_RESET0x0020 > > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x0040 > > +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER0x0080 > > > > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \ > > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \ > > @@ -349,7 +351,8 @@ enum { > > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \ > > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \ > > AUDIT_FEATURE_BITMAP_LOST_RESET | \ > > - AUDIT_FEATURE_BITMAP_FILTER_FS) > > + AUDIT_FEATURE_BITMAP_FILTER_FS | \ > > + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER) > > > > /* deprecated: AUDIT_VERSION_* */ > > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL > > diff --git a/kernel/audit.h b/kernel/audit.h > > index 214e149..aaa651a 100644 > > --- a/kernel/audit.h > > +++ b/kernel/audit.h > > @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino) > > > > extern int audit_match_class(int class, unsigned syscall); > > extern int audit_comparator(const u32 left, const u32 op, const u32 right); > > +extern int audit_comparator64(const u64 left, const u32 op, const u64 > > right); > > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right); > > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right); > > extern int parent_len(const char *path); > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index d7a807e..c4c8746 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, > > struct audit_field *f) > > /* FALL THROUGH */ > > case AUDIT_ARCH: > > case AUDIT_FSTYPE: > > + case AUDIT_CONTAINERID: > > if (f->op != Audit_not_equal && f->op != Audit_equal) > > return -EINVAL; > > brea
Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote: > Implement container ID filtering using the AUDIT_CONTAINERID field name > to send an 8-character string representing a u64 since the value field > is only u32. > > Sending it as two u32 was considered, but gathering and comparing two > fields was more complex. My only worry here is that you aren't really sending a string in the ASCII sense, you are sending an 8 byte buffer (that better be NUL terminated) that happens to be an unsigned 64-bit integer. To be clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and the code is okay with that, I just want us to pause for a minute and make sure that is an okay thing to do long term. > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER. > > This requires support from userspace to be useful. > See: https://github.com/linux-audit/audit-userspace/issues/40 > Signed-off-by: Richard Guy Briggs > --- > include/linux/audit.h | 1 + > include/uapi/linux/audit.h | 5 - > kernel/audit.h | 1 + > kernel/auditfilter.c | 47 > ++ > kernel/auditsc.c | 3 +++ > 5 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 3acbe9d..f10ca1b 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -76,6 +76,7 @@ struct audit_field { > u32 type; > union { > u32 val; > + u64 val64; > kuid_t uid; > kgid_t gid; > struct { > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index e83ccbd..8443a8f 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -262,6 +262,7 @@ > #define AUDIT_LOGINUID_SET 24 > #define AUDIT_SESSIONID25 /* Session ID */ > #define AUDIT_FSTYPE 26 /* FileSystem Type */ > +#define AUDIT_CONTAINERID 27 /* Container ID */ > > /* These are ONLY useful when checking > * at syscall exit time (AUDIT_AT_EXIT). */ > @@ -342,6 +343,7 @@ enum { > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x0010 > #define AUDIT_FEATURE_BITMAP_LOST_RESET0x0020 > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x0040 > +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER0x0080 > > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \ > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \ > @@ -349,7 +351,8 @@ enum { > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \ > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \ > AUDIT_FEATURE_BITMAP_LOST_RESET | \ > - AUDIT_FEATURE_BITMAP_FILTER_FS) > + AUDIT_FEATURE_BITMAP_FILTER_FS | \ > + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER) > > /* deprecated: AUDIT_VERSION_* */ > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL > diff --git a/kernel/audit.h b/kernel/audit.h > index 214e149..aaa651a 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino) > > extern int audit_match_class(int class, unsigned syscall); > extern int audit_comparator(const u32 left, const u32 op, const u32 right); > +extern int audit_comparator64(const u64 left, const u32 op, const u64 right); > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right); > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right); > extern int parent_len(const char *path); > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index d7a807e..c4c8746 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, > struct audit_field *f) > /* FALL THROUGH */ > case AUDIT_ARCH: > case AUDIT_FSTYPE: > + case AUDIT_CONTAINERID: > if (f->op != Audit_not_equal && f->op != Audit_equal) > return -EINVAL; > break; > @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct > audit_rule_data *data, > } > entry->rule.exe = audit_mark; > break; > + case AUDIT_CONTAINERID: > + if (f->val != sizeof(u64)) > + goto exit_free; > + str = audit_unpack_string(&bufp, &remain, f->val); > + if (IS_ERR(str)) > + goto exit_free; > + f->val64 = ((u64 *)str)[0]; > +