From: Miloslav Trmac <[email protected]>
Add TTY input auditing, used to audit system administrator's actions.
TTY input auditing works on a higher level than auditing all system
calls within the session, which would produce an overwhelming amount of
mostly useless audit events.
Add an "audit_tty" attribute, inherited across fork (). Data read from
TTYs by process with the attribute is sent to the audit subsystem by the
kernel. The audit netlink interface is extended to allow modifying the
audit_tty attribute, and to allow sending explanatory audit events from
user-space (for example, a shell might send an event containing the
final command, after the interactive command-line editing and history
expansion is performed, which might be difficult to decipher from the
TTY input alone).
Because the "audit_tty" attribute is inherited across fork (), it would
be set e.g. for sshd restarted within an audited session. To prevent
this, the audit_tty attribute is cleared when a process with no open TTY
file descriptors (e.g. after daemon startup) opens a TTY.
See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.
I'm sorry, I forgot the patch...
On Wed, 06 Jun 2007 12:10:28 +0200 Miloslav Trmac <[email protected]> wrote:
> From: Miloslav Trmac <[email protected]>
>
> Add TTY input auditing, used to audit system administrator's actions.
> TTY input auditing works on a higher level than auditing all system
> calls within the session, which would produce an overwhelming amount of
> mostly useless audit events.
>
> Add an "audit_tty" attribute, inherited across fork (). Data read from
> TTYs by process with the attribute is sent to the audit subsystem by the
> kernel. The audit netlink interface is extended to allow modifying the
> audit_tty attribute, and to allow sending explanatory audit events from
> user-space (for example, a shell might send an event containing the
> final command, after the interactive command-line editing and history
> expansion is performed, which might be difficult to decipher from the
> TTY input alone).
>
> Because the "audit_tty" attribute is inherited across fork (), it would
> be set e.g. for sshd restarted within an audited session. To prevent
> this, the audit_tty attribute is cleared when a process with no open TTY
> file descriptors (e.g. after daemon startup) opens a TTY.
>
> See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
> for a more detailed rationale document for an older version of this patch.
>
> ...
>
> +static void
> +tty_audit_buf_free(struct tty_audit_buf *buf)
> +{
The usual kernel style is
static void tty_audit_buf_free(struct tty_audit_buf *buf)
{
and the style which you've used here is usually only employed if its use
prevents an 80-column overflow.
There are plenty of exceptions to this, and I understand (and actually
agree with) the reason for the style which you've chosen, but
standardisation wins out.
The patch adds a lot of new code to n_tty.c, I suspect it would be neater
to put it all into a new file if possible?
> +/**
> + * tty_audit_exit - Handle a task exit
> + *
> + * Make sure all buffered data is written out and deallocate the buffer.
> + * Only needs to be called if current->signal->tty_audit_buf != %NULL.
> + */
> +void
> +tty_audit_exit(void)
> +{
> + struct tty_audit_buf *buf;
> +
> + spin_lock(¤t->sighand->siglock);
I think you have a bug here. ->siglock is taken elsewhere in an irq-safe
fashion (multiple instances)
> +/**
> + * tty_audit_add_data - Add data for TTY auditing.
> + *
> + * Audit @data of @size from @tty, if necessary.
> + */
> +static void
> +tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
> +{
> + struct tty_audit_buf *buf;
> + int major, minor;
> +
> + if (unlikely(size == 0))
> + return;
> +
> + buf = tty_audit_buf_get(tty);
> + if (!buf)
> + return;
> +
> + mutex_lock(&buf->mutex);
> + major = tty->driver->major;
> + minor = tty->driver->minor_start + tty->index;
> + if (buf->major != major || buf->minor != minor
> + || buf->icanon != tty->icanon) {
> + tty_audit_buf_push_current(buf);
> + buf->major = major;
> + buf->minor = minor;
> + buf->icanon = tty->icanon;
> + }
> + do {
> + size_t run;
> +
> + run = N_TTY_BUF_SIZE - buf->valid;
> + if (run > size)
> + run = size;
> + memcpy(buf->data + buf->valid, data, run);
> + buf->valid += run;
> + data += run;
> + size -= run;
> + if (buf->valid == N_TTY_BUF_SIZE)
> + tty_audit_buf_push_current(buf);
> + } while (size != 0);
the whitespace went bad here.
> + mutex_unlock(&buf->mutex);
> + tty_audit_buf_put(buf);
> +}
> +
>
> ...
>
> +
> +/* For checking whether a file is a TTY */
> +extern ssize_t tty_read(struct file * file, char __user * buf, size_t count,
> + loff_t *ppos);
Nope, please don't add extern declarations to C files. Do it via header
files.
> +/**
> + * tty_audit_opening - A TTY is being opened.
> + *
> + * As a special hack, tasks that close all their TTYs and open new ones
> + * are assumed to be system daemons (e.g. getty) and auditing is
> + * automatically disabled for them.
> + */
> +void
> +tty_audit_opening(void)
> +{
> + int disable;
> +
> + disable = 1;
> + spin_lock(¤t->sighand->siglock);
> + if (current->signal->audit_tty == 0)
> + disable = 0;
> + spin_unlock(¤t->sighand->siglock);
> + if (!disable)
> + return;
> +
> + task_lock(current);
> + if (current->files) {
> + struct fdtable *fdt;
> + unsigned i;
> +
> + /*
> + * We don't take a ref to the file, so we must hold ->file_lock
> + * instead.
> + */
> + spin_lock(¤t->files->file_lock);
So we make file_lock nest inside task_lock(). Was that lock ranking
already being used elsewhere in the kernel, or is it a new association?
Has this code had full coverage testing with all lockdep features enabled?
(I suspect not - lockdep should have gone wild over the siglock thing)
> + fdt = files_fdtable(current->files);
> + for (i = 0; i < fdt->max_fds; i++) {
> + struct file *filp;
> +
> + filp = fcheck_files(current->files, i);
> + if (!filp)
> + continue;
> + if (filp->f_op->read == tty_read) {
> + disable = 0;
> + break;
> + }
> + }
> + spin_unlock(¤t->files->file_lock);
> + }
> + task_unlock(current);
> + if (!disable)
> + return;
> +
> + spin_lock(¤t->sighand->siglock);
> + current->signal->audit_tty = 0;
> + spin_unlock(¤t->sighand->siglock);
> +}
> +#else
> +inline static void
> +tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
> +{
> +}
Please use `static inline' rather that `inline static'. Just a consistency
thing.
> +static void
> +tty_audit_push(struct tty_struct *tty)
> +{
> +}
Probably you meant `static inline' here too.
> +#endif
> +
> +static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
> + unsigned char __user *ptr)
> +{
> + tty_audit_add_data(tty, &x, 1);
> + return put_user(x, ptr);
> +}
> +
>
> ...
>
> @@ -487,6 +496,7 @@ extern int audit_signals;
> #define audit_mq_notify(d,n) ({ 0; })
> #define audit_mq_getsetattr(d,s) ({ 0; })
> #define audit_ptrace(t) ((void)0)
> +#define audit_enabled 0
> #define audit_n_rules 0
> #define audit_signals 0
> #endif
> @@ -515,6 +525,7 @@ extern void audit_log_d_path(struct audit_buffer *ab,
> const char *prefix,
> struct dentry *dentry,
> struct vfsmount *vfsmnt);
> +extern void audit_log_lost(const char *message);
> /* Private API (for audit.c only) */
> extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
> extern int audit_filter_type(int type);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d58e74b..3ae4904 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -506,6 +506,8 @@ struct signal_struct {
> #ifdef CONFIG_TASKSTATS
> struct taskstats *stats;
> #endif
> + unsigned audit_tty:1;
> + struct tty_audit_buf *tty_audit_buf;
> };
hm, bitfields are risky. If someone adds another one, it will land in
the same word and external locking will be needed. You do seem to be using
->siglock to cover this - was that to address the bitfield non-atomicity
problem?
A suitable (but somewhat less pretty) way to resolve all this is to not use
bitfields at all: add `unsigned long flags' and use set_bit/clear_bit/etc.
> extern int n_tty_ioctl(struct tty_struct * tty, struct file * file,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index d13276d..a071a96 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -58,6 +58,7 @@
> #include <linux/selinux.h>
> #include <linux/inotify.h>
> #include <linux/freezer.h>
> +#include <linux/tty.h>
>
> #include "audit.h"
>
> @@ -423,6 +424,32 @@ static int kauditd_thread(void *dummy)
> return 0;
> }
>
> +static int
> +audit_prepare_user_tty(pid_t pid, uid_t loginuid)
> +{
> + struct task_struct *tsk;
> + int err;
> +
> + read_lock(&tasklist_lock);
> + tsk = find_task_by_pid(pid);
> + err = -ESRCH;
> + if (!tsk)
> + goto out;
> + err = 0;
> +
> + spin_lock(&tsk->sighand->siglock);
> + if (!tsk->signal->audit_tty)
> + err = -EPERM;
> + spin_unlock(&tsk->sighand->siglock);
So siglock nests inside tasklist_lock? Sounds reasonable. Is this a
preexisting association, or did this patch just create it?
> + if (err)
> + goto out;
> +
> + tty_audit_push_task(tsk, loginuid);
> +out:
> + read_unlock(&tasklist_lock);
> + return err;
> +}
> +
>
> ...
>
> break;
> + case AUDIT_TTY_GET: {
> + struct audit_tty_status s;
> + struct task_struct *tsk;
> +
> + read_lock(&tasklist_lock);
> + tsk = find_task_by_pid(pid);
> + if (!tsk)
> + err = -ESRCH;
> + else {
> + spin_lock(&tsk->sighand->siglock);
> + s.enabled = tsk->signal->audit_tty != 0;
> + spin_unlock(&tsk->sighand->siglock);
The locking here looks dubious. tsk->signal->audit_tty can change state
the instant ->siglock gets unlocked, in which case s.enabled is now wrong.
If that is acceptable then we didn't need that locking at all.
On Jun 6 2007 11:49, Miloslav Trmac wrote:
>From: Miloslav Trmac <[email protected]>
>
>Add TTY input auditing, used to audit system administrator's actions.
_What_ exactly does it audit?
And why does it only audit sysadmin actions?
Is this supposed to be a keylogger?
>TTY input auditing works on a higher level than auditing all system
What about tty output?
>calls within the session, which would produce an overwhelming amount of
>mostly useless audit events.
>
Jan
--
> > + if (filp->f_op->read == tty_read) {
> > + disable = 0;
> > + break;
Why says a tty will always have f->op->read == tty_read ?
On Thursday 07 June 2007 04:13:42 Jan Engelhardt wrote:
> >Add TTY input auditing, used to audit system administrator's actions.
>
> _What_ exactly does it audit?
In theory, it should audit the actions performed by the sysadmin. This patch
doesn't cover actions done via X windows interface.
> And why does it only audit sysadmin actions?
This is what's called out for in various security standards such as NISPOM,
DCID 6/3, and PCI. Its all about non-repudiation.
> Is this supposed to be a keylogger?
Sort of. Its supposed to allow the Security Officer the ability to check on
what an admin was doing if they were found to be attempting access to
information outside their duties. Or if something is found to be
misconfigured, they can backtrack and see how it became that way if that was
important.
Some people try to be compliant with these security standards with user space
tools like rootsh, but that is too easy to detect and defeat. And then it
does not put its data into the audit system where its correlated with other
system events.
>What about tty output?
That is not required.
-Steve
Alan Cox napsal(a):
>>> + if (filp->f_op->read == tty_read) {
>>> + disable = 0;
>>> + break;
> Why says a tty will always have f->op->read == tty_read ?
AFAICS from tty_io.c, it will always be tty_read or hung_up_tty_read.
Normal user processes would exit after SIGHUP and not reopen a TTY.
(I have copied the condition from __do_SAK(). That of course doesn't
mean it's correct.)
Mirek
--- Steve Grubb <[email protected]> wrote:
> On Thursday 07 June 2007 04:13:42 Jan Engelhardt wrote:
> > >Add TTY input auditing, used to audit system administrator's actions.
> >
> > _What_ exactly does it audit?
>
> In theory, it should audit the actions performed by the sysadmin. This patch
> doesn't cover actions done via X windows interface.
>
> > And why does it only audit sysadmin actions?
>
> This is what's called out for in various security standards such as NISPOM,
> DCID 6/3, and PCI. Its all about non-repudiation.
>
> > Is this supposed to be a keylogger?
>
> Sort of. Its supposed to allow the Security Officer the ability to check on
> what an admin was doing if they were found to be attempting access to
> information outside their duties. Or if something is found to be
> misconfigured, they can backtrack and see how it became that way if that was
> important.
>
> Some people try to be compliant with these security standards with user space
>
> tools like rootsh, but that is too easy to detect and defeat. And then it
> does not put its data into the audit system where its correlated with other
> system events.
The evaluation teams that I have worked with (OrangeBook and CC)
as well as their technical advisory groups have always been clear
that keyboard logging is not an appropriate mechanism for security
audit logging. There is insufficient correlation between what is
typed and security relevent actions for keyboard (or mouse event)
logging to meet the audit requirements. You have to log what happened.
Logging what was requested is insufficient and logging what was
typed, which may or may not have resulted in an actual request is
not helpful to meeting security audit requirements.
> >What about tty output?
>
> That is not required.
>
> -Steve
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
Casey Schaufler
[email protected]
> logging to meet the audit requirements. You have to log what happened.
> Logging what was requested is insufficient and logging what was
> typed, which may or may not have resulted in an actual request is
> not helpful to meeting security audit requirements.
Key information can answer some questions as an additional adjunct, but
all of this really depends whether your security audit requirement is to
impress the monkeys or to protect your systems (or as usually is the case
to protect your system while still impressing a bunch of half-trained 19
year old box tickers on their first outing doing auditing)
On Thursday 07 June 2007 11:42, Casey Schaufler wrote:
> > tools like rootsh, but that is too easy to detect and defeat. And then it
> > does not put its data into the audit system where its correlated with
> > other system events.
>
> The evaluation teams that I have worked with (OrangeBook and CC)
> as well as their technical advisory groups have always been clear
> that keyboard logging is not an appropriate mechanism for security
> audit logging. There is insufficient correlation between what is
> typed and security relevent actions for keyboard (or mouse event)
> logging to meet the audit requirements.
Ok, this is a sample set of requirements we are trying to meet:
Implement automated audit trails for all system components to reconstruct the
following events:
All actions taken by any individual with root or administrative privileges
If we do not get commands typed at a prompt, we have to audit by execve.
Auditing execve for uid = 0 produces millions of events since that includes
daemons. If we get clever and restrict auditing to events for root uid and
auid >= 500, we still wind up with millions of events because of shell
scripts.
People in control of some of these security targets said to me that auditing
by execve cannot be the solution, because the audit trail becomes too big,
unwieldy, full of irrelavent events, and not easily analysed. So, that
approach does not work for people either.
Casey, so what approach would you take to meet the requirement?
> You have to log what happened.
Which can be done by auditing for execution of specific apps or watching
access to certain files. So, I don't see tty auditing as something that
replaces other auditing, it allows us to form a better picture of what
happened to the system.
> Logging what was requested is insufficient and logging what was
> typed, which may or may not have resulted in an actual request is
> not helpful to meeting security audit requirements.
I would disagree. Its helpful to complete the picture of what's happening on
the system.
-Steve
--- Steve Grubb <[email protected]> wrote:
> Ok, this is a sample set of requirements we are trying to meet:
>
> Implement automated audit trails for all system components to reconstruct the
>
> following events:
> All actions taken by any individual with root or administrative privileges
My knee jerk response is "good luck with that". I hope you're not
planning a GUI, because tracing the mouse movements, clicks, and
window placement in order to identify what action the admin is taking
is going to be a without-a-net trick.
> If we do not get commands typed at a prompt, we have to audit by execve.
I would suggest that you'll have to do that as well so that you can tell
the difference between typed actions like these:
# cat > /dev/null
badprogram --badthing --everyone
^D
#
# badprogram --badthing --everyone
where the same typed line is a Bad Thing in one case and completely
irrelevent in the other.
> Auditing execve for uid = 0 produces millions of events since that includes
> daemons. If we get clever and restrict auditing to events for root uid and
> auid >= 500, we still wind up with millions of events because of shell
> scripts.
Yes. Your Linux system performs millions (yea, billions) of security
relevent actions in the time it takes you (well, me) to spell check an
lkml posting. Audit systems spew lots of data because there's lots of
data that is required to track who did what to whom.
> People in control of some of these security targets said to me that auditing
> by execve cannot be the solution, because the audit trail becomes too big,
> unwieldy, full of irrelavent events, and not easily analysed. So, that
> approach does not work for people either.
OK. They're not willing to pay the price for the result they are asking for.
> Casey, so what approach would you take to meet the requirement?
If the requirement is that all human initiated administrative actions
be audited your only option is to seriously restrict the actions that
the human can perform by hand. This is usually accomplished by providing
an administration user interface facility as the admin shell. This shell
only invokes a tightly controlled set of commands, and audits their
parameters.
> > You have to log what happened.
>
> Which can be done by auditing for execution of specific apps or watching
> access to certain files. So, I don't see tty auditing as something that
> replaces other auditing, it allows us to form a better picture of what
> happened to the system.
Perhaps.
> > Logging what was requested is insufficient and logging what was
> > typed, which may or may not have resulted in an actual request is
> > not helpful to meeting security audit requirements.
>
> I would disagree. Its helpful to complete the picture of what's happening on
> the system.
As an adjunct to system level audit it might be helpful, but
it cannot stand on its own.
Casey Schaufler
[email protected]
Casey Schaufler napsal(a):
>> If we do not get commands typed at a prompt, we have to audit by execve.
> I would suggest that you'll have to do that as well so that you can tell
> the difference between typed actions like these:
>
> # cat > /dev/null
> badprogram --badthing --everyone
> ^D
> #
>
> # badprogram --badthing --everyone
>
> where the same typed line is a Bad Thing in one case and completely
> irrelevent in the other.
The proposed patch audits each process separately, and includes a part
of the command name in the audit event, so it is easy to distinguish
between data entered into (cat > /dev/null) and the shell.
The command name can be faked, but the actions necessary to fake the
command name would be audited.
Mirek
On Jun 7 2007 21:28, Miloslav Trmac wrote:
>Casey Schaufler napsal(a):
>>> If we do not get commands typed at a prompt, we have to audit by execve.
>> I would suggest that you'll have to do that as well so that you can tell
>> the difference between typed actions like these:
>>
>> # cat > /dev/null
>> badprogram --badthing --everyone
>> ^D
>> #
>>
>> # badprogram --badthing --everyone
>>
>> where the same typed line is a Bad Thing in one case and completely
>> irrelevent in the other.
>The proposed patch audits each process separately, and includes a part
>of the command name in the audit event, so it is easy to distinguish
>between data entered into (cat > /dev/null) and the shell.
>
>The command name can be faked, but the actions necessary to fake the
>command name would be audited.
Someone please enlighten me why a regular keylogger² that captures
both input and output could not do the same. (Auditing what one has done.)
² http://ttyrpld.sf.net (there are others too)
Jan
--
On Thu, 07 Jun 2007 16:20:07 +0200
Miloslav Trmac <[email protected]> wrote:
> Alan Cox napsal(a):
> >>> + if (filp->f_op->read == tty_read) {
> >>> + disable = 0;
> >>> + break;
> > Why says a tty will always have f->op->read == tty_read ?
> AFAICS from tty_io.c, it will always be tty_read or hung_up_tty_read.
> Normal user processes would exit after SIGHUP and not reopen a TTY.
>
> (I have copied the condition from __do_SAK(). That of course doesn't
> mean it's correct.)
> Mirek
Right it may be hung_up_tty_read that was what bothered me. I've had a
think through the different scenarios and I can't think of a simple one
where I can abuse this as the vhangup() path is current root triggered
and loses the tty (so I can't reopen on it)
There are more complex questions - what happens when the much needed
revoke() goes mainstream [and we fix all the security issues its lack
causes], and the case where I do
login on tty1
login on tty2
On tty1 run a process which sets nohup and causes a vhangup then opens
tty2 while tty2 command line is running some long running program that
doesn't take input that I could plausibly run legitimately (eg a long
complex sql query, or a slow security check etc)
Alan
--- Jan Engelhardt <[email protected]> wrote:
> Someone please enlighten me why a regular keylogger? that captures
> both input and output could not do the same. (Auditing what one has done.)
1. shell aliases
# innocuous -p 0
2. shell variables
# $INNOCUOUS -p 0
3. symlinks
# ./innocuous -p 0
Yes, I know there are ways to prevent each of these "attacks",
but it's surprising how often simple textual changes are effective
in hiding behavior.
Casey Schaufler
[email protected]
Thanks for the review.
Andrew Morton napsal(a):
> On Wed, 06 Jun 2007 12:10:28 +0200 Miloslav Trmac <[email protected]> wrote:
>> +/**
>> + * tty_audit_opening - A TTY is being opened.
>> + *
>> + * As a special hack, tasks that close all their TTYs and open new ones
>> + * are assumed to be system daemons (e.g. getty) and auditing is
>> + * automatically disabled for them.
>> + */
>> +void
>> +tty_audit_opening(void)
>> +{
>> + int disable;
>> +
>> + disable = 1;
>> + spin_lock(¤t->sighand->siglock);
>> + if (current->signal->audit_tty == 0)
>> + disable = 0;
>> + spin_unlock(¤t->sighand->siglock);
>> + if (!disable)
>> + return;
>> +
>> + task_lock(current);
>> + if (current->files) {
>> + struct fdtable *fdt;
>> + unsigned i;
>> +
>> + /*
>> + * We don't take a ref to the file, so we must hold ->file_lock
>> + * instead.
>> + */
>> + spin_lock(¤t->files->file_lock);
>
> So we make file_lock nest inside task_lock(). Was that lock ranking
> already being used elsewhere in the kernel, or is it a new association?
It is used in __do_SAK ().
> Has this code had full coverage testing with all lockdep features enabled?
>
> (I suspect not - lockdep should have gone wild over the siglock thing)
It was not. The new version will be.
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index d13276d..a071a96 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -423,6 +424,32 @@ static int kauditd_thread(void *dummy)
>> return 0;
>> }
>>
>> +static int
>> +audit_prepare_user_tty(pid_t pid, uid_t loginuid)
>> +{
>> + struct task_struct *tsk;
>> + int err;
>> +
>> + read_lock(&tasklist_lock);
>> + tsk = find_task_by_pid(pid);
>> + err = -ESRCH;
>> + if (!tsk)
>> + goto out;
>> + err = 0;
>> +
>> + spin_lock(&tsk->sighand->siglock);
>> + if (!tsk->signal->audit_tty)
>> + err = -EPERM;
>> + spin_unlock(&tsk->sighand->siglock);
> So siglock nests inside tasklist_lock? Sounds reasonable. Is this a
> preexisting association, or did this patch just create it?
This is used in send_sig_info() and several other functions in
kernel/signal.c.
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d58e74b..3ae4904 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -506,6 +506,8 @@ struct signal_struct {
>> #ifdef CONFIG_TASKSTATS
>> struct taskstats *stats;
>> #endif
>> + unsigned audit_tty:1;
>> + struct tty_audit_buf *tty_audit_buf;
>> };
>
> hm, bitfields are risky. If someone adds another one, it will land in
> the same word and external locking will be needed. You do seem to be using
> ->siglock to cover this - was that to address the bitfield non-atomicity
> problem?
I don't know what the memory access atomicity assumptions are in the
kernel, so I have used the basic rule that any write<->read conflict on
a variable with type other than atomic_t must be prevented by a lock.
This happens to work for the bit field as well.
> A suitable (but somewhat less pretty) way to resolve all this is to not use
> bitfields at all: add `unsigned long flags' and use set_bit/clear_bit/etc.
The new patch replaces the bit field by a simple "unsigned", a whole
word is allocated for the bit field anyway.
>>
>> break;
>> + case AUDIT_TTY_GET: {
>> + struct audit_tty_status s;
>> + struct task_struct *tsk;
>> +
>> + read_lock(&tasklist_lock);
>> + tsk = find_task_by_pid(pid);
>> + if (!tsk)
>> + err = -ESRCH;
>> + else {
>> + spin_lock(&tsk->sighand->siglock);
>> + s.enabled = tsk->signal->audit_tty != 0;
>> + spin_unlock(&tsk->sighand->siglock);
>
> The locking here looks dubious. tsk->signal->audit_tty can change state
> the instant ->siglock gets unlocked, in which case s.enabled is now wrong.
The user-space process must avoid concurrent AUDIT_TTY_SET to get
reasonable results. There's nothing better the kernel can do.
> If that is acceptable then we didn't need that locking at all.
So I can assume that int-sized reads are always atomic with respect to
concurrent writes?
Mirek
From: Miloslav Trmac <[email protected]>
Add TTY input auditing, used to audit system administrator's actions.
TTY input auditing works on a higher level than auditing all system
calls within the session, which would produce an overwhelming amount of
mostly useless audit events.
Add an "audit_tty" attribute, inherited across fork (). Data read from
TTYs by process with the attribute is sent to the audit subsystem by the
kernel. The audit netlink interface is extended to allow modifying the
audit_tty attribute, and to allow sending explanatory audit events from
user-space (for example, a shell might send an event containing the
final command, after the interactive command-line editing and history
expansion is performed, which might be difficult to decipher from the
TTY input alone).
Because the "audit_tty" attribute is inherited across fork (), it would
be set e.g. for sshd restarted within an audited session. To prevent
this, the audit_tty attribute is cleared when a process with no open TTY
file descriptors (e.g. after daemon startup) opens a TTY.
See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.
---
Changes since the previous patch:
* use spin_lock_irq() for siglock
* add an is_tty() function instead of checking f_op->read from n_tty.c;
handle hung TTYs
* replace the audit_tty bit field by a whole word to avoid the risk of
incorrect locking
* move most new code from n_tty.c to a separate file
* fix coding style violations
* fix compilation with !CONFIG_AUDIT
On Fri, 08 Jun 2007 06:23:23 +0200 Miloslav Trmac <[email protected]> wrote:
> From: Miloslav Trmac <[email protected]>
>
> Add TTY input auditing, used to audit system administrator's actions.
> TTY input auditing works on a higher level than auditing all system
> calls within the session, which would produce an overwhelming amount of
> mostly useless audit events.
>
> Add an "audit_tty" attribute, inherited across fork (). Data read from
> TTYs by process with the attribute is sent to the audit subsystem by the
> kernel. The audit netlink interface is extended to allow modifying the
> audit_tty attribute, and to allow sending explanatory audit events from
> user-space (for example, a shell might send an event containing the
> final command, after the interactive command-line editing and history
> expansion is performed, which might be difficult to decipher from the
> TTY input alone).
>
> Because the "audit_tty" attribute is inherited across fork (), it would
> be set e.g. for sshd restarted within an audited session. To prevent
> this, the audit_tty attribute is cleared when a process with no open TTY
> file descriptors (e.g. after daemon startup) opens a TTY.
>
> See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
> for a more detailed rationale document for an older version of this patch.
>
> ---
> Changes since the previous patch:
> * use spin_lock_irq() for siglock
> * add an is_tty() function instead of checking f_op->read from n_tty.c;
> handle hung TTYs
> * replace the audit_tty bit field by a whole word to avoid the risk of
> incorrect locking
> * move most new code from n_tty.c to a separate file
> * fix coding style violations
> * fix compilation with !CONFIG_AUDIT
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d58e74b..d9d734c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -506,6 +506,8 @@ struct signal_struct {
> #ifdef CONFIG_TASKSTATS
> struct taskstats *stats;
> #endif
> + unsigned audit_tty;
> + struct tty_audit_buf *tty_audit_buf;
> };
Can we ifdef these?
Andrew Morton napsal(a):
> On Fri, 08 Jun 2007 06:23:23 +0200 Miloslav Trmac <[email protected]> wrote:
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d58e74b..d9d734c 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -506,6 +506,8 @@ struct signal_struct {
>> #ifdef CONFIG_TASKSTATS
>> struct taskstats *stats;
>> #endif
>> + unsigned audit_tty;
>> + struct tty_audit_buf *tty_audit_buf;
>> };
>
> Can we ifdef these?
Sure, here's an incremental patch.
Mirek