linux-kernel - Re: [PATCH V4] audit: add tty field to LOGIN event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 26 Apr 2016 18:34:41 -0400
From:	Paul Moore <paul@...l-moore.com>
To:	Peter Hurley <peter@...leysoftware.com>,
	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, Paul Moore <pmoore@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] audit: add tty field to LOGIN event

On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley <peter@...leysoftware.com> wrote:
> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index b40ed5d..32cdafb 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/sched.h>
>>  #include <linux/ptrace.h>
>>  #include <uapi/linux/audit.h>
>> +#include <linux/tty.h>
>>
>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>>       return tsk->sessionid;
>>  }
>>
>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>> +{
>> +     struct tty_struct *tty = NULL;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&tsk->sighand->siglock, flags);
>> +     if (tsk->signal)
>> +             tty = tty_kref_get(tsk->signal->tty);
>> +     spin_unlock_irqrestore(&tsk->sighand->siglock, flags);

I just merged Richard's patch, if nothing else it is better than it
was.  However, I would like to talk about improving things, see below.

> Not that I'm objecting because I get that you're just refactoring
> existing code, but I thought I'd point out some stuff.
>
> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>    because if it is, this will blow up trying to dereference the
>    sighand_struct (ie tsk->sighand).
>
> 2. The existing usage is always tsk==current

Yep, there is only one caller I found that even works on task_structs
other than current (see audit_log_exit() via audit_free()), although
even then when it ends up calling into audit_log_task_info() tsk
should always be current.

I've got a patch compiling now to get rid of passing around current as
a a task_struct argument, assuming nothing blows up in testing I'll
post/merge it.

> 3. If the idea is to make this invulnerable to tsk being gone, then
>    the usage is unsafe anyway.

I don't think that is our concern here.

> So ultimately (but not necessarily for this patch) I'd prefer that either
> a. audit use existing tty api instead of open-coding, or
> b. add any tty api functions required.

I'm open to suggestions, care to elaborate on either option?  Feel
free to elaborate by patch too ;)

-- 
paul moore
www.paul-moore.com