Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753000AbcD1TcK (ORCPT ); Thu, 28 Apr 2016 15:32:10 -0400 Received: from mail-pf0-f174.google.com ([209.85.192.174]:32950 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752388AbcD1TcI (ORCPT ); Thu, 28 Apr 2016 15:32:08 -0400 Subject: Re: [PATCH V4] audit: add tty field to LOGIN event To: Richard Guy Briggs References: <571A5C54.7050704@hurleysoftware.com> <20160428013140.GD18994@madcap2.tricolour.ca> <57217DDF.5070608@hurleysoftware.com> <20160428192803.GG18994@madcap2.tricolour.ca> Cc: linux-audit@redhat.com, pmoore@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com, eparis@redhat.com From: Peter Hurley Message-ID: <57226535.9000000@hurleysoftware.com> Date: Thu, 28 Apr 2016 12:32:05 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160428192803.GG18994@madcap2.tricolour.ca> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8672 Lines: 254 On 04/28/2016 12:28 PM, Richard Guy Briggs wrote: > On 16/04/27, Peter Hurley wrote: >> On 04/27/2016 06:31 PM, Richard Guy Briggs wrote: >>> On 16/04/22, Peter Hurley wrote: >>>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote: >>>>> The tty field was missing from AUDIT_LOGIN events. >>>>> >>>>> Refactor code to create a new function audit_get_tty(), using it to >>>>> replace the call in audit_log_task_info() and to add it to >>>>> audit_log_set_loginuid(). Lock and bump the kref to protect it, adding >>>>> audit_put_tty() alias to decrement it. >>>>> >>>>> Signed-off-by: Richard Guy Briggs >>>>> --- >>>>> >>>>> V4: Add missing prototype for audit_put_tty() when audit syscall is not >>>>> enabled (MIPS). >>>>> >>>>> V3: Introduce audit_put_tty() alias to decrement kref. >>>>> >>>>> V2: Use kref to protect tty signal struct while in use. >>>>> >>>>> --- >>>>> >>>>> include/linux/audit.h | 24 ++++++++++++++++++++++++ >>>>> kernel/audit.c | 18 +++++------------- >>>>> kernel/auditsc.c | 8 ++++++-- >>>>> 3 files changed, 35 insertions(+), 15 deletions(-) >>>>> >>>>> 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #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); >>>> >>>> >>>> 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). >>> >>> Ok. This logic goes back 10 years and one month less two days. (45d9bb0e) >>> >>>> 2. The existing usage is always tsk==current >>> >>> My understanding is that when it is called via: >>> >>> copy_process() >>> audit_free() >>> __audit_free() >>> audit_log_exit() >>> audit_log_task_info() >>> >>> then tsk != current. >> >> While it's true that tsk != current here, everything relevant to tty >> in task_struct is the same because the nascent task is not even half-done. >> So tsk->sighand == current->sighand, tsk->signal == current->signal etc. > > I agree this is true except in the case of !CLONE_SIGHAND, if it fails > after copy_sighand() or copy_signal() then it would be null and would > get freed before audit_free() is called. By the time tty gets copied > from current in this case, it is past the point of failure in > copy_process(). Oh, right. >> If you're uncomfortable with pass-through execution like that, then the >> simple solution is: >> >> struct tty_struct *tty = NULL; >> >> /* tsk != current when copy_process() failed */ >> if (tsk == current) >> tty = get_current_tty(); >> >> because tty_kref_put(tty) accepts NULL tty and (obviously) so does >> tty_name(tty). > > Given the circumstances above, this appears reasonable to me at first > look. Ok. I'll spend more analysis time before I actually submit a patch for this. >>> This appears to be the only case which appears to >>> force lugging around tsk. This is noted in that commit referenced >>> above. >>> >>>> 3. If the idea is to make this invulnerable to tsk being gone, then >>>> the usage is unsafe anyway. >>>> >>>> >>>> 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. >>> >>> This latter option did cross my mind... >>> >>>> Peter Hurley >>>> >>>>> + return tty; >>>>> +} >>>>> + >>>>> +static inline void audit_put_tty(struct tty_struct *tty) >>>>> +{ >>>>> + tty_kref_put(tty); >>>>> +} >>>>> + >>>>> extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp); >>>>> extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode); >>>>> extern void __audit_bprm(struct linux_binprm *bprm); >>>>> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) >>>>> { >>>>> return -1; >>>>> } >>>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk) >>>>> +{ >>>>> + return NULL; >>>>> +} >>>>> +static inline void audit_put_tty(struct tty_struct *tty) >>>>> +{ } >>>>> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) >>>>> { } >>>>> static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid, >>>>> diff --git a/kernel/audit.c b/kernel/audit.c >>>>> index 3a3e5de..7edd776 100644 >>>>> --- a/kernel/audit.c >>>>> +++ b/kernel/audit.c >>>>> @@ -64,7 +64,6 @@ >>>>> #include >>>>> #endif >>>>> #include >>>>> -#include >>>>> #include >>>>> #include >>>>> >>>>> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) >>>>> { >>>>> const struct cred *cred; >>>>> char comm[sizeof(tsk->comm)]; >>>>> - char *tty; >>>>> + struct tty_struct *tty; >>>>> >>>>> if (!ab) >>>>> return; >>>>> >>>>> /* tsk == current */ >>>>> cred = current_cred(); >>>>> - >>>>> - spin_lock_irq(&tsk->sighand->siglock); >>>>> - if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name) >>>>> - tty = tsk->signal->tty->name; >>>>> - else >>>>> - tty = "(none)"; >>>>> - spin_unlock_irq(&tsk->sighand->siglock); >>>>> - >>>>> + tty = audit_get_tty(tsk); >>>>> audit_log_format(ab, >>>>> " ppid=%d pid=%d auid=%u uid=%u gid=%u" >>>>> " euid=%u suid=%u fsuid=%u" >>>>> @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) >>>>> from_kgid(&init_user_ns, cred->egid), >>>>> from_kgid(&init_user_ns, cred->sgid), >>>>> from_kgid(&init_user_ns, cred->fsgid), >>>>> - tty, audit_get_sessionid(tsk)); >>>>> - >>>>> + tty ? tty_name(tty) : "(none)", >>>>> + audit_get_sessionid(tsk)); >>>>> + audit_put_tty(tty); >>>>> audit_log_format(ab, " comm="); >>>>> audit_log_untrustedstring(ab, get_task_comm(comm, tsk)); >>>>> - >>>>> audit_log_d_path_exe(ab, tsk->mm); >>>>> audit_log_task_context(ab); >>>>> } >>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >>>>> index 195ffae..71e14d8 100644 >>>>> --- a/kernel/auditsc.c >>>>> +++ b/kernel/auditsc.c >>>>> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid, >>>>> { >>>>> struct audit_buffer *ab; >>>>> uid_t uid, oldloginuid, loginuid; >>>>> + struct tty_struct *tty; >>>>> >>>>> if (!audit_enabled) >>>>> return; >>>>> @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid, >>>>> uid = from_kuid(&init_user_ns, task_uid(current)); >>>>> oldloginuid = from_kuid(&init_user_ns, koldloginuid); >>>>> loginuid = from_kuid(&init_user_ns, kloginuid), >>>>> + tty = audit_get_tty(current); >>>>> >>>>> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN); >>>>> if (!ab) >>>>> return; >>>>> audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid); >>>>> audit_log_task_context(ab); >>>>> - audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d", >>>>> - oldloginuid, loginuid, oldsessionid, sessionid, !rc); >>>>> + audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u res=%d", >>>>> + oldloginuid, loginuid, tty ? tty_name(tty) : "(none)", >>>>> + oldsessionid, sessionid, !rc); >>>>> + audit_put_tty(tty); >>>>> audit_log_end(ab); >>>>> } >>>>> >>>>> >>>> >>> >>> - RGB >>> >>> -- >>> Richard Guy Briggs >>> Kernel Security Engineering, Base Operating Systems, Red Hat >>> Remote, Ottawa, Canada >>> Voice: +1.647.777.2635, Internal: (81) 32635 >>> >> > > - RGB > > -- > Richard Guy Briggs > Kernel Security Engineering, Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635 >