Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity inaudit rules and data

From: Richard Guy Briggs
Date: Thu May 09 2013 - 11:10:27 EST

Next message: Steven Rostedt: "Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_funcsucceeded"
Previous message: Peter Zijlstra: "Re: [PATCH 0/9] perf: Adding better precise_ip field handling"
In reply to: Richard Guy Briggs: "Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity inaudit rules and data"
Next in thread: Eric Paris: "Re: [PATCH] [BZ905179] audit: omit check for uid and gid validityin audit rules and data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, May 09, 2013 at 09:52:47AM -0400, Richard Guy Briggs wrote:
> On Thu, May 09, 2013 at 09:29:18AM -0400, Steve Grubb wrote:
> > On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> > > On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > > > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> writes:
> > > > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs <rgb@xxxxxxxxxx>
> > wrote:
> > > > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > > > >> EINVAL.>
> > > > The only case where this appears to make the least little bit of sense
> > > > is if the goal of the test is to test to see if an audit logloginuid
> > > > has been set at all. In which case depending on a test against
> > > > 4294967295 is bogus because you are depending on an intimate internal
> > > > kernel implementation detail.
> > > >
> > > > How about something like my untested patch below that add an explicit
> > > > operation to test if loginuid has been set?
> > >
> > > Sorry for the delay in testing this, I had another urgent bug and a
> > > belligerent test box...
> > >
> > > I like this approach better than mine now that I understand it. I've
> > > tested the patch below without any changes. It works as expected with
> > > my previous test case. I don't know if a Signed-off-by: is appropriate
> > > for me in this case, but I'll throw in a:
> > >
> > > Tested-by: Richard Guy Briggs <rbriggs@xxxxxxxxxx>
> > >
> > > and recommend a:
> > >
> > > Reported-By: Steve Grubb <sbrubb@xxxxxxxxxx>
> >
> > If this is the approved patch, can it be put in stable? The audit system
> > hasn't worked as intended since January.
>
> Most certainly. I should have added that. I only thought about that
> after sending.

No worries, Eric added it in his set in his pull request to Linus.

> > Thanks,
> > -Steve
> >
> > > > From: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > > > Date: Tue, 9 Apr 2013 02:22:10 -0700
> > > > Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> > > >
> > > > audit rule additions containing "-F auid!=4294967295" were failing
> > > > with EINVAL.
> > > >
> > > > Apparently some userland audit rule sets want to know if loginuid uid
> > > > has been set and are using a test for auid != 4294967295 to determine
> > > > that.
> > > >
> > > > In practice that is a horrible way to ask if a value has been set,
> > > > because it relies on subtle implementation details and will break
> > > > every time the uid implementation in the kernel changes.
> > > >
> > > > So add a clean way to test if the audit loginuid has been set, and
> > > > silently convert the old idiom to the cleaner and more comprehensible
> > > > new idiom.
> > > >
> > > > Reported-By: Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > > > Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > include/linux/audit.h | 5 +++++
> > > > include/uapi/linux/audit.h | 1 +
> > > > kernel/auditfilter.c | 29 +++++++++++++++++++++++++++++
> > > > kernel/auditsc.c | 5 ++++-
> > > > 4 files changed, 39 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index a9fefe2..8a1ddde 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
> > > > *t)
> > > >
> > > > #define audit_signals 0
> > > > #endif /* CONFIG_AUDITSYSCALL */
> > > >
> > > > +static inline bool audit_loginuid_set(struct task_struct *tsk)
> > > > +{
> > > > + return uid_valid(audit_get_loginuid(tsk));
> > > > +}
> > > > +
> > > >
> > > > #ifdef CONFIG_AUDIT
> > > > /* These are defined in audit.c */
> > > >
> > > > /* Public API */
> > > >
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index 9f096f1..9554a19 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -246,6 +246,7 @@
> > > >
> > > > #define AUDIT_OBJ_TYPE 21
> > > > #define AUDIT_OBJ_LEV_LOW 22
> > > > #define AUDIT_OBJ_LEV_HIGH 23
> > > >
> > > > +#define AUDIT_LOGINUID_SET 24
> > > >
> > > > /* These are ONLY useful when checking
> > > >
> > > > * at syscall exit time (AUDIT_AT_EXIT). */
> > > >
> > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > > index 540f986..6381d17 100644
> > > > --- a/kernel/auditfilter.c
> > > > +++ b/kernel/auditfilter.c
> > > > @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > > audit_rule *rule)>
> > > > if (f->op == Audit_bad)
> > > >
> > > > goto exit_free;
> > > >
> > > > + /* Support legacy tests for a valid loginuid */
> > > > + if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > > + f->type = AUDIT_LOGINUID_SET;
> > > > + f->val = 0;
> > > > + }
> > > > +
> > > >
> > > > switch(f->type) {
> > > >
> > > > default:
> > > > goto exit_free;
> > > >
> > > > @@ -377,6 +383,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > > > audit_rule *rule)>
> > > > if (!gid_valid(f->gid))
> > > >
> > > > goto exit_free;
> > > >
> > > > break;
> > > >
> > > > + case AUDIT_LOGINUID_SET:
> > > > + if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > > + goto exit_free;
> > > > + if ((f->val != 0) && (f->val != 1))
> > > > + goto exit_free;
> > > > + break;
> > > >
> > > > case AUDIT_PID:
> > > > case AUDIT_PERS:
> > > >
> > > > case AUDIT_MSGTYPE:
> > > > @@ -459,6 +471,13 @@ static struct audit_entry *audit_data_to_entry(struct
> > > > audit_rule_data *data,>
> > > > f->gid = INVALID_GID;
> > > > f->lsm_str = NULL;
> > > > f->lsm_rule = NULL;
> > > >
> > > > +
> > > > + /* Support legacy tests for a valid loginuid */
> > > > + if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > > > + f->type = AUDIT_LOGINUID_SET;
> > > > + f->val = 0;
> > > > + }
> > > > +
> > > >
> > > > switch(f->type) {
> > > > case AUDIT_UID:
> > > >
> > > > case AUDIT_EUID:
> > > > @@ -487,6 +506,12 @@ static struct audit_entry *audit_data_to_entry(struct
> > > > audit_rule_data *data,>
> > > > if (!gid_valid(f->gid))
> > > >
> > > > goto exit_free;
> > > >
> > > > break;
> > > >
> > > > + case AUDIT_LOGINUID_SET:
> > > > + if ((f->op != Audit_not_equal) && (f->op != Audit_equal))
> > > > + goto exit_free;
> > > > + if ((f->val != 0) && (f->val != 1))
> > > > + goto exit_free;
> > > > + break;
> > > >
> > > > case AUDIT_PID:
> > > > case AUDIT_PERS:
> > > >
> > > > case AUDIT_MSGTYPE:
> > > > @@ -1380,6 +1405,10 @@ static int audit_filter_user_rules(struct
> > > > audit_krule *rule,>
> > > > result = audit_uid_comparator(audit_get_loginuid(current),
> > > >
> > > > f->op, f->uid);
> > > >
> > > > break;
> > > >
> > > > + case AUDIT_LOGINUID_SET:
> > > > + result = audit_comparator(audit_loginuid_set(current),
> > > > + f->op, f->val);
> > > > + break;
> > > >
> > > > case AUDIT_SUBJ_USER:
> > > > case AUDIT_SUBJ_ROLE:
> > > >
> > > > case AUDIT_SUBJ_TYPE:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 3a11d34..27d0a50 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -750,6 +750,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > > >
> > > > if (ctx)
> > > >
> > > > result = audit_uid_comparator(tsk->loginuid, f->op,
> > f->uid);
> > > >
> > > > break;
> > > >
> > > > + case AUDIT_LOGINUID_SET:
> > > > + result = audit_comparator(audit_loginuid_set(tsk), f->op,
> > f->val);
> > > > + break;
> > > >
> > > > case AUDIT_SUBJ_USER:
> > > > case AUDIT_SUBJ_ROLE:
> > > >
> > > > case AUDIT_SUBJ_TYPE:
> > > > @@ -2317,7 +2320,7 @@ int audit_set_loginuid(kuid_t loginuid)
> > > >
> > > > unsigned int sessionid;
> > > >
> > > > #ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
> > > >
> > > > - if (uid_valid(task->loginuid))
> > > > + if (audit_loginuid_set(task))
> > > >
> > > > return -EPERM;
> > > >
> > > > #else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
> > > >
> > > > if (!capable(CAP_AUDIT_CONTROL))
> > >
> > > - RGB
> > >
> > > --
> > > Richard Guy Briggs <rbriggs@xxxxxxxxxx>
> > > Senior Software Engineer
> > > AMER ENG Base Operating Systems
> > > Remote, Canada, Ottawa
> > > Voice: 1.647.777.2635
> > > Internal: (81) 32635
> > >
> > > --
> > > Linux-audit mailing list
> > > Linux-audit@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@xxxxxxxxxx>
> Senior Software Engineer
> AMER ENG Base Operating Systems
> Remote, Ottawa, Canada
> Voice: 1.647.777.2635
> Internal: (81) 32635

- RGB

--
Richard Guy Briggs <rbriggs@xxxxxxxxxx>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Steven Rostedt: "Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_funcsucceeded"
Previous message: Peter Zijlstra: "Re: [PATCH 0/9] perf: Adding better precise_ip field handling"
In reply to: Richard Guy Briggs: "Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity inaudit rules and data"
Next in thread: Eric Paris: "Re: [PATCH] [BZ905179] audit: omit check for uid and gid validityin audit rules and data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]