Re: [PATCH V9 3/3] audit: add audit by children of executable path

[Richard Guy Briggs]

On 15/08/07, Paul Moore wrote:
> On Fri, Aug 7, 2015 at 12:03 PM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > On 15/08/07, Paul Moore wrote:
> >> On Friday, August 07, 2015 02:37:15 AM Richard Guy Briggs wrote:
> >> > On 15/08/06, Paul Moore wrote:
> >> >
> >> > > I guess what I'm saying is that I'm not currently convinced that
> >> > > there is enough value in this to offset the risk I feel the loop
> >> > > presents. I understand the use cases that you are mentioning, the
> >> > > are the same as the last time we discussed this, but I'm going to
> >> > > need something better than that.
> >> >
> >> > Can you better describe the loop that concerns you?  I don't quite see
> >> > it.
> >>
> >> It would be the only loop in the patch, look at the for loop in
> >> audit_filter_rules() which iterates up the process' parent chain.
> >
> > Sorry, I should reword that...  What risk do you see in that loop?  It
> > works up the task ancestry tree until it triggers, or hits init for that
> > PID namespace that terminates the loop.  Do you see a risk in the
> > numerical pids rolling underneath the loop?
> 
> I suppose there is some risk of PID overlap, and while that is a
> concern, it isn't my first.
> 
> My main concern is that a malicious user could add an extra level of
> burden to the system by making an absurdly tall process tree and then
> hammer the system with trivial, short lived syscalls.  Granted, there
> are userspace limits which would bound the impact to some extent, but
> there is no way to really reduce the risk.  You could further put hard
> limits on the loop, but what good would that do?  Malicious users
> would just know to blow past that limit before they did their Evil
> Deeds.
> 
> I'll say it again; I'm not completely opposed to something like this -
> perhaps in some modified form - but I have yet to see a need for this
> functionality that is great enough to counter the risk.

I am not going to lobby hard for it.  I split this part of the patch out
to avoid jeopardizing the acceptability of the rest of the patchset and
to isolate it to make it easier to focus on its issues and apply it
later once they are addressed.

I'll reflect on this concern and see if I can come up with any ways to
minimize this danger.  This issue is related to the request to list the
chain of processes back to the first ancestor in each record.  You can
make a best effort to record or track the entire chain, but at some
point need to put a limit on it to avoid a DoS, at which point there is
no point in listing the information since it is incomplete.  (Too many
"point"s in that last sentence...)

> > I *do* notice that find_task_by_vpid(pid_t) must be replaced with
> > find_task_by_pid_ns(pid_t, &init_pid_ns), since task_struct->pid is
> > always stored in the initial PID namespace.
> 
> Another thing that needs to be resolved.

I've already fixed it in my tree:
	ptsk = find_task_by_pid_ns(ptsk->parent->pid) &init_pid_ns)

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@xxxxxxxxxx>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
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/