[PATCH] loginuid change logging details

[PATCH] loginuid change logging details

[Richard Guy Briggs]

I missed posting this before the holidays.  I discovered this while adding
other information to other message types.

It seemed to me that loginuid changes were significantly missing context
references.  This patch adds that.  Is this sufficient, or is there more
information missing too?  If this is sufficient, stop reading this cover letter
and review the patch.  If it is not sufficient, keep reading below...

The question has been raised that perhaps we should be switching this to use
audit_log_task_info() istead which adds a whole lot more information about this
task.

In the existing message
	pid
	uid
are already given, before
	old-auid
	new-auid
	old-ses
	new-ses
.

The function audit_log_task_info() gives:
	ppid
	pid
	auid
	uid
	gid
	euid
	suid
	fsuid
	egid
	sgid
	fsgid
	tty
	ses
        comm
	exe
	res
.

So, 
	pid
	uid
are in the right order, along with
	new-auid (auid)
	new-ses (ses)
but if we give the
	old-auid
	old-ses
values first, then call audit_log_task_info(), the old values will preceed 
	pid
	uid
.

Is this re-ordering acceptable to gain more information and reduce code duplicity?


Richard Guy Briggs (1):
  audit: log task context when setting loginuid

 kernel/auditsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

[PATCH] audit: log task context when setting loginuid

[Richard Guy Briggs]

Add task context to log when loginuid is being set.
---
 kernel/auditsc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 10176cd..62500fe 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1981,6 +1981,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
 	if (!ab)
 		return;
+	audit_log_task_context(ab);
 	audit_log_format(ab, "pid=%d uid=%u"
 			 " old-auid=%u new-auid=%u old-ses=%u new-ses=%u"
 			 " res=%d",
-- 
1.7.1

Re: [PATCH] loginuid change logging details

[Eric Paris]

I think this just touches the surface of what be/have been done.  There
appears to be no logic, consistency, or predictability to audit logs.
They are a piecemeal mish mash of information.  Every record has
whatever fields it has, not because there is some coherent reason, but
because that's what somebody accidentally put in there when the record
was created.

This is a great example.  The only things we log are the pid and uid.
The information in the uid field is actually useless (given the
uid==new-auid but anyway) but clearly for searching reasons it makes
sense.  But, why is pid and uid enough?  Why not selinux context?  Why
not ppid?  Why not all of the *id's (gid, fsuid, etc.) Why not comm?
why not tty?

What is the minimal set of information we should be sending with every
record that uniquely identifies a process?  Why is every record it's own
little world?  I know we grew organically to get here, but I'd really
like to find a consistent usable future...

Maybe we don't need everything from a syscall record in every record,
but there is some meaningful subset which identifies a process and user.
What is that subset?


On Fri, 2014-01-17 at 18:34 -0500, Richard Guy Briggs wrote:
> I missed posting this before the holidays.  I discovered this while adding
> other information to other message types.
> 
> It seemed to me that loginuid changes were significantly missing context
> references.  This patch adds that.  Is this sufficient, or is there more
> information missing too?  If this is sufficient, stop reading this cover letter
> and review the patch.  If it is not sufficient, keep reading below...
> 
> The question has been raised that perhaps we should be switching this to use
> audit_log_task_info() istead which adds a whole lot more information about this
> task.
> 
> In the existing message
> 	pid
> 	uid
> are already given, before
> 	old-auid
> 	new-auid
> 	old-ses
> 	new-ses
> .
> 
> The function audit_log_task_info() gives:
> 	ppid
> 	pid
> 	auid
> 	uid
> 	gid
> 	euid
> 	suid
> 	fsuid
> 	egid
> 	sgid
> 	fsgid
> 	tty
> 	ses
>         comm
> 	exe
> 	res
> .
> 
> So, 
> 	pid
> 	uid
> are in the right order, along with
> 	new-auid (auid)
> 	new-ses (ses)
> but if we give the
> 	old-auid
> 	old-ses
> values first, then call audit_log_task_info(), the old values will preceed 
> 	pid
> 	uid
> .
> 
> Is this re-ordering acceptable to gain more information and reduce code duplicity?
> 
> 
> Richard Guy Briggs (1):
>   audit: log task context when setting loginuid
> 
>  kernel/auditsc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 

Re: [PATCH] loginuid change logging details

[Steve Grubb]

On Friday, January 17, 2014 06:34:56 PM Richard Guy Briggs wrote:
> I missed posting this before the holidays.  I discovered this while adding
> other information to other message types.
> 
> It seemed to me that loginuid changes were significantly missing context
> references.  This patch adds that.  Is this sufficient, or is there more
> information missing too?  If this is sufficient, stop reading this cover
> letter and review the patch.  If it is not sufficient, keep reading
> below...
> 
> The question has been raised that perhaps we should be switching this to use
> audit_log_task_info() istead which adds a whole lot more information about
> this task.
> 
> In the existing message
> 	pid
> 	uid
> are already given, before
> 	old-auid
> 	new-auid
> 	old-ses
> 	new-ses

I'd rather have old-auid/ses and auid/ses so that I don't have to expand 
everything to start looking for 'new-' variants. think of all the values as 
current as of when the syscall completes successfully and the proposed values 
if denied.

-Steve


> The function audit_log_task_info() gives:
> 	ppid
> 	pid
> 	auid
> 	uid
> 	gid
> 	euid
> 	suid
> 	fsuid
> 	egid
> 	sgid
> 	fsgid
> 	tty
> 	ses
>         comm
> 	exe
> 	res
> .
> 
> So,
> 	pid
> 	uid
> are in the right order, along with
> 	new-auid (auid)
> 	new-ses (ses)
> but if we give the
> 	old-auid
> 	old-ses
> values first, then call audit_log_task_info(), the old values will preceed
> 	pid
> 	uid
> .
> 
> Is this re-ordering acceptable to gain more information and reduce code
> duplicity?
> 
> 
> Richard Guy Briggs (1):
>   audit: log task context when setting loginuid
> 
>  kernel/auditsc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Re: [PATCH] loginuid change logging details

[Steve Grubb]

On Monday, January 20, 2014 11:44:49 AM Eric Paris wrote:
> I think this just touches the surface of what be/have been done.  There
> appears to be no logic, consistency, or predictability to audit logs.

Kernel maintainers have not added all the fields I have asked for at some 
points. I think it was proposed to add a syscall record to everything which I 
absolutely do not want to see. that is too much information.


> They are a piecemeal mish mash of information.  Every record has
> whatever fields it has, not because there is some coherent reason, but
> because that's what somebody accidentally put in there when the record
> was created.

What is required is this:

1) when it happened
2) who did it
3) what did they act upon
4) what did they do
5) what was the result


#1 is done automatically.
#2 depends on which API the action occurred on and if we have a MAC subsystem 
or not. For netlink, we are limited to what rides along in the skb. For the 
syscall interface, we have everything. For actions through /proc, we probably 
can have everything.  Then there are various events embedded in the kernel 
like the IMA events which trigger when they get loaded. So, what is necessary 
to identify the subject? In descending order of importance: auid, uid, ses, 
tty, pid, subj, exe, comm, euid, gid, egid, everything else.
#3 is usually given by auxiliary records such as PATH, OBJ_PID, EXECVE, etc. 
They should include fields that make it possible to know exactly what was acted 
upon such as device, inode, ownership/group, permissions, object labels, or 
whatever it takes to know what was being manipulated depending on what it 
actually is.
#4 is given by the event type. In cases where the event type is multiplexed, 
we need a op= field to say what operation is being performed. This is trusted 
information and never under attacker control and should never be logged as an 
untrusted string. In cases where there might be a space between words, just 
convert the space to a dash.
#5 This is a simple res= field.

That said...whatever we have right now, the audit utilities looks for that and 
only that. If we suddenly added a bunch of new fields, I still have to support 
all the mistakes of the past. The 2.x user space audit code supports all 
kernels since 2.6.16. Because there is no way to tell when features got added 
to what kernels, it will always have to support the whole mess. All of it. 
With or without the (proposed) new fields present.


> This is a great example.  The only things we log are the pid and uid.
> The information in the uid field is actually useless (given the
> uid==new-auid but anyway) but clearly for searching reasons it makes
> sense.  But, why is pid and uid enough?  Why not selinux context?  Why
> not ppid?  Why not all of the *id's (gid, fsuid, etc.) Why not comm?
> why not tty?

Because kernel maintainers didn't add it.


> What is the minimal set of information we should be sending with every
> record that uniquely identifies a process?  Why is every record it's own
> little world?

To save disk space. That is paramount. We cannot add syscall to everything 
without eating up a lot of disk space. The main thing to remember is that 
people who really use auditing never have enough disk space to keep everything 
they want. So, we should always consider doing anything possible to minimize 
disk usage no matter what.


>  I know we grew organically to get here, but I'd really
> like to find a consistent usable future...
> 
> Maybe we don't need everything from a syscall record in every record,
> but there is some meaningful subset which identifies a process and user.
> What is that subset?

See above.

HTH...

-Steve

 
> On Fri, 2014-01-17 at 18:34 -0500, Richard Guy Briggs wrote:
> > I missed posting this before the holidays.  I discovered this while adding
> > other information to other message types.
> > 
> > It seemed to me that loginuid changes were significantly missing context
> > references.  This patch adds that.  Is this sufficient, or is there more
> > information missing too?  If this is sufficient, stop reading this cover
> > letter and review the patch.  If it is not sufficient, keep reading
> > below...
> > 
> > The question has been raised that perhaps we should be switching this to
> > use audit_log_task_info() istead which adds a whole lot more information
> > about this task.
> > 
> > In the existing message
> > 
> > 	pid
> > 	uid
> > 
> > are already given, before
> > 
> > 	old-auid
> > 	new-auid
> > 	old-ses
> > 	new-ses
> > 
> > .
> > 
> > The function audit_log_task_info() gives:
> > 	ppid
> > 	pid
> > 	auid
> > 	uid
> > 	gid
> > 	euid
> > 	suid
> > 	fsuid
> > 	egid
> > 	sgid
> > 	fsgid
> > 	tty
> > 	ses
> > 	
> >         comm
> > 	
> > 	exe
> > 	res
> > 
> > .
> > 
> > So,
> > 
> > 	pid
> > 	uid
> > 
> > are in the right order, along with
> > 
> > 	new-auid (auid)
> > 	new-ses (ses)
> > 
> > but if we give the
> > 
> > 	old-auid
> > 	old-ses
> > 
> > values first, then call audit_log_task_info(), the old values will preceed
> > 
> > 	pid
> > 	uid
> > 
> > .
> > 
> > Is this re-ordering acceptable to gain more information and reduce code
> > duplicity?> 
> > Richard Guy Briggs (1):
> >   audit: log task context when setting loginuid
> >  
> >  kernel/auditsc.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)

Re: [PATCH] loginuid change logging details

[Eric Paris]

On Mon, 2014-02-03 at 12:03 -0500, Steve Grubb wrote:
> On Monday, January 20, 2014 11:44:49 AM Eric Paris wrote:
> > I think this just touches the surface of what be/have been done.  There
> > appears to be no logic, consistency, or predictability to audit logs.
> 
> Kernel maintainers have not added all the fields I have asked for at some 
> points. I think it was proposed to add a syscall record to everything which I 
> absolutely do not want to see. that is too much information.

Where did you ask?  That's the whole point of this e-mail, and I finish
reading your response and still don't know the answer...

> What is required is this:

> 2) who did it

This is the only part that we have question/inconsistency/stoopidity
with, that I can see.  But I still don't know how to solve it.

> #2 depends on which API the action occurred on and if we have a MAC subsystem 
> or not.

What does MAC have to do with it?

>  For netlink, we are limited to what rides along in the skb.

Not true. (this was true in the past, but not for years).  We (in
kernel) know everything about the task that sends a netlink message.

The place we have the least information is in the kaudit code storing
who sent a signal to auditd.  I'll avoid that nightmare though...

> For the 
> syscall interface, we have everything. For actions through /proc, we probably 
> can have everything.  Then there are various events embedded in the kernel 
> like the IMA events which trigger when they get loaded. So, what is necessary 
> to identify the subject? In descending order of importance: 

> auid, uid, ses, 
> tty, pid, subj, exe, comm, euid, gid, egid, everything else.

Ok, so you want these from every audit event?  All of these?  And these
are all that matter?  What does 'everything else' mean?  Do we want
more?  Do we not?

That's the point of the question.  What fields about the task doing an
operation should be included in events....

> > What is the minimal set of information we should be sending with every
> > record that uniquely identifies a process?  Why is every record it's own
> > little world?
> 
> To save disk space. That is paramount. We cannot add syscall to everything 
> without eating up a lot of disk space. The main thing to remember is that 
> people who really use auditing never have enough disk space to keep everything 
> they want. So, we should always consider doing anything possible to minimize 
> disk usage no matter what.

Bam, back to senseless non-uniform mishmash mess....

Re: [PATCH] loginuid change logging details

[Steve Grubb]

On Monday, February 03, 2014 12:43:50 PM Eric Paris wrote:
> On Mon, 2014-02-03 at 12:03 -0500, Steve Grubb wrote:
> > On Monday, January 20, 2014 11:44:49 AM Eric Paris wrote:
> > > I think this just touches the surface of what be/have been done.  There
> > > appears to be no logic, consistency, or predictability to audit logs.
> > 
> > Kernel maintainers have not added all the fields I have asked for at some
> > points. I think it was proposed to add a syscall record to everything
> > which I absolutely do not want to see. that is too much information.
> 
> Where did you ask?  That's the whole point of this e-mail, and I finish
> reading your response and still don't know the answer...

On this mail list years ago, IRC, telecons, all over. Besides, I shouldn't be 
the only one pointing this stuff out. Whoever is maintaining this really needs 
to understand it and just fix it....along time ago.


> > What is required is this:
> > 
> > 2) who did it
> 
> This is the only part that we have question/inconsistency/stoopidity
> with, that I can see.  But I still don't know how to solve it.
> 
> > #2 depends on which API the action occurred on and if we have a MAC
> > subsystem or not.
> 
> What does MAC have to do with it?

Because if its not a MAC system, there is no subject label. CAPP disavows any 
knowledge of labels.


> >  For netlink, we are limited to what rides along in the skb.
> 
> Not true. (this was true in the past, but not for years).  We (in
> kernel) know everything about the task that sends a netlink message.
> 
> The place we have the least information is in the kaudit code storing
> who sent a signal to auditd.  I'll avoid that nightmare though...

That's another place.

> > For the
> > syscall interface, we have everything. For actions through /proc, we
> > probably can have everything.  Then there are various events embedded in
> > the kernel like the IMA events which trigger when they get loaded. So,
> > what is necessary to identify the subject? In descending order of
> > importance:
> > 
> > auid, uid, ses,
> > tty, pid, subj, exe, comm, euid, gid, egid, everything else.
> 
> Ok, so you want these from every audit event?

That would have been nice.

> All of these?  And these are all that matter?

Its a pretty good list. There may be times when something else is important, 
but we'd probably need to see the event.


>  What does 'everything else' mean?  Do we want
> more?  Do we not?

Everything else as in fsuid and minor permissions. Those are important in a 
syscall related to opening a file, but is wasting disk space for any other 
syscall. IOW, just adding a syscall record like has been done in the past just 
wastes disk space.

> That's the point of the question.  What fields about the task doing an
> operation should be included in events....

As many of the important ones as can be gotten in the same order for every 
record.


> > > What is the minimal set of information we should be sending with every
> > > record that uniquely identifies a process?  Why is every record it's own
> > > little world?
> > 
> > To save disk space. That is paramount. We cannot add syscall to everything
> > without eating up a lot of disk space. The main thing to remember is that
> > people who really use auditing never have enough disk space to keep
> > everything they want. So, we should always consider doing anything
> > possible to minimize disk usage no matter what.
> 
> Bam, back to senseless non-uniform mishmash mess....

No. Why do you say that? You can add a helper function that records those 
fields in a specific order and simply doesn't add a syscall record which eats a 
lot of space.

What I am trying to say is that this needed to be fixed a long time ago. Just 
moving stuff around and adding fields arbitrarily messes up parsing and slows 
down searching big files. Any time you do this, I also have to add logic to 
conditionally support searching as stuff gets added and that makes a mess of 
user space code.

-Steve