Re: [PATCH v3 4/4] ima: Differentiate auditing policy rules from "audit" actions
On Mon, Jun 4, 2018 at 4:54 PM, Stefan Berger wrote: > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > Since we defined a new message type we can now also pass the > audit_context and get an associated SYSCALL record. This now produces > the following records when parsing IMA policy's rules: Aaand now I see you included the current->audit_context pointer I mentioned in my comments for 3/4 ;) So basically this should be fine, although I should point out that you do not need to define a new message type to associate records together. The fact that we don't associate all connected records is basically a bug. Anyway, patches 3/4 and 4/4 look good to me. Considering this is likely going in during the *next* merge window, I would ask that you convert from "current->audit_context" to "audit_context()" as soon as this merge window closes. Thanks! > type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \ > func=MMAP_CHECK mask=MAY_EXEC res=1 > type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \ > func=FILE_CHECK mask=MAY_READ res=1 > type=SYSCALL msg=audit(1527888965.738:320): arch=c03e syscall=1 \ > success=yes exit=17 a0=1 a1=55bcfcca9030 a2=11 a3=7fcc1b55fb38 \ > items=0 ppid=1567 pid=1601 auid=0 uid=0 gid=0 euid=0 suid=0 \ > fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="echo" \ > exe="/usr/bin/echo" \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) > > Signed-off-by: Stefan Berger > --- > include/uapi/linux/audit.h | 1 + > security/integrity/ima/ima_policy.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 65d9293f1fb8..cb358551376b 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -148,6 +148,7 @@ > #define AUDIT_INTEGRITY_PCR1804 /* PCR invalidation msgs */ > #define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > #define AUDIT_INTEGRITY_EVM_XATTR 1806 /* New EVM-covered xattr */ > +#define AUDIT_INTEGRITY_POLICY_RULE 1807 /* IMA policy rules */ > > #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A > REQUEST. */ > > diff --git a/security/integrity/ima/ima_policy.c > b/security/integrity/ima/ima_policy.c > index bc99713dfe57..f7230db217a7 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -652,8 +652,8 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) > bool uid_token; > int result = 0; > > - ab = integrity_audit_log_start(NULL, GFP_KERNEL, > - AUDIT_INTEGRITY_RULE); > + ab = integrity_audit_log_start(current->audit_context, GFP_KERNEL, > + AUDIT_INTEGRITY_POLICY_RULE); > > entry->uid = INVALID_UID; > entry->fowner = INVALID_UID; > -- > 2.13.6 > -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 3/4] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
On Mon, Jun 4, 2018 at 4:54 PM, Stefan Berger
wrote:
> If Integrity is not auditing, IMA shouldn't audit, either.
>
> Signed-off-by: Stefan Berger
> ---
> security/integrity/ima/Kconfig | 1 +
> security/integrity/ima/ima_policy.c | 6 +-
> security/integrity/integrity.h | 15 +++
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 6a8f67714c83..94c2151331aa 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -12,6 +12,7 @@ config IMA
> select TCG_TIS if TCG_TPM && X86
> select TCG_CRB if TCG_TPM && ACPI
> select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
> + select INTEGRITY_AUDIT if AUDIT
> help
> The Trusted Computing Group(TCG) runtime Integrity
> Measurement Architecture(IMA) maintains a list of hash
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 3fcf0935468c..bc99713dfe57 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -628,6 +628,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
> static void ima_log_string_op(struct audit_buffer *ab, char *key, char
> *value,
> bool (*rule_operator)(kuid_t, kuid_t))
> {
> + if (!ab)
> + return;
> +
> if (rule_operator == &uid_gt)
> audit_log_format(ab, "%s>", key);
> else if (rule_operator == &uid_lt)
> @@ -649,7 +652,8 @@ static int ima_parse_rule(char *rule, struct
> ima_rule_entry *entry)
> bool uid_token;
> int result = 0;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
> + ab = integrity_audit_log_start(NULL, GFP_KERNEL,
> + AUDIT_INTEGRITY_RULE);
There was a lot of confusion here, so this is understandable, but you
should pass "audit_context()"[1] as the first parameter instead of
NULL. Other than that this patch looks fine.
[1] In Linus' tree at the moment you would need to use
current->audit_context, but the audit PR heading to Linus during this
merge window will introduce the "audit_context()" function which is
preferable as we may need to change things around a bit in the near
future.
> entry->uid = INVALID_UID;
> entry->fowner = INVALID_UID;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 0bb372eed62a..e60473b13a8d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> /* iint action cache flags */
> #define IMA_MEASURE0x0001
> @@ -199,6 +200,13 @@ static inline void evm_load_x509(void)
> void integrity_audit_msg(int audit_msgno, struct inode *inode,
> const unsigned char *fname, const char *op,
> const char *cause, int result, int info);
> +
> +static inline struct audit_buffer *
> +integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int
> type)
> +{
> + return audit_log_start(ctx, gfp_mask, type);
> +}
> +
> #else
> static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
>const unsigned char *fname,
> @@ -206,4 +214,11 @@ static inline void integrity_audit_msg(int audit_msgno,
> struct inode *inode,
>int result, int info)
> {
> }
> +
> +static inline struct audit_buffer *
> +integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int
> type)
> +{
> + return NULL;
> +}
> +
> #endif
> --
> 2.13.6
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
On Sat, Jun 2, 2018 at 1:53 PM, Richard Guy Briggs wrote: > On 2018-06-01 18:15, Paul Moore wrote: >> On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs wrote: >> > On 2018-05-31 11:48, Paul Moore wrote: >> >> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs >> >> wrote: >> >> > Most uses of audit_enabled don't care about the distinction between >> >> > AUDIT_ON and AUDIT_LOCKED, so using audit_enabled as a boolean makes >> >> > more sense and is easier to read. Most uses of audit_enabled treat it as >> >> > a boolean, so switch the remaining AUDIT_OFF usage to simply use >> >> > audit_enabled as a boolean where applicable. >> >> > >> >> > See: https://github.com/linux-audit/audit-kernel/issues/86 >> >> > >> >> > Signed-off-by: Richard Guy Briggs >> >> > --- >> >> > drivers/tty/tty_audit.c | 2 +- >> >> > include/net/xfrm.h | 2 +- >> >> > kernel/audit.c | 8 >> >> > net/netfilter/xt_AUDIT.c | 2 +- >> >> > net/netlabel/netlabel_user.c | 2 +- >> >> > 5 files changed, 8 insertions(+), 8 deletions(-) >> >> >> >> I'm not sure I like this idea. Yes, technically this change is >> >> functionally equivalent but I worry that this will increase the chance >> >> that non-audit folks will mistake audit_enabled as a true/false value >> >> when it is not. It might work now, but I worry about some subtle >> >> problem in the future. >> > >> > Would you prefer a patch to change the majority (18) of uses of >> > audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or >> > "audit_enabled != AUDIT_OFF")? >> > >> > I prefer the approach in this patch because it makes the code smaller >> > and significantly easier to read, but either way, I'd like all uses to >> > be consistent so that it is easier to read all the code similarly. >> > >> >> If you are bothered by the comparison to 0 (magic numbers), you could >> >> move the AUDIT_OFF/AUDIT_ON/AUDIT_LOCKED definitions into >> >> include/linux/audit.h and convert the "audit_enabled == 0" to >> >> "audit_enabled == AUDIT_OFF". >> > >> > I'd be fine doing that if you really dislike this patch's approach. >> >> Like I said, I'm don't really care for the boolean-like approach of >> this first patch. > > That doesn't really address the original issue though. To be honest, there really isn't an issue to begin with, at least not in my mind. Sure, I understand you think all non-audit users of audit_enabled should treat audit_enabled as a boolean; at this point in time, I don't think that is necessary or desirable. > Without any elaboration, I am not able to guess why you don't like this > or what possible future subtleties would cause a problem. As I said previously: "I worry that this will increase the chance that non-audit folks will mistake audit_enabled as a true/false value when it is not. It might work now, but I worry about some subtle problem in the future.". > Can you explain the problem with "non-audit folks will mistake > audit_enabled as a true/false value when it is not"? See the "it might work but ..." part above. > While I realize people change their opinions given a broader context, > and the origninal reply was ambiguous, I went ahead with this patch > based on your "Sounds good." from: > https://www.redhat.com/archives/linux-audit/2018-April/msg00089.html I think the confusion comes from what was meant by "clean them all up". We obviously have different understandings of what "cleaning" meant. > Would you accept a patch that defines a function by the same name as the > global variable that returns a boolean (and localizes and renames the > existing global with a "__" prefix? At this point I think I've been clear that I don't like treating it as a boolean, regardless of if it is wrapped in a function or not. Why? Well, it's not a boolean for starters. If you wanted to submit a patch that would swap out 0 for AUDIT_OFF I would accept that. > I'm not willing to offer a patch to make the existing boolean usage harder to > read to bring it all into similar usage. Okay ... ? Patch submission has always been voluntary as far as I can tell; if you aren't willing to submit a patch, that's fine. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: Auditd syslog plugin
If you're on a system using rsyslog, you can also leverage imfile and send it directly to a remote logserver. rsyslog event queuing also handles interruptions in remote logging more gracefully than audispd syslog. On 06/04/2018 06:11 PM, Steve Grubb wrote: > On Monday, June 4, 2018 9:02:04 AM EDT Boyce, Kevin P [US] (AS) wrote: >> All, >> >> After enabling the syslog plugin for audispd and sending logs to a remote >> server I am seeing every event being written to /var/log/messages locally >> which is filling up /var. >> >> This is all redundant since local audit logs are kept in /var/log/audit. >> Is there a way to prevent auditd syslog plugin from writing to >> /var/log/messages? > That is pretty much what the plugin does. It writes all events to syslog > which based on rules in /etc/rsyslog.conf decides what to do with the text. > Typically it is to write everything to /var/log/messages. > > However, you can assign a specific facility to the audit events in the /etc/ > audisp/plugins.d/syslog.conf file and then in rsyslog.conf exclude the > facility by putting .none on the /var/log/messages line. > > -Steve > > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak89 V2] audit: rename FILTER_TYPE to FILTER_EXCLUDE
On Fri, Jun 1, 2018 at 4:45 PM, Richard Guy Briggs wrote:
> The AUDIT_FILTER_TYPE name is vague and misleading due to not describing
> where or when the filter is applied and obsolete due to its available
> filter fields having been expanded.
>
> Userspace has already renamed it from AUDIT_FILTER_TYPE to
> AUDIT_FILTER_EXCLUDE without checking if it already exists. The
> userspace maintainer assures that as long as it is set to the same value
> it will not be a problem since the userspace code does not treat
> compiler warnings as errors. If this policy changes then checks if it
> already exists can be added at the same time.
>
> See: https://github.com/linux-audit/audit-kernel/issues/89
>
> Signed-off-by: Richard Guy Briggs
> ---
> v2:
> - Change from AUDIT_FILTER_EXCL to AUDIT_FILTER_EXCLUDE
> ---
> include/uapi/linux/audit.h | 3 ++-
> kernel/audit.c | 2 +-
> kernel/auditfilter.c | 10 +-
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 04f9bd2..2678422 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -156,8 +156,9 @@
> #define AUDIT_FILTER_ENTRY 0x02/* Apply rule at syscall entry */
> #define AUDIT_FILTER_WATCH 0x03/* Apply rule to file system watches
> */
> #define AUDIT_FILTER_EXIT 0x04/* Apply rule at syscall exit */
> -#define AUDIT_FILTER_TYPE 0x05/* Apply rule at audit_log_start */
> +#define AUDIT_FILTER_EXCLUDE 0x05/* Apply rule at audit_log_start */
I realize you just inherited the comment, but since we're changing the
macro, perhaps a better comment would be appropriate?
> #define AUDIT_FILTER_FS0x06/* Apply rule at
> __audit_inode_child */
> +#define AUDIT_FILTER_TYPE AUDIT_FILTER_EXCLUDE /* obsolete misleading
> naming */
Let's move this up to just under the AUDIT_FILTER_EXCLUDE definition
so we keep the two together.
> #define AUDIT_NR_FILTERS 7
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3a18e59..513a10e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1754,7 +1754,7 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask,
> if (audit_initialized != AUDIT_INITIALIZED)
> return NULL;
>
> - if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> + if (unlikely(!audit_filter(type, AUDIT_FILTER_EXCLUDE)))
> return NULL;
>
> /* NOTE: don't ever fail/sleep on these two conditions:
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index eaa3201..261843d 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -264,7 +264,7 @@ static inline struct audit_entry
> *audit_to_entry_common(struct audit_rule_data *
> case AUDIT_FILTER_TASK:
> #endif
> case AUDIT_FILTER_USER:
> - case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_EXCLUDE:
> case AUDIT_FILTER_FS:
> ;
> }
> @@ -337,7 +337,7 @@ static int audit_field_valid(struct audit_entry *entry,
> struct audit_field *f)
> {
> switch(f->type) {
> case AUDIT_MSGTYPE:
> - if (entry->rule.listnr != AUDIT_FILTER_TYPE &&
> + if (entry->rule.listnr != AUDIT_FILTER_EXCLUDE &&
> entry->rule.listnr != AUDIT_FILTER_USER)
> return -EINVAL;
> break;
> @@ -931,7 +931,7 @@ static inline int audit_add_rule(struct audit_entry
> *entry)
> /* If any of these, don't count towards total */
> switch(entry->rule.listnr) {
> case AUDIT_FILTER_USER:
> - case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_EXCLUDE:
> case AUDIT_FILTER_FS:
> dont_count = 1;
> }
> @@ -1013,7 +1013,7 @@ int audit_del_rule(struct audit_entry *entry)
> /* If any of these, don't count towards total */
> switch(entry->rule.listnr) {
> case AUDIT_FILTER_USER:
> - case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_EXCLUDE:
> case AUDIT_FILTER_FS:
> dont_count = 1;
> }
> @@ -1369,7 +1369,7 @@ int audit_filter(int msgtype, unsigned int listtype)
> break;
> }
> if (result > 0) {
> - if (e->rule.action == AUDIT_NEVER || listtype ==
> AUDIT_FILTER_TYPE)
> + if (e->rule.action == AUDIT_NEVER || listtype ==
> AUDIT_FILTER_EXCLUDE)
> ret = 0;
> break;
> }
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak88 V1] audit: tie ANOM_ABEND records to syscall
On Thu, May 31, 2018 at 4:28 PM, Richard Guy Briggs wrote: > Since core dump events are triggered by user activity, tie the > ANOM_ABEND record to the syscall record to collect all records from the > same event. > > See: https://github.com/linux-audit/audit-kernel/issues/88 > > Signed-off-by: Richard Guy Briggs > --- > kernel/auditsc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Another for after the merge window. > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index fefb9e2..5f0bd5e 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2461,7 +2461,7 @@ void audit_core_dumps(long signr) > if (signr == SIGQUIT) /* don't care for those */ > return; > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND); > + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_ANOM_ABEND); > if (unlikely(!ab)) > return; > audit_log_task(ab); > -- > 1.8.3.1 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak87 V1] audit: tie SECCOMP records to syscall
On Thu, May 31, 2018 at 4:27 PM, Richard Guy Briggs wrote:
> Since seccomp events are triggered by user activity, tie the SECCOMP
> record to the syscall record to collect all records from the same event.
>
> See: https://github.com/linux-audit/audit-kernel/issues/87
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/auditsc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks good to me, queued up for after the merge window.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c45..fefb9e2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2485,7 +2485,7 @@ void audit_seccomp(unsigned long syscall, long signr,
> int code)
> {
> struct audit_buffer *ab;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
On Fri, Jun 1, 2018 at 4:05 PM, Richard Guy Briggs wrote: > On 2018-06-01 10:12, Ondrej Mosnacek wrote: ... >> audit_receive_msg -- this function doesn't work with context at all, >> so I wasn't sure if audit_filter should consider it being NULL or if >> it should get it from the current task. My hunch is the second option >> is the right one, but the first one is safer (AUDIT_DIR will simply >> never be checked here). I don't have such insight into the logic of >> audit_context's lifetime, so I need someone to tell me what makes more >> sense here. Given the nature of audit_receive_msg(), would it ever match on an AUDIT_DIR field? I don't think it would since there aren't really any vfs accesses that occur in the source of sending a netlink message down to the kernel ... am I missing something? > That is starting to work with context. The recent FEATURE_CHANGE patch > to connect records of the same event uses current->audit_context (now > audit_context()) from audit_log_feature_change() called from > audit_set_feature() called from audit_receive_msg(). > > There is also a work in progress to convert all the CONFIG_CHANGE > records over. I'm just trying to track down all the instances. This will be a nice improvement. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
On Fri, Jun 1, 2018 at 4:12 AM, Ondrej Mosnacek wrote: > I'm actually playing with the idea of unifying the filtering logic in > these two functions, where sharing this function wouldn't be > necessary. However, that is quite a big change (a lot of LOC being > moved around) so I'd prefer the simple & dirty approach now and keep > the cleanup for a later patch. [Resend as I forgot the reply-all - oops] I've had similar thoughts in the past, and deferred the work for exactly the same reason. If I recall correctly, I believe part of the reason may stem from the fact that some fields are simply not always valid when the filter is run and this may have been a crude effort at optimization (smaller function size). Regardless, we might preserve some of this idea by creating some helper functions (two?) with the different filter fields in each (no overlap) and a couple of wrapper functions which call the appropriate helpers ... or that just might be extra work for no noticeable perf advantage :) -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: Auditd syslog plugin
On Monday, June 4, 2018 9:02:04 AM EDT Boyce, Kevin P [US] (AS) wrote: > All, > > After enabling the syslog plugin for audispd and sending logs to a remote > server I am seeing every event being written to /var/log/messages locally > which is filling up /var. > > This is all redundant since local audit logs are kept in /var/log/audit. > Is there a way to prevent auditd syslog plugin from writing to > /var/log/messages? That is pretty much what the plugin does. It writes all events to syslog which based on rules in /etc/rsyslog.conf decides what to do with the text. Typically it is to write everything to /var/log/messages. However, you can assign a specific facility to the audit events in the /etc/ audisp/plugins.d/syslog.conf file and then in rsyslog.conf exclude the facility by putting .none on the /var/log/messages line. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 4/4] ima: Differentiate auditing policy rules from "audit" actions
The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. Since we defined a new message type we can now also pass the audit_context and get an associated SYSCALL record. This now produces the following records when parsing IMA policy's rules: type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \ func=MMAP_CHECK mask=MAY_EXEC res=1 type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \ func=FILE_CHECK mask=MAY_READ res=1 type=SYSCALL msg=audit(1527888965.738:320): arch=c03e syscall=1 \ success=yes exit=17 a0=1 a1=55bcfcca9030 a2=11 a3=7fcc1b55fb38 \ items=0 ppid=1567 pid=1601 auid=0 uid=0 gid=0 euid=0 suid=0 \ fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="echo" \ exe="/usr/bin/echo" \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 1 + security/integrity/ima/ima_policy.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 65d9293f1fb8..cb358551376b 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -148,6 +148,7 @@ #define AUDIT_INTEGRITY_PCR1804 /* PCR invalidation msgs */ #define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ #define AUDIT_INTEGRITY_EVM_XATTR 1806 /* New EVM-covered xattr */ +#define AUDIT_INTEGRITY_POLICY_RULE 1807 /* IMA policy rules */ #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index bc99713dfe57..f7230db217a7 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -652,8 +652,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) bool uid_token; int result = 0; - ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + ab = integrity_audit_log_start(current->audit_context, GFP_KERNEL, + AUDIT_INTEGRITY_POLICY_RULE); entry->uid = INVALID_UID; entry->fowner = INVALID_UID; -- 2.13.6 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 2/4] ima: Use audit_log_format() rather than audit_log_string()
Remove the usage of audit_log_string() and replace it with
audit_log_format().
Signed-off-by: Stefan Berger
Suggested-by: Steve Grubb
Reviewed-by: Mimi Zohar
Acked-by: Paul Moore
---
security/integrity/ima/ima_policy.c | 3 +--
security/integrity/integrity_audit.c | 6 +-
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 1d00db19d167..3fcf0935468c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -634,8 +634,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char
*key, char *value,
audit_log_format(ab, "%s<", key);
else
audit_log_format(ab, "%s=", key);
- audit_log_string(ab, value);
- audit_log_format(ab, " ");
+ audit_log_format(ab, "%s ", value);
}
static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
{
diff --git a/security/integrity/integrity_audit.c
b/security/integrity/integrity_audit.c
index 90987d15b6fe..db30763d5525 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -45,11 +45,7 @@ void integrity_audit_msg(int audit_msgno, struct inode
*inode,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
audit_get_sessionid(current));
audit_log_task_context(ab);
- audit_log_format(ab, " op=");
- audit_log_string(ab, op);
- audit_log_format(ab, " cause=");
- audit_log_string(ab, cause);
- audit_log_format(ab, " comm=");
+ audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
audit_log_untrustedstring(ab, get_task_comm(name, current));
if (fname) {
audit_log_format(ab, " name=");
--
2.13.6
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 0/4] IMA: work on audit records produced by IMA
This series of patches cleans up some usages of the audit subsystem's API by IMA. We also introduce a new record type that IMA creates while parsing policy rules. Stefan v2->v3: - reworked patch 4; pass current->audit_context rather than NULL v1->v2: - dropped several patches that extended existing messages with missing fields - Using audit_log_task_info() for new record type in last patch - rebased on security-next; new message type is now 1807 Stefan Berger (4): ima: Call audit_log_string() rather than logging it untrusted ima: Use audit_log_format() rather than audit_log_string() ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set ima: Differentiate auditing policy rules from "audit" actions include/uapi/linux/audit.h | 1 + security/integrity/ima/Kconfig | 1 + security/integrity/ima/ima_policy.c | 9 ++--- security/integrity/integrity.h | 15 +++ security/integrity/integrity_audit.c | 6 +- 5 files changed, 24 insertions(+), 8 deletions(-) -- 2.13.6 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 3/4] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
If Integrity is not auditing, IMA shouldn't audit, either.
Signed-off-by: Stefan Berger
---
security/integrity/ima/Kconfig | 1 +
security/integrity/ima/ima_policy.c | 6 +-
security/integrity/integrity.h | 15 +++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..94c2151331aa 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -12,6 +12,7 @@ config IMA
select TCG_TIS if TCG_TPM && X86
select TCG_CRB if TCG_TPM && ACPI
select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
+ select INTEGRITY_AUDIT if AUDIT
help
The Trusted Computing Group(TCG) runtime Integrity
Measurement Architecture(IMA) maintains a list of hash
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 3fcf0935468c..bc99713dfe57 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -628,6 +628,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
bool (*rule_operator)(kuid_t, kuid_t))
{
+ if (!ab)
+ return;
+
if (rule_operator == &uid_gt)
audit_log_format(ab, "%s>", key);
else if (rule_operator == &uid_lt)
@@ -649,7 +652,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry
*entry)
bool uid_token;
int result = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+ ab = integrity_audit_log_start(NULL, GFP_KERNEL,
+ AUDIT_INTEGRITY_RULE);
entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0bb372eed62a..e60473b13a8d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
/* iint action cache flags */
#define IMA_MEASURE0x0001
@@ -199,6 +200,13 @@ static inline void evm_load_x509(void)
void integrity_audit_msg(int audit_msgno, struct inode *inode,
const unsigned char *fname, const char *op,
const char *cause, int result, int info);
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+ return audit_log_start(ctx, gfp_mask, type);
+}
+
#else
static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
const unsigned char *fname,
@@ -206,4 +214,11 @@ static inline void integrity_audit_msg(int audit_msgno,
struct inode *inode,
int result, int info)
{
}
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+ return NULL;
+}
+
#endif
--
2.13.6
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 1/4] ima: Call audit_log_string() rather than logging it untrusted
The parameters passed to this logging function are all provided by a privileged user and therefore we can call audit_log_string() rather than audit_log_untrustedstring(). Signed-off-by: Stefan Berger Suggested-by: Steve Grubb Acked-by: Paul Moore --- security/integrity/ima/ima_policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8bbc18eb07eb..1d00db19d167 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value, audit_log_format(ab, "%s<", key); else audit_log_format(ab, "%s=", key); - audit_log_untrustedstring(ab, value); + audit_log_string(ab, value); audit_log_format(ab, " "); } static void ima_log_string(struct audit_buffer *ab, char *key, char *value) -- 2.13.6 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
On 2018-06-04 13:32, Ondrej Mosnacek wrote:
> 2018-06-01 22:05 GMT+02:00 Richard Guy Briggs :
> > On 2018-06-01 10:12, Ondrej Mosnacek wrote:
> >> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs :
> >> > On 2018-05-30 10:45, Ondrej Mosnacek wrote:
> >> >> This patch allows the AUDIR_DIR field to be used also with the exclude
> >> >> filter.
> >> >>
> >> >> Not-yet-signed-off-by: Ondrej Mosnacek
> >> >> ---
> >> >> kernel/audit.c | 5 +++--
> >> >> kernel/audit.h | 32 +++-
> >> >> kernel/audit_tree.c | 4 +++-
> >> >> kernel/auditfilter.c | 6 +-
> >> >> kernel/auditsc.c | 28
> >> >> 5 files changed, 42 insertions(+), 33 deletions(-)
> >> >>
> >> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> >> index e7478cb58079..aac5b6ecc11d 100644
> >> >> --- a/kernel/audit.c
> >> >> +++ b/kernel/audit.c
> >> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb,
> >> >> struct nlmsghdr *nlh)
> >> >> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> >> >> return 0;
> >> >>
> >> >> - err = audit_filter(msg_type, AUDIT_FILTER_USER);
> >> >> + // FIXME: do we need to pass the context here?
> >> >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
> >> >> if (err == 1) { /* match or error */
> >> >> err = 0;
> >> >> if (msg_type == AUDIT_USER_TTY) {
> >> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct
> >> >> audit_context *ctx, gfp_t gfp_mask,
> >> >> if (audit_initialized != AUDIT_INITIALIZED)
> >> >> return NULL;
> >> >>
> >> >> - if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> >> >> + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
> >> >> return NULL;
> >> >>
> >> >> /* NOTE: don't ever fail/sleep on these two conditions:
> >> >> diff --git a/kernel/audit.h b/kernel/audit.h
> >> >> index 214e14948370..43cfeba25802 100644
> >> >> --- a/kernel/audit.h
> >> >> +++ b/kernel/audit.h
> >> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head
> >> >> *list);
> >> >> #define audit_kill_trees(list) BUG()
> >> >> #endif
> >> >>
> >> >> +struct audit_tree_refs {
> >> >> + struct audit_tree_refs *next;
> >> >> + struct audit_chunk *c[31];
> >> >> +};
> >> >> +
> >> >> +/* A utility function to match tree refs: */
> >> >> +static inline int match_tree_refs(struct audit_context *ctx, struct
> >> >> audit_tree *tree)
> >> >> +{
> >> >> +#ifdef CONFIG_AUDIT_TREE
> >> >> + struct audit_tree_refs *p;
> >> >> + int n;
> >> >> + if (!tree)
> >> >> + return 0;
> >> >> + /* full ones */
> >> >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> >> >> + for (n = 0; n < 31; n++)
> >> >> + if (audit_tree_match(p->c[n], tree))
> >> >> + return 1;
> >> >> + }
> >> >> + /* partial */
> >> >> + if (p) {
> >> >> + for (n = ctx->tree_count; n < 31; n++)
> >> >> + if (audit_tree_match(p->c[n], tree))
> >> >> + return 1;
> >> >> + }
> >> >> +#endif
> >> >> + return 0;
> >> >> +}
> >> >> +
> >> >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t
> >> >> len);
> >> >>
> >> >> extern pid_t audit_sig_pid;
> >> >> extern kuid_t audit_sig_uid;
> >> >> extern u32 audit_sig_sid;
> >> >>
> >> >> -extern int audit_filter(int msgtype, unsigned int listtype);
> >> >> +extern int audit_filter(int msgtype, unsigned int listtype,
> >> >> + struct audit_context *ctx);
> >> >>
> >> >> #ifdef CONFIG_AUDITSYSCALL
> >> >> extern int audit_signal_info(int sig, struct task_struct *t);
> >> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> >> >> index 67e6956c0b61..d4d36389c3d7 100644
> >> >> --- a/kernel/audit_tree.c
> >> >> +++ b/kernel/audit_tree.c
> >> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
> >> >>
> >> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
> >> >> {
> >> >> + if (krule->listnr != AUDIT_FILTER_EXIT &&
> >> >> + krule->listnr != AUDIT_FILTER_TYPE)
> >> >> + return -EINVAL;
> >> >>
> >> >> if (pathname[0] != '/' ||
> >> >> - rule->listnr != AUDIT_FILTER_EXIT ||
> >> >> op != Audit_equal ||
> >> >> rule->inode_f || rule->watch || rule->tree)
> >> >> return -EINVAL;
> >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> >> index 6db9847ca031..e1d70cb77ea3 100644
> >> >> --- a/kernel/auditfilter.c
> >> >> +++ b/kernel/auditfilter.c
> >> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname,
> >> >> const char *path, int parentlen)
> >> >> return strncmp(p, dname, dlen);
> >> >> }
> >> >>
> >> >> -int audit_
Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE
On Wed, May 30, 2018 at 4:45 AM, Ondrej Mosnacek wrote: > This patch removes the restriction of the AUDIT_EXE field to only > SYSCALL filter and teaches audit_filter to recognize this field. > > This makes it possible to write rule lists such as: > > auditctl -a exit,always [some general rule] > # Filter out events with executable name /bin/exe1 or /bin/exe2: > auditctl -a exclude,always -F exe=/bin/exe1 > auditctl -a exclude,always -F exe=/bin/exe2 > > See: https://github.com/linux-audit/audit-kernel/issues/54 > > Signed-off-by: Ondrej Mosnacek > --- > kernel/auditfilter.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) Thanks for your patience Ondrej. Having reflected a bit on things from the recent IMA audit discussion, my current thinking is to go ahead and merge this patch into audit/next once the merge window closes. > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index eaa320148d97..6db9847ca031 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, > struct audit_field *f) > case AUDIT_EXE: > if (f->op != Audit_not_equal && f->op != Audit_equal) > return -EINVAL; > - if (entry->rule.listnr != AUDIT_FILTER_EXIT) > - return -EINVAL; > break; > } > return 0; > @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype) > f->type, f->op, > f->lsm_rule, NULL); > } > break; > + case AUDIT_EXE: > + result = audit_exe_compare(current, > e->rule.exe); > + if (f->op == Audit_not_equal) > + result = !result; > + break; > default: > goto unlock_and_return; > } > -- > 2.17.0 -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On 2018-06-04 16:23, Richard Guy Briggs wrote:
> On 2018-06-04 12:09, Steve Grubb wrote:
> > On Friday, June 1, 2018 5:04:55 PM EDT Richard Guy Briggs wrote:
> > > Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
> > >
> > > From: Richard Guy Briggs
> > > To: Me
> > > CC: linux-...@vger.kernel.org,
> > > contain...@lists.linux-foundation.org, LKML
> > > , epa...@parisplace.org, ... Date: 6/1/18
> > > 5:04 PM
> > >
> > > On 2018-05-17 17:00, Steve Grubb wrote:
> > > > On Fri, 16 Mar 2018 05:00:28 -0400
> > > >
> > > > Richard Guy Briggs wrote:
> > > > > Implement the proc fs write to set the audit container ID of a
> > > > > process, emitting an AUDIT_CONTAINER record to document the event.
> > > > >
> > > > > This is a write from the container orchestrator task to a proc entry
> > > > > of the form /proc/PID/containerid where PID is the process ID of the
> > > > > newly created task that is to become the first task in a container,
> > > > > or an additional task added to a container.
> > > > >
> > > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > > >
> > > > > This will produce a record such as this:
> > > > > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > > > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > > > > res=0
> > > >
> > > > The was one thing I was wondering about. Currently when we set the
> > > > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> > > > set the container id, the event should be AUDIT_CONTAINERID or
> > > > AUDIT_CONTAINER_ID.
> > > >
> > > > During syscall events, the path info is returned in a a record simply
> > > > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> > > > calling the record that gets attached to everything
> > > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > > >
> > > > > The "op" field indicates an initial set. The "pid" to "ses" fields
> > > > > are the orchestrator while the "opid" field is the object's PID, the
> > > > > process being "contained". Old and new container ID values are given
> > > > > in the "contid" fields, while res indicates its success.
> > > > >
> > > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > > child inherits its parent's container ID, but then can be set only
> > > > > once after.
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs
> > > > > ---
> > > > > fs/proc/base.c | 37
> > > > > include/linux/audit.h | 16 +
> > > > > include/linux/init_task.h | 4 ++-
> > > > > include/linux/sched.h | 1 +
> > > > > include/uapi/linux/audit.h | 2 ++
> > > > > kernel/auditsc.c | 84
> > > > > ++ 6 files changed, 143
> > > > > insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > > index 60316b5..6ce4fbe 100644
> > > > > --- a/fs/proc/base.c
> > > > > +++ b/fs/proc/base.c
> > > > > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > > > > * file, char __user * buf, .read= proc_sessionid_read,
> > > > > .llseek = generic_file_llseek,
> > > > > };
> > > > > +
> > > > > +static ssize_t proc_containerid_write(struct file *file, const char
> > > > > __user *buf,
> > > > > + size_t count, loff_t *ppos)
> > > > > +{
> > > > > + struct inode *inode = file_inode(file);
> > > > > + u64 containerid;
> > > > > + int rv;
> > > > > + struct task_struct *task = get_proc_task(inode);
> > > > > +
> > > > > + if (!task)
> > > > > + return -ESRCH;
> > > > > + if (*ppos != 0) {
> > > > > + /* No partial writes. */
> > > > > + put_task_struct(task);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > > > > + if (rv < 0) {
> > > > > + put_task_struct(task);
> > > > > + return rv;
> > > > > + }
> > > > > +
> > > > > + rv = audit_set_containerid(task, containerid);
> > > > > + put_task_struct(task);
> > > > > + if (rv < 0)
> > > > > + return rv;
> > > > > + return count;
> > > > > +}
> > > > > +
> > > > > +static const struct file_operations proc_containerid_operations = {
> > > > > + .write = proc_containerid_write,
> > > > > + .llseek = generic_file_llseek,
> > > > > +};
> > > > > +
> > > > > #endif
> > > > >
> > > > > #ifdef CONFIG_FAULT_INJECTION
> > > > > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > > > > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCAL
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On 2018-06-04 12:09, Steve Grubb wrote:
> On Friday, June 1, 2018 5:04:55 PM EDT Richard Guy Briggs wrote:
> > Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
> >
> > From: Richard Guy Briggs
> > To: Me
> > CC: linux-...@vger.kernel.org, contain...@lists.linux-foundation.org, LKML
> > , epa...@parisplace.org, ... Date:6/1/18
> > 5:04 PM
> >
> > On 2018-05-17 17:00, Steve Grubb wrote:
> > > On Fri, 16 Mar 2018 05:00:28 -0400
> > >
> > > Richard Guy Briggs wrote:
> > > > Implement the proc fs write to set the audit container ID of a
> > > > process, emitting an AUDIT_CONTAINER record to document the event.
> > > >
> > > > This is a write from the container orchestrator task to a proc entry
> > > > of the form /proc/PID/containerid where PID is the process ID of the
> > > > newly created task that is to become the first task in a container,
> > > > or an additional task added to a container.
> > > >
> > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > >
> > > > This will produce a record such as this:
> > > > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > > > res=0
> > >
> > > The was one thing I was wondering about. Currently when we set the
> > > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> > > set the container id, the event should be AUDIT_CONTAINERID or
> > > AUDIT_CONTAINER_ID.
> > >
> > > During syscall events, the path info is returned in a a record simply
> > > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> > > calling the record that gets attached to everything
> > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > >
> > > > The "op" field indicates an initial set. The "pid" to "ses" fields
> > > > are the orchestrator while the "opid" field is the object's PID, the
> > > > process being "contained". Old and new container ID values are given
> > > > in the "contid" fields, while res indicates its success.
> > > >
> > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > child inherits its parent's container ID, but then can be set only
> > > > once after.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > >
> > > > Signed-off-by: Richard Guy Briggs
> > > > ---
> > > > fs/proc/base.c | 37
> > > > include/linux/audit.h | 16 +
> > > > include/linux/init_task.h | 4 ++-
> > > > include/linux/sched.h | 1 +
> > > > include/uapi/linux/audit.h | 2 ++
> > > > kernel/auditsc.c | 84
> > > > ++ 6 files changed, 143
> > > > insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 60316b5..6ce4fbe 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > > > * file, char __user * buf, .read= proc_sessionid_read,
> > > > .llseek = generic_file_llseek,
> > > > };
> > > > +
> > > > +static ssize_t proc_containerid_write(struct file *file, const char
> > > > __user *buf,
> > > > + size_t count, loff_t *ppos)
> > > > +{
> > > > + struct inode *inode = file_inode(file);
> > > > + u64 containerid;
> > > > + int rv;
> > > > + struct task_struct *task = get_proc_task(inode);
> > > > +
> > > > + if (!task)
> > > > + return -ESRCH;
> > > > + if (*ppos != 0) {
> > > > + /* No partial writes. */
> > > > + put_task_struct(task);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > > > + if (rv < 0) {
> > > > + put_task_struct(task);
> > > > + return rv;
> > > > + }
> > > > +
> > > > + rv = audit_set_containerid(task, containerid);
> > > > + put_task_struct(task);
> > > > + if (rv < 0)
> > > > + return rv;
> > > > + return count;
> > > > +}
> > > > +
> > > > +static const struct file_operations proc_containerid_operations = {
> > > > + .write = proc_containerid_write,
> > > > + .llseek = generic_file_llseek,
> > > > +};
> > > > +
> > > > #endif
> > > >
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > > > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > > #endif
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > REG("make-it-f
Re: [PATCH ghak82 v2] audit: Fix extended comparison of GID/EGID
On Thu, May 31, 2018 at 3:19 AM, Ondrej Mosnacek wrote:
> The audit_filter_rules() function in auditsc.c used the in_[e]group_p()
> functions to check GID/EGID match, but these functions use the current
> task's credentials, while the comparison should use the credentials of
> the task given to audit_filter_rules() as a parameter (tsk).
>
> Note that we can use group_search(cred->group_info, ...) as a
> replacement for both in_group_p and in_egroup_p as these functions only
> compare the parameter to cred->fsgid/egid and then call group_search.
>
> In fact, the usage of in_group_p was even more incorrect: it compares to
> cred->fsgid (which is usually equal to cred->egid) and not cred->gid.
>
> GitHub issue:
> https://github.com/linux-audit/audit-kernel/issues/82
>
> Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")
> Signed-off-by: Ondrej Mosnacek
> ---
> kernel/auditsc.c | 26 --
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c4596c51..3a324ca2fd20 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -492,23 +492,21 @@ static int audit_filter_rules(struct task_struct *tsk,
> break;
> case AUDIT_GID:
> result = audit_gid_comparator(cred->gid, f->op,
> f->gid);
> - if (f->op == Audit_equal) {
> - if (!result)
> - result = in_group_p(f->gid);
> - } else if (f->op == Audit_not_equal) {
> - if (result)
> - result = !in_group_p(f->gid);
> - }
> + if (f->op == Audit_equal)
> + result = result ||
> + groups_search(cred->group_info,
> f->gid);
Okay, so I get you're being clever, and everybody likes to be clever,
but to my eyes this is not an improvement in readability. From my
perspective the following pattern:
if (x)
x = y;
... is so much easier to read than the following pattern:
x = x || y;
Since we've got time during the merge window, please restore the
original if-conditional style. The same comment applies to the other
instances as well.
Other than that, everything else looks okay. I worry we might
surprise some users with this, but the current behavior is not
correct.
> + else if (f->op == Audit_not_equal)
> + result = result &&
> + !groups_search(cred->group_info,
> f->gid);
> break;
> case AUDIT_EGID:
> result = audit_gid_comparator(cred->egid, f->op,
> f->gid);
> - if (f->op == Audit_equal) {
> - if (!result)
> - result = in_egroup_p(f->gid);
> - } else if (f->op == Audit_not_equal) {
> - if (result)
> - result = !in_egroup_p(f->gid);
> - }
> + if (f->op == Audit_equal)
> + result = result ||
> + groups_search(cred->group_info,
> f->gid);
> + else if (f->op == Audit_not_equal)
> + result = result &&
> + !groups_search(cred->group_info,
> f->gid);
> break;
> case AUDIT_SGID:
> result = audit_gid_comparator(cred->sgid, f->op,
> f->gid);
> --
> 2.17.0
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Friday, June 1, 2018 5:04:55 PM EDT Richard Guy Briggs wrote:
> Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
>
> From: Richard Guy Briggs
> To: Me
> CC: linux-...@vger.kernel.org, contain...@lists.linux-foundation.org, LKML
> , epa...@parisplace.org, ... Date: 6/1/18
> 5:04 PM
>
> On 2018-05-17 17:00, Steve Grubb wrote:
> > On Fri, 16 Mar 2018 05:00:28 -0400
> >
> > Richard Guy Briggs wrote:
> > > Implement the proc fs write to set the audit container ID of a
> > > process, emitting an AUDIT_CONTAINER record to document the event.
> > >
> > > This is a write from the container orchestrator task to a proc entry
> > > of the form /proc/PID/containerid where PID is the process ID of the
> > > newly created task that is to become the first task in a container,
> > > or an additional task added to a container.
> > >
> > > The write expects up to a u64 value (unset: 18446744073709551615).
> > >
> > > This will produce a record such as this:
> > > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > > res=0
> >
> > The was one thing I was wondering about. Currently when we set the
> > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> > set the container id, the event should be AUDIT_CONTAINERID or
> > AUDIT_CONTAINER_ID.
> >
> > During syscall events, the path info is returned in a a record simply
> > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> > calling the record that gets attached to everything
> > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> >
> > > The "op" field indicates an initial set. The "pid" to "ses" fields
> > > are the orchestrator while the "opid" field is the object's PID, the
> > > process being "contained". Old and new container ID values are given
> > > in the "contid" fields, while res indicates its success.
> > >
> > > It is not permitted to self-set, unset or re-set the container ID. A
> > > child inherits its parent's container ID, but then can be set only
> > > once after.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > fs/proc/base.c | 37
> > > include/linux/audit.h | 16 +
> > > include/linux/init_task.h | 4 ++-
> > > include/linux/sched.h | 1 +
> > > include/uapi/linux/audit.h | 2 ++
> > > kernel/auditsc.c | 84
> > > ++ 6 files changed, 143
> > > insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 60316b5..6ce4fbe 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > > * file, char __user * buf, .read= proc_sessionid_read,
> > > .llseek = generic_file_llseek,
> > > };
> > > +
> > > +static ssize_t proc_containerid_write(struct file *file, const char
> > > __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + struct inode *inode = file_inode(file);
> > > + u64 containerid;
> > > + int rv;
> > > + struct task_struct *task = get_proc_task(inode);
> > > +
> > > + if (!task)
> > > + return -ESRCH;
> > > + if (*ppos != 0) {
> > > + /* No partial writes. */
> > > + put_task_struct(task);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > > + if (rv < 0) {
> > > + put_task_struct(task);
> > > + return rv;
> > > + }
> > > +
> > > + rv = audit_set_containerid(task, containerid);
> > > + put_task_struct(task);
> > > + if (rv < 0)
> > > + return rv;
> > > + return count;
> > > +}
> > > +
> > > +static const struct file_operations proc_containerid_operations = {
> > > + .write = proc_containerid_write,
> > > + .llseek = generic_file_llseek,
> > > +};
> > > +
> > > #endif
> > >
> > > #ifdef CONFIG_FAULT_INJECTION
> > > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > > #endif
> > > #ifdef CONFIG_FAULT_INJECTION
> > > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > > proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> > > proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> > > CONFIG_AUDITSYSCALL REG("loginuid", S_IWUSR|S_IRUGO,
> > > proc_loginuid_operations), REG("sessionid",
Auditd syslog plugin
All, After enabling the syslog plugin for audispd and sending logs to a remote server I am seeing every event being written to /var/log/messages locally which is filling up /var. This is all redundant since local audit logs are kept in /var/log/audit. Is there a way to prevent auditd syslog plugin from writing to /var/log/messages? Thanks, Kevin -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
2018-06-01 22:05 GMT+02:00 Richard Guy Briggs :
> On 2018-06-01 10:12, Ondrej Mosnacek wrote:
>> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs :
>> > On 2018-05-30 10:45, Ondrej Mosnacek wrote:
>> >> This patch allows the AUDIR_DIR field to be used also with the exclude
>> >> filter.
>> >>
>> >> Not-yet-signed-off-by: Ondrej Mosnacek
>> >> ---
>> >> kernel/audit.c | 5 +++--
>> >> kernel/audit.h | 32 +++-
>> >> kernel/audit_tree.c | 4 +++-
>> >> kernel/auditfilter.c | 6 +-
>> >> kernel/auditsc.c | 28
>> >> 5 files changed, 42 insertions(+), 33 deletions(-)
>> >>
>> >> diff --git a/kernel/audit.c b/kernel/audit.c
>> >> index e7478cb58079..aac5b6ecc11d 100644
>> >> --- a/kernel/audit.c
>> >> +++ b/kernel/audit.c
>> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb,
>> >> struct nlmsghdr *nlh)
>> >> if (!audit_enabled && msg_type != AUDIT_USER_AVC)
>> >> return 0;
>> >>
>> >> - err = audit_filter(msg_type, AUDIT_FILTER_USER);
>> >> + // FIXME: do we need to pass the context here?
>> >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
>> >> if (err == 1) { /* match or error */
>> >> err = 0;
>> >> if (msg_type == AUDIT_USER_TTY) {
>> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct
>> >> audit_context *ctx, gfp_t gfp_mask,
>> >> if (audit_initialized != AUDIT_INITIALIZED)
>> >> return NULL;
>> >>
>> >> - if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
>> >> + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
>> >> return NULL;
>> >>
>> >> /* NOTE: don't ever fail/sleep on these two conditions:
>> >> diff --git a/kernel/audit.h b/kernel/audit.h
>> >> index 214e14948370..43cfeba25802 100644
>> >> --- a/kernel/audit.h
>> >> +++ b/kernel/audit.h
>> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head
>> >> *list);
>> >> #define audit_kill_trees(list) BUG()
>> >> #endif
>> >>
>> >> +struct audit_tree_refs {
>> >> + struct audit_tree_refs *next;
>> >> + struct audit_chunk *c[31];
>> >> +};
>> >> +
>> >> +/* A utility function to match tree refs: */
>> >> +static inline int match_tree_refs(struct audit_context *ctx, struct
>> >> audit_tree *tree)
>> >> +{
>> >> +#ifdef CONFIG_AUDIT_TREE
>> >> + struct audit_tree_refs *p;
>> >> + int n;
>> >> + if (!tree)
>> >> + return 0;
>> >> + /* full ones */
>> >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
>> >> + for (n = 0; n < 31; n++)
>> >> + if (audit_tree_match(p->c[n], tree))
>> >> + return 1;
>> >> + }
>> >> + /* partial */
>> >> + if (p) {
>> >> + for (n = ctx->tree_count; n < 31; n++)
>> >> + if (audit_tree_match(p->c[n], tree))
>> >> + return 1;
>> >> + }
>> >> +#endif
>> >> + return 0;
>> >> +}
>> >> +
>> >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t
>> >> len);
>> >>
>> >> extern pid_t audit_sig_pid;
>> >> extern kuid_t audit_sig_uid;
>> >> extern u32 audit_sig_sid;
>> >>
>> >> -extern int audit_filter(int msgtype, unsigned int listtype);
>> >> +extern int audit_filter(int msgtype, unsigned int listtype,
>> >> + struct audit_context *ctx);
>> >>
>> >> #ifdef CONFIG_AUDITSYSCALL
>> >> extern int audit_signal_info(int sig, struct task_struct *t);
>> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
>> >> index 67e6956c0b61..d4d36389c3d7 100644
>> >> --- a/kernel/audit_tree.c
>> >> +++ b/kernel/audit_tree.c
>> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
>> >>
>> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
>> >> {
>> >> + if (krule->listnr != AUDIT_FILTER_EXIT &&
>> >> + krule->listnr != AUDIT_FILTER_TYPE)
>> >> + return -EINVAL;
>> >>
>> >> if (pathname[0] != '/' ||
>> >> - rule->listnr != AUDIT_FILTER_EXIT ||
>> >> op != Audit_equal ||
>> >> rule->inode_f || rule->watch || rule->tree)
>> >> return -EINVAL;
>> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> >> index 6db9847ca031..e1d70cb77ea3 100644
>> >> --- a/kernel/auditfilter.c
>> >> +++ b/kernel/auditfilter.c
>> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname,
>> >> const char *path, int parentlen)
>> >> return strncmp(p, dname, dlen);
>> >> }
>> >>
>> >> -int audit_filter(int msgtype, unsigned int listtype)
>> >> +int audit_filter(int msgtype, unsigned int listtype, struct
>> >> audit_context *ctx)
>> >> {
>> >> struct audit_entry *e;
>> >> int ret = 1; /* Audit by default */
>> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgty
