Re: question about selinux_restore_tty() in sudo's selinux.c

[Stephen Smalley <sds@tycho.nsa.gov>]

On 9/27/19 10:49 AM, Stephen Smalley wrote:
> On 9/27/19 10:28 AM, Dominick Grift wrote:
>> On Fri, Sep 27, 2019 at 10:17:16AM -0400, Stephen Smalley wrote:
>>> On 9/27/19 10:01 AM, Stephen Smalley wrote:
>>>> On 9/27/19 9:57 AM, Dominick Grift wrote:
>>>>> On Fri, Sep 27, 2019 at 09:51:26AM -0400, Stephen Smalley wrote:
>>>>>> On 9/27/19 9:49 AM, Dominick Grift wrote:
>>>>>>> On Fri, Sep 27, 2019 at 09:33:06AM -0400, Stephen Smalley wrote:
>>>>>>>> On 9/27/19 9:08 AM, Dominick Grift wrote:
>>>>>>>>> On Fri, Sep 27, 2019 at 08:59:26AM -0400, Stephen Smalley wrote:
>>>>>>>>>> On 9/27/19 3:55 AM, Dominick Grift wrote:
>>>>>>>>>>> sudo does not reset the role of my tty
>>>>>>>>>>> properly [1], and i was wondering if anyone
>>>>>>>>>>> is able to determine what is causing this
>>>>>>>>>>> [2]
>>>>>>>>>>>
>>>>>>>>>>> [1] https://bugzilla.sudo.ws/show_bug.cgi?id=898
>>>>>>>>>>> [2] https://www.sudo.ws/repos/sudo/file/tip/src/selinux.c
>>>>>>>>>>
>>>>>>>>>> Are you sure sudo is calling selinux_restore_tty()?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> running sudo with:
>>>>>>>>>
>>>>>>>>> Debug sudo /var/log/sudo_debug all@debug
>>>>>>>>> Debug sudoers.so /var/log/sudo_debug all@debug
>>>>>>>>>
>>>>>>>>> Yields:
>>>>>>>>>
>>>>>>>>> grep selinux /var/log/sudo_debug
>>>>>>>>> Sep 27 15:06:29 sudo[3417] <- sudo_new_key_val_v1 @
>>>>>>>>> ../../../lib/util/key_val.c:61 :=
>>>>>>>>> selinux_role=sysadm.role
>>>>>>>>> Sep 27 15:06:29 sudo[3417]     7: selinux_role=sysadm.role
>>>>>>>>> Sep 27 15:06:29 sudo[3447] -> selinux_setup @ 
>>>>>>>>> ../../src/selinux.c:349
>>>>>>>>> Sep 27 15:06:29 sudo[3447] -> get_exec_context @
>>>>>>>>> ../../src/selinux.c:274
>>>>>>>>> Sep 27 15:06:29 sudo[3447] <- get_exec_context @
>>>>>>>>> ../../src/selinux.c:328 := 0x564eed3621b0
>>>>>>>>> Sep 27 15:06:29 sudo[3447] -> relabel_tty @ 
>>>>>>>>> ../../src/selinux.c:160
>>>>>>>>> Sep 27 15:06:29 sudo[3447] <- relabel_tty @
>>>>>>>>> ../../src/selinux.c:253 := 0
>>>>>>>>> Sep 27 15:06:29 sudo[3447] -> audit_role_change @
>>>>>>>>> ../../src/selinux.c:76
>>>>>>>>> Sep 27 15:06:29 sudo[3447] <- audit_role_change @
>>>>>>>>> ../../src/selinux.c:98 := 6
>>>>>>>>> Sep 27 15:06:29 sudo[3447] <- selinux_setup @
>>>>>>>>> ../../src/selinux.c:395 := 0
>>>>>>>>> Sep 27 15:06:29 sudo[3447] -> selinux_execve @
>>>>>>>>> ../../src/selinux.c:405
>>>>>>>>> Sep 27 15:06:29 sudo[3417] -> selinux_restore_tty @
>>>>>>>>> ../../src/selinux.c:114
>>>>>>>>> Sep 27 15:06:29 sudo[3417] <- selinux_restore_tty @
>>>>>>>>> ../../src/selinux.c:142 := 0
>>>>>>>>
>>>>>>>> Ok, so you entered and exited selinux_restore_tty() without 
>>>>>>>> error. No
>>>>>>>> warning messages of any kind in any of the sudo logs? Not sure 
>>>>>>>> where
>>>>>>>> sudo_warn() and sudo_warnx() messages go.
>>>>>>>
>>>>>>> No warnings or any other clues. I guess it must be this then:
>>>>>>>
>>>>>>>        if (se_state.ttyfd == -1 || se_state.new_tty_context == NULL)
>>>>>>>      goto skip_relabel;
>>>>>>
>>>>>> That should only occur if there was no tty or 
>>>>>> security_compute_relabel()
>>>>>> didn't provide a new context to set in the first place. Not if
>>>>>> it actually
>>>>>> relabeled the tty earlier.
>>>>>>
>>>>>> Does newrole work correctly with your policy?
>>>>>
>>>>> Yes:
>>>>>
>>>>> kcinimod@seamus:~$ id -Z
>>>>> wheel.id:wheel.role:wheel.subj:s0
>>>>> kcinimod@seamus:~$ ls -alZ `tty`
>>>>> crw--w----. 1 kcinimod tty
>>>>> wheel.id:wheel.role:users.terminals.pty.pty_file:s0 136, 10 Sep 27
>>>>> 15:55 /dev/pts/10
>>>>> kcinimod@seamus:~$ newrole -r sysadm.role
>>>>> Password:
>>>>> newrole: incorrect password for kcinimod
>>>>> kcinimod@seamus:~$ ls -alZ `tty`
>>>>> crw--w----. 1 kcinimod tty
>>>>> wheel.id:wheel.role:users.terminals.pty.pty_file:s0 136, 10 Sep 27
>>>>> 15:56 /dev/pts/10
>>>>> kcinimod@seamus:~$ newrole -r sysadm.role
>>>>> Password:
>>>>> kcinimod@seamus:~$ id -Z
>>>>> wheel.id:sysadm.role:sysadm.subj:s0
>>>>> kcinimod@seamus:~$ ls -alZ `tty`
>>>>> crw--w----. 1 kcinimod tty
>>>>> wheel.id:sysadm.role:users.terminals.pty.pty_file:s0 136, 10 Sep 27
>>>>> 15:56 /dev/pts/10
>>>>> kcinimod@seamus:~$ exit
>>>>> logout
>>>>> kcinimod@seamus:~$ ls -alZ `tty`
>>>>> crw--w----. 1 kcinimod tty
>>>>> wheel.id:wheel.role:users.terminals.pty.pty_file:s0 136, 10 Sep 27
>>>>> 15:56 /dev/pts/10
>>>>
>>>> Ok, so at least we know it is a bug in the sudo code somewhere.  I'd
>>>> assume that for some reason ttyfd or new_tty_context are either not
>>>> being set in the first place or are being unset somewhere?
>>>
>>> Can you provide your sudo package version so I can be sure I'm 
>>> looking at
>>> the right sources?  The line numbers in your debug output don't quite 
>>> align
>>> with the current upstream.
>>
>> root@seamus:~# apt info sudo
>> Package: sudo
>> Version: 1.8.27-1+b1
>> Priority: optional
>> Section: admin
>> Source: sudo (1.8.27-1)
>> Maintainer: Bdale Garbee <bdale@gag.com>
>> Installed-Size: 3,879 kB
>> Depends: libaudit1 (>= 1:2.2.1), libc6 (>= 2.27), libpam0g (>= 
>> 0.99.7.1), libselinux1 (>= 1.32), libpam-modules, lsb-base
>> Conflicts: sudo-ldap
>> Replaces: sudo-ldap
>> Homepage: http://www.sudo.ws/
>> Tag: admin::login, admin::user-management, implemented-in::c,
>>   interface::commandline, role::program, scope::utility,
>>   security::authentication, use::login
>> Download-Size: 1,244 kB
>> APT-Manual-Installed: yes
>> APT-Sources: http://mirror.nl.leaseweb.net/debian sid/main amd64 Packages
>> Description: Provide limited super user privileges to specific users
>>   Sudo is a program designed to allow a sysadmin to give limited root
>>   privileges to users and log root activity.  The basic philosophy is 
>> to give
>>   as few privileges as possible but still allow people to get their 
>> work done.
>>   .
>>   This version is built with minimal shared library dependencies, use the
>>   sudo-ldap package instead if you need LDAP support for sudoers.
> 
> Ok, I can match up the line numbers successfully.
> 
> I could be wrong since I am not especially familiar with sudo's internal 
> structure, but I'm wondering if the call to selinux_setup() is occurring 
> in a different process than the call to selinux_restore_tty().  If so, 
> then the use of global state by those functions like the se_state would 
> be broken.  Ask the sudo maintainers.   This probably wouldn't show up 
> under Fedora's default policies since I don't think they bother with 
> different types on ttys for the different user roles, so no relabeling 
> is needed.

Actually, looking back at your debug logs, that is evident from the 
different PIDs reported for the calls to selinux_setup() vs 
selinux_restore_tty().  Originally these were probably both called from 
the parent process, then someone must have re-factored and moved the 
selinux_setup() to the child?