Follow up on auditing cmdline

Follow up on auditing cmdline

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 1453 bytes --]

So this still seems to be lingering as unresolved in my mind. I need to
find out what the remaining reservations are on this feature. I am going to
try and summarize...

Steve Grub:
1. Anyway to use argv values as cmdline could be a page (too big)
2. Doesn't like disappearing audit entries

Richard Briggs:
1. Can we make it dynamic on/off

Stephen Smalley:
1. Can we cache the data for performance reasons

So I addressed RGB's issues, which led to one of steve Grub's concerns.
Which I can address both with if feature on then print cmdline=value else
print cmdline=(null)

Unfortunately the data I want to audit, is the full proc/cmdline entry,
which I think is the most
generic way of getting at potential vm data through various fork mazes on
Android, as well
as gathering the data on other architectures as well. This also prevents us
from hitting the
16 char width issue on task->comm. Increasing that will result in more
non-pageable kernel
memory use, versus my transient use of a page. I also need to make sure I
can get this
data before the process terminates, which can happen if I try to acquire it
in user-space.

Also, on error conditions, the last patch version will not print
cmdline=(null) which is an error and can be trivially corrected.

But before I put more time into it, I want to make sure the underlying idea
will be accepted, architectures, cacheing, print formats etc are all
trivial.


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 1807 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Follow up on auditing cmdline

[Steve Grubb]

On Tuesday, November 05, 2013 08:43:03 PM William Roberts wrote:
> So this still seems to be lingering as unresolved in my mind.

Yes, it is.

> I need to find out what the remaining reservations are on this feature. I am
> going to try and summarize...
> 
> Steve Grub:
> 1. Anyway to use argv values as cmdline could be a page (too big)
> 2. Doesn't like disappearing audit entries

There's more to it than this. I think you are overlooking my main point. This 
is a generic problem for all arches. Why don't we just solve the problem once 
and for all?

pgname is limited to 16 bytes. Why? Because that was a reasonable compromise 
way back in time when program names were short. With the Android naming 
convention 16 bytes is insufficient for anything listing processes like lsof, 
ps, netstat, etc. Why can't a new define for prctl be created to set an 
extended name? PR_SET_PROCTITLE. From what I saw in past discussions on LKML, 
there was a lot of concern that a trojan could change the proc title to 
something innocent and hide.

i think that is a losing argument because there are quite a few ways to 
accomplish the same thing. That is basically arguing for the status quo and 
that means the problem never gets fixed.

I think with the rise of Android's popularity, there is a growing importance 
to fix the problem.


> Richard Briggs:
> 1. Can we make it dynamic on/off

I object to this because we do not have any precedent for configuring a rule to 
get something and then another configuration to get all of it. I have worked on 
the audit system since 2004. A lot of maintainers have come and gone in that 
time. I don't want to see this subsystem have this oddity when they (people 
currently contributing patches) lose interest and go away.


> Stephen Smalley:
> 1. Can we cache the data for performance reasons
> 
> So I addressed RGB's issues, which led to one of steve Grub's concerns.
> Which I can address both with if feature on then print cmdline=value else
> print cmdline=(null)
> 
> Unfortunately the data I want to audit, is the full proc/cmdline entry,
> which I think is the most
> generic way of getting at potential vm data through various fork mazes on
> Android, as well
> as gathering the data on other architectures as well.

You know, we have the same issue with sudo. We have to get the full command 
line. What was done is sudo was patched to collect it and it emits a 
AUDIT_USER_CMD event. Maybe this works for you?

-Steve

> This also prevents us from hitting the
> 16 char width issue on task->comm. Increasing that will result in more
> non-pageable kernel
> memory use, versus my transient use of a page. I also need to make sure I
> can get this
> data before the process terminates, which can happen if I try to acquire it
> in user-space.
> 
> Also, on error conditions, the last patch version will not print
> cmdline=(null) which is an error and can be trivially corrected.
> 
> But before I put more time into it, I want to make sure the underlying idea
> will be accepted, architectures, cacheing, print formats etc are all
> trivial.

Re: Follow up on auditing cmdline

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 3911 bytes --]

On Wed, Nov 6, 2013 at 8:29 AM, Steve Grubb <sgrubb@redhat.com> wrote:

> On Tuesday, November 05, 2013 08:43:03 PM William Roberts wrote:
> > So this still seems to be lingering as unresolved in my mind.
>
> Yes, it is.
>
> > I need to find out what the remaining reservations are on this feature.
> I am
> > going to try and summarize...
> >
> > Steve Grub:
> > 1. Anyway to use argv values as cmdline could be a page (too big)
> > 2. Doesn't like disappearing audit entries
>
> There's more to it than this. I think you are overlooking my main point.
> This
> is a generic problem for all arches. Why don't we just solve the problem
> once
> and for all?
>
> pgname is limited to 16 bytes. Why? Because that was a reasonable
> compromise
> way back in time when program names were short. With the Android naming
> convention 16 bytes is insufficient for anything listing processes like
> lsof,
> ps, netstat, etc. Why can't a new define for prctl be created to set an
> extended name? PR_SET_PROCTITLE. From what I saw in past discussions on
> LKML,
> there was a lot of concern that a trojan could change the proc title to
> something innocent and hide.
>

Yes we share that concern. If you want to move something into prctl for
this, I am ok with doing that. It makes
caching the values a possibility as you can detect when the value needs to
be updated. Now to prevent
it from having the same issues as setproctitle() I should probably bind
this to a capability of some sort, should
I reuse and existing one or create a new one?


>
> i think that is a losing argument because there are quite a few ways to
> accomplish the same thing. That is basically arguing for the status quo and
> that means the problem never gets fixed.
>
> I think with the rise of Android's popularity, there is a growing
> importance
> to fix the problem.
>
>
> > Richard Briggs:
> > 1. Can we make it dynamic on/off
>
> I object to this because we do not have any precedent for configuring a
> rule to
> get something and then another configuration to get all of it. I have
> worked on
> the audit system since 2004. A lot of maintainers have come and gone in
> that
> time. I don't want to see this subsystem have this oddity when they (people
> currently contributing patches) lose interest and go away.
>

>
> > Stephen Smalley:
> > 1. Can we cache the data for performance reasons
> >
> > So I addressed RGB's issues, which led to one of steve Grub's concerns.
> > Which I can address both with if feature on then print cmdline=value else
> > print cmdline=(null)
> >
> > Unfortunately the data I want to audit, is the full proc/cmdline entry,
> > which I think is the most
> > generic way of getting at potential vm data through various fork mazes on
> > Android, as well
> > as gathering the data on other architectures as well.
>
> You know, we have the same issue with sudo. We have to get the full command
> line. What was done is sudo was patched to collect it and it emits a
> AUDIT_USER_CMD event. Maybe this works for you?
>

I really like this suggestion, the only downfall is that its not directly
tied to the audit record, but
can be joined together later based on pid and other various identifiers.



>
> -Steve
>
> > This also prevents us from hitting the
> > 16 char width issue on task->comm. Increasing that will result in more
> > non-pageable kernel
> > memory use, versus my transient use of a page. I also need to make sure I
> > can get this
> > data before the process terminates, which can happen if I try to acquire
> it
> > in user-space.
> >
> > Also, on error conditions, the last patch version will not print
> > cmdline=(null) which is an error and can be trivially corrected.
> >
> > But before I put more time into it, I want to make sure the underlying
> idea
> > will be accepted, architectures, cacheing, print formats etc are all
> > trivial.
>
>


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 5261 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]