Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

From: Paul Moore <paul@paul-moore.com>
To: rgb@redhat.com
Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org,
	Eric Paris <eparis@parisplace.org>,
	sgrubb@redhat.com, aviro@redhat.com
Subject: Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved
Date: Fri, 20 Jul 2018 10:21:50 -0400	[thread overview]
Message-ID: <CAHC9VhT0WnmAGWk5pLJ9TYusdb_8Dububdi=8tWnvoaY+NMnSQ@mail.gmail.com> (raw)
In-Reply-To: <20180720132737.nukm4xycty6mozh6@madcap2.tricolour.ca>

On Fri, Jul 20, 2018 at 9:30 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-19 18:47, Paul Moore wrote:
> > On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-07-18 17:45, Paul Moore wrote:
> > > > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > On 2018-06-28 15:41, Paul Moore wrote:
> > > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > > The failure to add an audit rule due to audit locked gives no clue
> > > > > > > what CONFIG_CHANGE operation failed.
> > > > > > > Similarly the set operation is the only other operation that doesn't
> > > > > > > give the "op=" field to indicate the action.
> > > > > > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > > > > > to what sort of configuration change is being executed.
> > > > > > >
> > > > > > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > > > > > op= field, add them to bring them in line with the rest.
> > > > > >
> > > > > > Normally this would be an immediate reject because this patch inserts
> > > > > > a field into an existing record, but the CONFIG_CHANGE record is so
> > > > > > variable (supposedly bad in its own right) that I don't this really
> > > > > > matters.
> > > > > >
> > > > > > With that out of the way, I think this patch is fine, but I don't
> > > > > > think it is complete.  At the very least there is another
> > > > > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > > > > > appear to include an "op" field.  If we want to make sure we have an
> > > > > > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > > > > > :)
> > > > >
> > > > > The version I'm looking at already had it when it was added in 2009.
> > > >
> > > > Yup, there it is ... now I'm wondering what tree I was looking at as a
> > > > reference while reviewing this?
> > > >
> > > > /me scratches head
> > > >
> > > > > This one doesn't add the auid and ses fields because they will be
> > > > > covered by the linking of this record with the syscall record via the
> > > > > audit_context() introduced in another patch.
> > > >
> > > > Yeah, I'm not concerned about that for the reasons you state.
> > > >
> > > > > > and one more in audit_receive_msg().  There may be more.
> > > > >
> > > > > I believe they're covered by other patches in the ghak59 set.
> > > >
> > > > If they are in the later patches it might be good to move those "op="
> > > > additions into this patch.
> > >
> > > I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
> > > that are missing op= field.  Can you narrow it down?
> >
> > Well, just grep'ing my way through audit_receive_msg() I see that
> > AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.
>
> The failure case is addressed in this patch.  The success case is
> addressed in audit_log_rule_change().  The latter already has it.  What
> is the problem?  What tree are you looking at?  What am I missing?

So it does.  This discussion dragged out long enough that I forgot to
check the actual patch submission.

I think this patch is fine, I would recommend updating this patchset
using the feedback on the other individual patches and resubmitting.

-- 
paul moore
www.paul-moore.com