[ghak90,(was,ghak32),V4,06/10] audit: add containerid support for tty_audit
diff mbox series

Message ID e41cb6760a8183c0955c378fce7b500819f9838f.1533065887.git.rgb@redhat.com
State New
Headers show
Series
  • audit: implement container identifier
Related show

Commit Message

Richard Guy Briggs July 31, 2018, 8:07 p.m. UTC
Add audit container identifier auxiliary record to tty logging rule
event standalone records.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
 drivers/tty/tty_audit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Paul Moore Oct. 19, 2018, 11:17 p.m. UTC | #1
On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record to tty logging rule
> event standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
>  drivers/tty/tty_audit.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> index 50f567b..3e21477 100644
> --- a/drivers/tty/tty_audit.c
> +++ b/drivers/tty/tty_audit.c
> @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
>         uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
>         uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
>         unsigned int sessionid = audit_get_sessionid(tsk);
> +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
>
> -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
>         if (ab) {
>                 char name[sizeof(tsk->comm)];
>
> @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
>                 audit_log_n_hex(ab, data, size);
>                 audit_log_end(ab);
>         }
> +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> +       audit_free_context(context);
>  }

Since I never polished up my task_struct/current fix patch enough to
get it past RFC status during this development window (new job, stolen
laptop, etc.) *and* it looks like you are going to need at least one
more respin of this patchset, go ahead and fix this patch to use
current instead of generating a local context.  I'll deal with the
merge fallout if/when it happens.

Local contexts are a last resort.  If you ever find yourself writing
code that generates a local context, you should first be 100% certain
that the event is not the the result of a process initiated action (in
which case it should take from the task's context).

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Richard Guy Briggs Oct. 31, 2018, 9:17 p.m. UTC | #2
On 2018-10-19 19:17, Paul Moore wrote:
> On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add audit container identifier auxiliary record to tty logging rule
> > event standalone records.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > ---
> >  drivers/tty/tty_audit.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > index 50f567b..3e21477 100644
> > --- a/drivers/tty/tty_audit.c
> > +++ b/drivers/tty/tty_audit.c
> > @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> >         uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
> >         uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
> >         unsigned int sessionid = audit_get_sessionid(tsk);
> > +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
> >
> > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> >         if (ab) {
> >                 char name[sizeof(tsk->comm)];
> >
> > @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> >                 audit_log_n_hex(ab, data, size);
> >                 audit_log_end(ab);
> >         }
> > +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> > +       audit_free_context(context);
> >  }
> 
> Since I never polished up my task_struct/current fix patch enough to
> get it past RFC status during this development window (new job, stolen
> laptop, etc.) *and* it looks like you are going to need at least one
> more respin of this patchset, go ahead and fix this patch to use
> current instead of generating a local context.  I'll deal with the
> merge fallout if/when it happens.

Sure, I will switch it to current in the call to audit_get_contid().

The local context is a distinct issue.  Like USER records, I prefer
local due to potential record volume, it is already trackable as far as
Steve is concerned, and if it is to be connected with the syscall
record, it should be linked to syscall records in a seperate new github
issue with its own patch.  It accumulates events until the buffer is
flushed to a record, so the timestamp only represents the flush (usually
user "CR/enter").

> Local contexts are a last resort.  If you ever find yourself writing
> code that generates a local context, you should first be 100% certain
> that the event is not the the result of a process initiated action (in
> which case it should take from the task's context).

Well, I'm 100% certain it is linked to a process, but so are USER
records that are already being discussed as the exception.  This is
basically a keystroke logger (that has a flag to omit passwords).

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Paul Moore Jan. 3, 2019, 8:11 p.m. UTC | #3
On Wed, Oct 31, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
&gt; On 2018-10-19 19:17, Paul Moore wrote:
&gt; &gt; On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs
<rgb@redhat.com> wrote:
&gt; &gt; &gt; Add audit container identifier auxiliary record to tty
logging rule
&gt; &gt; &gt; event standalone records.
&gt; &gt; &gt;
&gt; &gt; &gt; Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
&gt; &gt; &gt; Acked-by: Serge Hallyn <serge@hallyn.com>
&gt; &gt; &gt; ---
&gt; &gt; &gt;  drivers/tty/tty_audit.c | 5 ++++-
&gt; &gt; &gt;  1 file changed, 4 insertions(+), 1 deletion(-)
&gt; &gt; &gt;
&gt; &gt; &gt; diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
&gt; &gt; &gt; index 50f567b..3e21477 100644
&gt; &gt; &gt; --- a/drivers/tty/tty_audit.c
&gt; &gt; &gt; +++ b/drivers/tty/tty_audit.c
&gt; &gt; &gt; @@ -66,8 +66,9 @@ static void tty_audit_log(const char
*description, dev_t dev,
&gt; &gt; &gt;         uid_t uid = from_kuid(&amp;init_user_ns, task_uid(tsk));
&gt; &gt; &gt;         uid_t loginuid = from_kuid(&amp;init_user_ns,
audit_get_loginuid(tsk));
&gt; &gt; &gt;         unsigned int sessionid = audit_get_sessionid(tsk);
&gt; &gt; &gt; +       struct audit_context *context =
audit_alloc_local(GFP_KERNEL);
&gt; &gt; &gt;
&gt; &gt; &gt; -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
&gt; &gt; &gt; +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
&gt; &gt; &gt;         if (ab) {
&gt; &gt; &gt;                 char name[sizeof(tsk-&gt;comm)];
&gt; &gt; &gt;
&gt; &gt; &gt; @@ -80,6 +81,8 @@ static void tty_audit_log(const char
*description, dev_t dev,
&gt; &gt; &gt;                 audit_log_n_hex(ab, data, size);
&gt; &gt; &gt;                 audit_log_end(ab);
&gt; &gt; &gt;         }
&gt; &gt; &gt; +       audit_log_contid(context, "tty", audit_get_contid(tsk));
&gt; &gt; &gt; +       audit_free_context(context);
&gt; &gt; &gt;  }
&gt; &gt;
&gt; &gt; Since I never polished up my task_struct/current fix patch enough to
&gt; &gt; get it past RFC status during this development window (new job, stolen
&gt; &gt; laptop, etc.) *and* it looks like you are going to need at least one
&gt; &gt; more respin of this patchset, go ahead and fix this patch to use
&gt; &gt; current instead of generating a local context.  I'll deal with the
&gt; &gt; merge fallout if/when it happens.
&gt;
&gt; Sure, I will switch it to current in the call to audit_get_contid().
&gt;
&gt; The local context is a distinct issue.  Like USER records, I prefer
&gt; local due to potential record volume, it is already trackable as far as
&gt; Steve is concerned, and if it is to be connected with the syscall
&gt; record, it should be linked to syscall records in a seperate new github
&gt; issue with its own patch.  It accumulates events until the buffer is
&gt; flushed to a record, so the timestamp only represents the flush (usually
&gt; user "CR/enter").

Generally, yes, associating records is a separate issue, but in this
particular case you are changing this record by making it a "local"
record, which as we've discussed before, I view as a necessary evil
and something that must be minimized.  A quick look at the
tty_audit_log() callers shows tty_audit_tiocsti() which is an ioctl
handler (and thus current should be valid and correct), and
tty_audit_buf_push() whose callers all seem have valid and correct
current values; if you find that not to be the case please let me
know.

> > Local contexts are a last resort.  If you ever find yourself writing
> > code that generates a local context, you should first be 100% certain
> > that the event is not the the result of a process initiated action (in
> > which case it should take from the task's context).
>
> Well, I'm 100% certain it is linked to a process, but so are USER
> records that are already being discussed as the exception.  This is
> basically a keystroke logger (that has a flag to omit passwords).

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Richard Guy Briggs Jan. 10, 2019, 10:58 p.m. UTC | #4
On 2019-01-03 15:11, Paul Moore wrote:
> On Wed, Oct 31, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-19 19:17, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs
> <rgb@redhat.com> wrote:
> > > > Add audit container identifier auxiliary record to tty logging rule
> > > > event standalone records.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > ---
> > > >  drivers/tty/tty_audit.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > > > index 50f567b..3e21477 100644
> > > > --- a/drivers/tty/tty_audit.c
> > > > +++ b/drivers/tty/tty_audit.c
> > > > @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > >         uid_t uid = from_kuid(&amp;init_user_ns, task_uid(tsk));
> > > >         uid_t loginuid = from_kuid(&amp;init_user_ns, audit_get_loginuid(tsk));
> > > >         unsigned int sessionid = audit_get_sessionid(tsk);
> > > > +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
> > > >
> > > > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> > > >         if (ab) {
> > > >                 char name[sizeof(tsk->comm)];
> > > >
> > > > @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > >                 audit_log_n_hex(ab, data, size);
> > > >                 audit_log_end(ab);
> > > >         }
> > > > +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> > > > +       audit_free_context(context);
> > > >  }
> > >
> > > Since I never polished up my task_struct/current fix patch enough to
> > > get it past RFC status during this development window (new job, stolen
> > > laptop, etc.) *and* it looks like you are going to need at least one
> > > more respin of this patchset, go ahead and fix this patch to use
> > > current instead of generating a local context.  I'll deal with the
> > > merge fallout if/when it happens.
> >
> > Sure, I will switch it to current in the call to audit_get_contid().
> >
> > The local context is a distinct issue.  Like USER records, I prefer
> > local due to potential record volume, it is already trackable as far as
> > Steve is concerned, and if it is to be connected with the syscall
> > record, it should be linked to syscall records in a seperate new github
> > issue with its own patch.  It accumulates events until the buffer is
> > flushed to a record, so the timestamp only represents the flush (usually
> > user "CR/enter").
> 
> Generally, yes, associating records is a separate issue, but in this
> particular case you are changing this record by making it a "local"
> record, which as we've discussed before, I view as a necessary evil
> and something that must be minimized.  A quick look at the
> tty_audit_log() callers shows tty_audit_tiocsti() which is an ioctl
> handler (and thus current should be valid and correct), and
> tty_audit_buf_push() whose callers all seem have valid and correct
> current values; if you find that not to be the case please let me
> know.

Unless I'm misunderstanding what "local" means, it already had a local
context by virtue of having a NULL context since it was never previously
connected to syscall events, so changing it to a local context doesn't
change that other than making it possible to associate an auxiliary
audit container identifier record.

The reasoning I'm also applying here is that the contents of this record
don't all come from this one syscall, but most likely came in from an
entire line of individual keystrokes, so the syscall information is only
from the last one of those syscalls, though that syscall information
other than the timestamp should be the same.

Reading your reply above it isn't clear to me that I had made these two
points clear previously.  If you still think this record should be
associated to a syscall despite my reasoning above, I'm willing to
connect it, but will do so in a seperate issue/patch.

> > > Local contexts are a last resort.  If you ever find yourself writing
> > > code that generates a local context, you should first be 100% certain
> > > that the event is not the the result of a process initiated action (in
> > > which case it should take from the task's context).
> >
> > Well, I'm 100% certain it is linked to a process, but so are USER
> > records that are already being discussed as the exception.  This is
> > basically a keystroke logger (that has a flag to omit passwords).
> 
> --
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Paul Moore Jan. 11, 2019, 1:12 a.m. UTC | #5
On Thu, Jan 10, 2019 at 5:59 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-01-03 15:11, Paul Moore wrote:
> > On Wed, Oct 31, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-10-19 19:17, Paul Moore wrote:
> > > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs
> > <rgb@redhat.com> wrote:
> > > > > Add audit container identifier auxiliary record to tty logging rule
> > > > > event standalone records.
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > > ---
> > > > >  drivers/tty/tty_audit.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > > > > index 50f567b..3e21477 100644
> > > > > --- a/drivers/tty/tty_audit.c
> > > > > +++ b/drivers/tty/tty_audit.c
> > > > > @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > >         uid_t uid = from_kuid(&amp;init_user_ns, task_uid(tsk));
> > > > >         uid_t loginuid = from_kuid(&amp;init_user_ns, audit_get_loginuid(tsk));
> > > > >         unsigned int sessionid = audit_get_sessionid(tsk);
> > > > > +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
> > > > >
> > > > > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> > > > >         if (ab) {
> > > > >                 char name[sizeof(tsk->comm)];
> > > > >
> > > > > @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > >                 audit_log_n_hex(ab, data, size);
> > > > >                 audit_log_end(ab);
> > > > >         }
> > > > > +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> > > > > +       audit_free_context(context);
> > > > >  }
> > > >
> > > > Since I never polished up my task_struct/current fix patch enough to
> > > > get it past RFC status during this development window (new job, stolen
> > > > laptop, etc.) *and* it looks like you are going to need at least one
> > > > more respin of this patchset, go ahead and fix this patch to use
> > > > current instead of generating a local context.  I'll deal with the
> > > > merge fallout if/when it happens.
> > >
> > > Sure, I will switch it to current in the call to audit_get_contid().
> > >
> > > The local context is a distinct issue.  Like USER records, I prefer
> > > local due to potential record volume, it is already trackable as far as
> > > Steve is concerned, and if it is to be connected with the syscall
> > > record, it should be linked to syscall records in a seperate new github
> > > issue with its own patch.  It accumulates events until the buffer is
> > > flushed to a record, so the timestamp only represents the flush (usually
> > > user "CR/enter").
> >
> > Generally, yes, associating records is a separate issue, but in this
> > particular case you are changing this record by making it a "local"
> > record, which as we've discussed before, I view as a necessary evil
> > and something that must be minimized.  A quick look at the
> > tty_audit_log() callers shows tty_audit_tiocsti() which is an ioctl
> > handler (and thus current should be valid and correct), and
> > tty_audit_buf_push() whose callers all seem have valid and correct
> > current values; if you find that not to be the case please let me
> > know.
>
> Unless I'm misunderstanding what "local" means, it already had a local
> context by virtue of having a NULL context since it was never previously
> connected to syscall events, so changing it to a local context doesn't
> change that other than making it possible to associate an auxiliary
> audit container identifier record.
>
> The reasoning I'm also applying here is that the contents of this record
> don't all come from this one syscall, but most likely came in from an
> entire line of individual keystrokes, so the syscall information is only
> from the last one of those syscalls, though that syscall information
> other than the timestamp should be the same.

Looking at the callers to tty_audit_log(), I think we can all agree
that in the tty_audit_tiocsti() case it is correct to associate the
tty record with current, as it is the current task which sent the
ioctl with the data.  Do you not agree?

With tty_audit_buf_push() we need to do a bit more work to track down
all the callers.  Looking quickly it appears that all of the
tty_audit_add_data() callers are copying data to/from userspace, so
associating these tty records with their syscall would seem
appropriate.  With tty_audit_push() it either appears to be
tty_audit_tiocsti() (again) or more userspace copy routines.  I didn't
bother looking at tty_audit_exit() because that seemed pretty clear to
be something worth associating with a syscall.  You may find that if
you dig deeper into the call stacks things fall apart and there are
cases where the records shouldn't be associated with the current
syscall, but based on what I'm seeing right now that doesn't appear to
be the case.

> Reading your reply above it isn't clear to me that I had made these two
> points clear previously.  If you still think this record should be
> associated to a syscall despite my reasoning above, I'm willing to
> connect it, but will do so in a seperate issue/patch.
Richard Guy Briggs Jan. 11, 2019, 3:38 a.m. UTC | #6
On 2019-01-10 20:12, Paul Moore wrote:
> On Thu, Jan 10, 2019 at 5:59 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-01-03 15:11, Paul Moore wrote:
> > > On Wed, Oct 31, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2018-10-19 19:17, Paul Moore wrote:
> > > > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs
> > > <rgb@redhat.com> wrote:
> > > > > > Add audit container identifier auxiliary record to tty logging rule
> > > > > > event standalone records.
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > > > ---
> > > > > >  drivers/tty/tty_audit.c | 5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > > > > > index 50f567b..3e21477 100644
> > > > > > --- a/drivers/tty/tty_audit.c
> > > > > > +++ b/drivers/tty/tty_audit.c
> > > > > > @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > > >         uid_t uid = from_kuid(&amp;init_user_ns, task_uid(tsk));
> > > > > >         uid_t loginuid = from_kuid(&amp;init_user_ns, audit_get_loginuid(tsk));
> > > > > >         unsigned int sessionid = audit_get_sessionid(tsk);
> > > > > > +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
> > > > > >
> > > > > > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> > > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> > > > > >         if (ab) {
> > > > > >                 char name[sizeof(tsk->comm)];
> > > > > >
> > > > > > @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > > >                 audit_log_n_hex(ab, data, size);
> > > > > >                 audit_log_end(ab);
> > > > > >         }
> > > > > > +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> > > > > > +       audit_free_context(context);
> > > > > >  }
> > > > >
> > > > > Since I never polished up my task_struct/current fix patch enough to
> > > > > get it past RFC status during this development window (new job, stolen
> > > > > laptop, etc.) *and* it looks like you are going to need at least one
> > > > > more respin of this patchset, go ahead and fix this patch to use
> > > > > current instead of generating a local context.  I'll deal with the
> > > > > merge fallout if/when it happens.
> > > >
> > > > Sure, I will switch it to current in the call to audit_get_contid().
> > > >
> > > > The local context is a distinct issue.  Like USER records, I prefer
> > > > local due to potential record volume, it is already trackable as far as
> > > > Steve is concerned, and if it is to be connected with the syscall
> > > > record, it should be linked to syscall records in a seperate new github
> > > > issue with its own patch.  It accumulates events until the buffer is
> > > > flushed to a record, so the timestamp only represents the flush (usually
> > > > user "CR/enter").
> > >
> > > Generally, yes, associating records is a separate issue, but in this
> > > particular case you are changing this record by making it a "local"
> > > record, which as we've discussed before, I view as a necessary evil
> > > and something that must be minimized.  A quick look at the
> > > tty_audit_log() callers shows tty_audit_tiocsti() which is an ioctl
> > > handler (and thus current should be valid and correct), and
> > > tty_audit_buf_push() whose callers all seem have valid and correct
> > > current values; if you find that not to be the case please let me
> > > know.
> >
> > Unless I'm misunderstanding what "local" means, it already had a local
> > context by virtue of having a NULL context since it was never previously
> > connected to syscall events, so changing it to a local context doesn't
> > change that other than making it possible to associate an auxiliary
> > audit container identifier record.
> >
> > The reasoning I'm also applying here is that the contents of this record
> > don't all come from this one syscall, but most likely came in from an
> > entire line of individual keystrokes, so the syscall information is only
> > from the last one of those syscalls, though that syscall information
> > other than the timestamp should be the same.
> 
> Looking at the callers to tty_audit_log(), I think we can all agree
> that in the tty_audit_tiocsti() case it is correct to associate the
> tty record with current, as it is the current task which sent the
> ioctl with the data.  Do you not agree?

I'm fine with that, yes.

> With tty_audit_buf_push() we need to do a bit more work to track down
> all the callers.  Looking quickly it appears that all of the
> tty_audit_add_data() callers are copying data to/from userspace, so
> associating these tty records with their syscall would seem
> appropriate.  With tty_audit_push() it either appears to be
> tty_audit_tiocsti() (again) or more userspace copy routines.  I didn't
> bother looking at tty_audit_exit() because that seemed pretty clear to
> be something worth associating with a syscall.  You may find that if
> you dig deeper into the call stacks things fall apart and there are
> cases where the records shouldn't be associated with the current
> syscall, but based on what I'm seeing right now that doesn't appear to
> be the case.

Ok.  Except that it might be multiple subsequent calls that assemble
that data, only flushed to an audit record on exceeding N_TTY_BUF_SIZE
(4k) or that task switching device and flushing the previous device or
that task switching icanon mode.

> > Reading your reply above it isn't clear to me that I had made these two
> > points clear previously.  If you still think this record should be
> > associated to a syscall despite my reasoning above, I'm willing to
> > connect it, but will do so in a seperate issue/patch.

So can I conclude that you are fine with the buffering of the data from
multiple syscalls being associated with the output of that buffer to one
audit record with the last of those syscalls?

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Paul Moore Jan. 11, 2019, 11:16 p.m. UTC | #7
On Thu, Jan 10, 2019 at 10:39 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-01-10 20:12, Paul Moore wrote:
> > On Thu, Jan 10, 2019 at 5:59 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2019-01-03 15:11, Paul Moore wrote:
> > > > On Wed, Oct 31, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > On 2018-10-19 19:17, Paul Moore wrote:
> > > > > > On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs
> > > > <rgb@redhat.com> wrote:
> > > > > > > Add audit container identifier auxiliary record to tty logging rule
> > > > > > > event standalone records.
> > > > > > >
> > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > > > > ---
> > > > > > >  drivers/tty/tty_audit.c | 5 ++++-
> > > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > > > > > > index 50f567b..3e21477 100644
> > > > > > > --- a/drivers/tty/tty_audit.c
> > > > > > > +++ b/drivers/tty/tty_audit.c
> > > > > > > @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > > > >         uid_t uid = from_kuid(&amp;init_user_ns, task_uid(tsk));
> > > > > > >         uid_t loginuid = from_kuid(&amp;init_user_ns, audit_get_loginuid(tsk));
> > > > > > >         unsigned int sessionid = audit_get_sessionid(tsk);
> > > > > > > +       struct audit_context *context = audit_alloc_local(GFP_KERNEL);
> > > > > > >
> > > > > > > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> > > > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> > > > > > >         if (ab) {
> > > > > > >                 char name[sizeof(tsk->comm)];
> > > > > > >
> > > > > > > @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> > > > > > >                 audit_log_n_hex(ab, data, size);
> > > > > > >                 audit_log_end(ab);
> > > > > > >         }
> > > > > > > +       audit_log_contid(context, "tty", audit_get_contid(tsk));
> > > > > > > +       audit_free_context(context);
> > > > > > >  }
> > > > > >
> > > > > > Since I never polished up my task_struct/current fix patch enough to
> > > > > > get it past RFC status during this development window (new job, stolen
> > > > > > laptop, etc.) *and* it looks like you are going to need at least one
> > > > > > more respin of this patchset, go ahead and fix this patch to use
> > > > > > current instead of generating a local context.  I'll deal with the
> > > > > > merge fallout if/when it happens.
> > > > >
> > > > > Sure, I will switch it to current in the call to audit_get_contid().
> > > > >
> > > > > The local context is a distinct issue.  Like USER records, I prefer
> > > > > local due to potential record volume, it is already trackable as far as
> > > > > Steve is concerned, and if it is to be connected with the syscall
> > > > > record, it should be linked to syscall records in a seperate new github
> > > > > issue with its own patch.  It accumulates events until the buffer is
> > > > > flushed to a record, so the timestamp only represents the flush (usually
> > > > > user "CR/enter").
> > > >
> > > > Generally, yes, associating records is a separate issue, but in this
> > > > particular case you are changing this record by making it a "local"
> > > > record, which as we've discussed before, I view as a necessary evil
> > > > and something that must be minimized.  A quick look at the
> > > > tty_audit_log() callers shows tty_audit_tiocsti() which is an ioctl
> > > > handler (and thus current should be valid and correct), and
> > > > tty_audit_buf_push() whose callers all seem have valid and correct
> > > > current values; if you find that not to be the case please let me
> > > > know.
> > >
> > > Unless I'm misunderstanding what "local" means, it already had a local
> > > context by virtue of having a NULL context since it was never previously
> > > connected to syscall events, so changing it to a local context doesn't
> > > change that other than making it possible to associate an auxiliary
> > > audit container identifier record.
> > >
> > > The reasoning I'm also applying here is that the contents of this record
> > > don't all come from this one syscall, but most likely came in from an
> > > entire line of individual keystrokes, so the syscall information is only
> > > from the last one of those syscalls, though that syscall information
> > > other than the timestamp should be the same.
> >
> > Looking at the callers to tty_audit_log(), I think we can all agree
> > that in the tty_audit_tiocsti() case it is correct to associate the
> > tty record with current, as it is the current task which sent the
> > ioctl with the data.  Do you not agree?
>
> I'm fine with that, yes.
>
> > With tty_audit_buf_push() we need to do a bit more work to track down
> > all the callers.  Looking quickly it appears that all of the
> > tty_audit_add_data() callers are copying data to/from userspace, so
> > associating these tty records with their syscall would seem
> > appropriate.  With tty_audit_push() it either appears to be
> > tty_audit_tiocsti() (again) or more userspace copy routines.  I didn't
> > bother looking at tty_audit_exit() because that seemed pretty clear to
> > be something worth associating with a syscall.  You may find that if
> > you dig deeper into the call stacks things fall apart and there are
> > cases where the records shouldn't be associated with the current
> > syscall, but based on what I'm seeing right now that doesn't appear to
> > be the case.
>
> Ok.  Except that it might be multiple subsequent calls that assemble
> that data, only flushed to an audit record on exceeding N_TTY_BUF_SIZE
> (4k) or that task switching device and flushing the previous device or
> that task switching icanon mode.

It seems just as reasonable to associate it with that final triggering
syscall just as much as it seems reasonable to log the buffered tty
data via the audit subsystem.  If for some reason we need to
dissociate the tty data from the syscall data I don't see why we can't
make that change in the future.

> > > Reading your reply above it isn't clear to me that I had made these two
> > > points clear previously.  If you still think this record should be
> > > associated to a syscall despite my reasoning above, I'm willing to
> > > connect it, but will do so in a seperate issue/patch.
>
> So can I conclude that you are fine with the buffering of the data from
> multiple syscalls being associated with the output of that buffer to one
> audit record with the last of those syscalls?

Patch
diff mbox series

diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 50f567b..3e21477 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -66,8 +66,9 @@  static void tty_audit_log(const char *description, dev_t dev,
 	uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
 	uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
 	unsigned int sessionid = audit_get_sessionid(tsk);
+	struct audit_context *context = audit_alloc_local(GFP_KERNEL);
 
-	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
 	if (ab) {
 		char name[sizeof(tsk->comm)];
 
@@ -80,6 +81,8 @@  static void tty_audit_log(const char *description, dev_t dev,
 		audit_log_n_hex(ab, data, size);
 		audit_log_end(ab);
 	}
+	audit_log_contid(context, "tty", audit_get_contid(tsk));
+	audit_free_context(context);
 }
 
 /**