Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Steve Grubb <sgrubb@redhat.com>
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     Paul Moore <paul@paul-moore.com>, linux-audit@redhat.com,
        linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
Date:   Mon, 13 Nov 2017 13:30:00 -0500
Message-ID: <1785304.d0a7c0oaZ9@x2>
Organization: Red Hat
In-Reply-To: <20171109205246.ohro2gbsnvc7hpij@madcap2.tricolour.ca>
References: <fa0938a05c2d783556d67d4dafef208b156a35ad.1503484357.git.rgb@redhat.com> <CAHC9VhQMzR06Tqr8WNDuV9zgnDfH8reaKam+e_sAFCm1ZK3QQQ@mail.gmail.com> <20171109205246.ohro2gbsnvc7hpij@madcap2.tricolour.ca>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
> > >> > It might be simplest to just apply a corrective patch over top of
> > >> > this one so that you don't have to muck about with git branches and
> > >> > commit messages.
> > >> 
> > >> A quick note on the "corrective patch": given we are just days away
> > >> from the merge window opening, it is *way* to late for something like
> > >> that, at this point the only options are to leave it as-is or
> > >> yank/revert and make another pass during the next development phase.
> > > 
> > > Then yank it. I think that is overreacting but given the options you
> > > presented its the only one that avoids changing a critical field
> > > format.
> > 
> > It's not overreacting Steve, there is simply no way we can test and
> > adequately soak new changes in the few days we have left.

Its just moving the output of the information a few lines down further in the 
code. 10 minutes of work, tops.

> > Event yanks/reverts carry a risk at this stage, but I consider that the
> > less risky option for these patches.  Neither is a great option, and that
> > is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.
> 
> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

The path should stay. Just the file system type needs decoupling and moving.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?". 

I would let it optionally swing in and out at the end of the record. This 
should never show up on a normal system because the rules will suppress 
generating this information by default. So, it really won't be all that 
visible.

> How are the analysis tools stymied by a text prefix to a path that it can't
> find anyways?

Because they do not actually resolve anything in the file system. They take 
the event as ground truth and use that. Also, the tools expect name=value. 
This has been the way since the beginning. We do not lump multiple independent 
values together. And then what if the path has a special character in it? The 
whole value then has to be encoded. And I don't think the patch is using 
untrusted string like it should.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.
> 
> > >> As for the objection itself: ungh.  There is really no good reason why
> > >> you couldn't have seen this in the *several* *months* prior to this;
> > >> Richard wrote a nice patch description which *included* sample audit
> > >> events, and you were involved in discussions regarding this patchset.
> > >> To say I'm disappointed would be an understatement.
> > > 
> > > I am also disappointed to find that we are modifying a searchable field
> > > that has been defined since 2005. The "name" field is very important.
> > > It's used in quite a few reports, its used in the text format, it's
> > > searchable, and we have a dictionary that defines exactly what it is.
> > > Fields that are searchable and used in common reports cannot be changed
> > > without a whole lot of coordination. I'm also disappointed to have to
> > > point out that new information should go in its own field. I thought
> > > this was common knowledge. In any event, it was caught and problems can
> > > be avoided.
> 
> So why does this make it unsearchable?

I didn't say it makes in unsearchable, but now that you mention it...it does 
in one case. Searchable fields are more important. They typically are the 
object or subject or some kind of special attribute that is commonly searched 
on to group events. Searchable fields can either be partial or full word 
match. By combining information in the same field, it will change this 
behavior. The path name is the object of the event. By combining information 
that is not the object with it, everyone will have to change and update their 
software to handle this change in behavior.

> I still don't understand any explanations that have been made so far

Try ausearch --text on those events, or aureport --file.

-Steve

From 1583627318314107769@xxx Thu Nov 09 21:58:20 +0000 2017
X-GM-THRID: 1576519731154696149
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread