The result is: the user is not able to modify the file via sudoedit. "sudoedit /etc/motd" exits almost immediately after the password is given.

The same result is achievable when you use "sudo -e" instead of "sudoedit".

I played a little bit with EDITOR variable, but the issue still remains.

$ EDITOR=/usr/bin/modifier.sh sudoedit /etc/motd
[sudo] password for sudotester: 
sudoedit: mkstemps: Permission denied

$ cat /usr/bin/modifier.sh
#!/bin/bash
echo '# blahblah' >> $1

You need to change the type and role for the user to an admin role.

"USERNAME ALL = (root) ROLE=sysadm_r TYPE=sysadm_t sudoedit /etc/motd" >> /etc/sudoers

staff_t is not an admin, it is a user role that can transition to an admin role through sudo, but you need to transition via sudo or new role.

newrole -r sysadm_r
sudoedit /etc/motd 

Should have worked also, but I prefer to modify the sudo command.

# useradd -Z staff_u sudotester
# passwd sudotester
Changing password for user sudotester.
New password: 
Retype new password: 
passwd: all authentication tokens updated successfully.
# visudo 
# grep sudotester /etc/sudoers
sudotester ALL = (root) ROLE=sysadm_r TYPE=sysadm_t sudoedit /etc/motd
# 

[sudotester@localhost ~]$ id
uid=511(sudotester) gid=512(sudotester) groups=512(sudotester) context=staff_u:staff_r:staff_t:s0
[sudotester@localhost ~]$ newrole -r sysadm_r
Password: 
[sudotester@localhost ~]$ id
uid=511(sudotester) gid=512(sudotester) groups=512(sudotester) context=staff_u:sysadm_r:sysadm_t:s0
[sudotester@localhost ~]$ sudoedit /etc/motd 
[sudo] password for sudotester: 
sudoedit: mkstemps: Permission denied
[sudotester@localhost ~]$ echo $?
1

mkstemps still complains and I still see following AVCs:
----
time->Tue Apr 19 17:08:41 2011
type=SYSCALL msg=audit(1303225721.811:1462): arch=40000003 syscall=99 success=no exit=-13 a0=121dff0 a1=bfea8068 a2=f847cc a3=2 items=0 ppid=11836 pid=11868 auid=511 uid=0 gid=512 euid=0 suid=0 fsuid=0 egid=512 sgid=512 fsgid=512 tty=pts0 ses=72 comm="sudoedit" exe="/usr/bin/sudoedit" subj=staff_u:sysadm_r:sysadm_sudo_t:s0 key=(null)
type=AVC msg=audit(1303225721.811:1462): avc:  denied  { getattr } for  pid=11868 comm="sudoedit" name="/" dev=devpts ino=1 scontext=staff_u:sysadm_r:sysadm_sudo_t:s0 tcontext=system_u:object_r:devpts_t:s0 tclass=filesystem
----
time->Tue Apr 19 17:08:45 2011
type=SYSCALL msg=audit(1303225725.419:1466): arch=40000003 syscall=5 success=no exit=-13 a0=1224678 a1=c2 a2=180 a3=3c3759 items=0 ppid=11836 pid=11868 auid=511 uid=511 gid=512 euid=511 suid=0 fsuid=511 egid=512 sgid=512 fsgid=512 tty=pts0 ses=72 comm="sudoedit" exe="/usr/bin/sudoedit" subj=staff_u:sysadm_r:sysadm_sudo_t:s0 key=(null)
type=AVC msg=audit(1303225725.419:1466): avc:  denied  { write } for  pid=11868 comm="sudoedit" name="tmp" dev=dm-0 ino=47 scontext=staff_u:sysadm_r:sysadm_sudo_t:s0 tcontext=system_u:object_r:tmp_t:s0 tclass=dir
----

Miroslav I think we need to add

	domain_auto_transition_pattern($1_sudo_t, sudo_exec_t, $3)

to sudo_role_template.

Milos if you add

policy_module(mysudo, 1.0)
gen_require(`
type sysadm_sudo_t, sudo_exec_t, sysadm_t;
')
domain_auto_transition_pattern(sysadm_sudo_t, sudo_exec_t, sysadm_t)


Does this work?

The module described in comment#5 did not help. I still see the same AVCs as in comment#4.

The problem with 

"USERNAME ALL = (root) ROLE=sysadm_r TYPE=sysadm_t /usr/bin/sudoedit /etc/motd" 

or

newrole -r sysadm_r
sudoedit /etc/motd 


is we have the following transition

sysadm_t -> sudo_exec_t -> sysadm_sudo_t

It works fine with 

type sysadm_sudo_tmp_t;
files_type(sysadm_sudo_tmp_t)

manage_files_pattern(sysadm_sudo_t, sysadm_sudo_tmp_t, sysadm_sudo_tmp_t)
files_tmp_filetrans(sysadm_sudo_t, sysadm_sudo_tmp_t, { file })

One more avc msg

avc:  denied  { write } for  pid=32458 comm="sudoedit" name="motd" dev=dm-0 ino=132402 scontext=staff_u:sysadm_r:sysadm_sudo_t:s0 tcontext=system_u:object_r:etc_runtime_t:s0  tclass=file


Milos,
could you test it with a local policy which will have



type sysadm_sudo_tmp_t;
files_type(sysadm_sudo_tmp_t)

manage_files_pattern(sysadm_sudo_t, sysadm_sudo_tmp_t, sysadm_sudo_tmp_t)
files_tmp_filetrans(sysadm_sudo_t, sysadm_sudo_tmp_t, { file })

Following module solved my problem. I don't see any AVCs in enforcing mode when running the automated test.

policy_module(mypol2, 1.0)

gen_require(`
    type sysadm_sudo_t, sudo_exec_t, sysadm_t, etc_t, devpts_t;
')

type sysadm_sudo_tmp_t;
files_type(sysadm_sudo_tmp_t)

manage_files_pattern(sysadm_sudo_t, sysadm_sudo_tmp_t, sysadm_sudo_tmp_t)
files_tmp_filetrans(sysadm_sudo_t, sysadm_sudo_tmp_t, { file })

allow sysadm_sudo_t etc_t:file write;
allow sysadm_sudo_t devpts_t:filesystem getattr;

We need to make changes to the sudoedit code, perhaps some more SELinux inteligence.  We do not want to allow sudoedit to manage any file.

We want to have the editing context actually be syadm_t.

It looks from the AVC's as if sudoedit copies the file to /tmp and then when the user finishes editing it, copies the file over the existing file.

With this policy we have the editor running as sysadm_t which is what we want, but we want the portion of sudoedit that copies /tmp/motd over /etc/motd to be done as sysadm_t also, not as sysadm_sudo_t or staff_sudo_t.

I am reassigning for now to sudo to see if they can explain how sudoedit is working here.

I do not think we can get this done quickly and probably should move this bug to 6.2

(In reply to comment #12)
> We need to make changes to the sudoedit code, perhaps some more SELinux
> inteligence.  We do not want to allow sudoedit to manage any file.

I see two options here:

 1. Use setcon() to change the domain from sudo_t to the requested one
    before doing anything to the files. This probably requires setcurrent & 
    dyntrans to be allowed.

 2. Create a helper (like sesh) which sudo could exec() in the right
    domain and the helper would do the file manipulation and execute the
    editor.

I guess the later option doesn't require modification of the policy since such a transition (exec based) is already allowed.

> We want to have the editing context actually be syadm_t.
>
> It looks from the AVC's as if sudoedit copies the file to /tmp and then when
> the user finishes editing it, copies the file over the existing file.

Yes, that is true. See below for a schematic view of how it works there.

> With this policy we have the editor running as sysadm_t which is what we want,
> but we want the portion of sudoedit that copies /tmp/motd over /etc/motd to be
> done as sysadm_t also, not as sysadm_sudo_t or staff_sudo_t.
> 
> I am reassigning for now to sudo to see if they can explain how sudoedit is
> working here.

Here's how it works in the sudoedit code:

source user = user executing sudo
target user = ...

1. make a copy
  - for each file do:
      seteuid(target user)
      open() + stat() original file
      seteuid(root)
      ... sanity checks ...
      seteuid(source user)
      mkstemp()
      seteuid(root)
      read()+write() (original->temp)
      close()
 
2. run the editor
  - here is all the selinux stuff performed:
    run_command() -> selinux_setup() -> fork()+selinux_execve()

3. replace the original files with the modified copies
  - for each temp file:
    seteuid(source user)
    open() + stat()
    seteuid(root)
    ... sanity & modification checks ...
    seteuid(target user)
    open() original file
    seteuid(root)
    read()+write() (temp->original)
    unlink() temp file
    close()

Let's wait for Dan's comment.

The second solution could work. If I understand correctly.

I like the sesh idea so sudo execs sesh -c EDITOR /tmp/BLAH

Things are more complicated than I thought with the second option. Sudo executes the $EDITOR under the source user. That's good for security, but not for what we are trying to do and I thing that it's actually wrong behavior if selinux is enabled and used (ROLE= and TYPE= set in sudoers), because the editor gets executed with the source uid + target domain, not target uid (runas) and target domain.

However, we can't just change this so that the editor gets executed as the target user, because then he could escape from it and do what he's not supposed to do under the target uid (usually 0).

I'll attach a patch which tries to solve at least the problem with moving the files to /tmp and back. Here is a short description of how it should look like with the patch applied:

1) Copy to /tmp
 - construct (from, to) pairs of strings where
    from = path to the original file
      to = tempnam("/tmp", ...) generated path
 - execute "sesh -e 0 from1 to1 from2 to2 ..." under the target domain
    here the "to#" files are opened with O_CREAT | O_EXCL, and if something
    fails, we'll abort the whole process
 - chown() the temp files to the source user

2) Run the editor (same as before)

3) Copy the files back
   almost the same as in 1) but sesh is executed with each pair reversed and
   the temp files are chowned() back to the target user before executing the
   sesh helper. Also -e is set to 1 to signal the other direction to sesh
   (needed for doing a proper cleanup).

We could generate the temporary files in the helper but then we would have to pass somehow the paths to the parent which executes the editor since we can't execute it from sesh (can't to seteuid() there...)

As this is way too complex (either way) and changes the behavior of sudo, I think it's a good idea not to include this in 6.2, but in 6.3 where a rebase from the 1.7.x to the 1.8.x branch is planned.

Created attachment 529253 [details]
proposed patch

The attached patch changes the way sudo copies files to and from a temporary place for editing. With the patch applied, the copy operation is performed under the selinux domain specified in the sudoers file.

There are still some AVCs. I guess we need to allow chown for the sesh process. It's needed to change the owner of the temporary file before and after editing.

----
time->Thu Oct 20 14:12:53 2011
type=USER_CMD msg=audit(1319112773.688:35249): user pid=18105 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='cwd="/home/sudotester" cmd=2F62696E2F7669202F6574632F6D6F7464 terminal=pts/1 res=success'
----
time->Thu Oct 20 14:12:53 2011
type=USER_ROLE_CHANGE msg=audit(1319112773.690:35250): user pid=18105 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='newrole: old-context=staff_u:staff_r:staff_t:s0 new-context=staff_u:sysadm_r:sysadm_t:s0: exe="/usr/bin/sudo" hostname=? addr=? terminal=pts/1 res=success'
----
time->Thu Oct 20 14:12:53 2011
type=USER_ROLE_CHANGE msg=audit(1319112773.691:35251): user pid=18106 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='newrole: old-context=staff_u:staff_r:staff_t:s0 new-context=staff_u:sysadm_r:sysadm_t:s0: exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/1 res=success'
----
time->Thu Oct 20 14:12:53 2011
type=USER_ROLE_CHANGE msg=audit(1319112773.731:35253): user pid=18107 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='newrole: old-context=staff_u:staff_r:staff_t:s0 new-context=staff_u:sysadm_r:sysadm_t:s0: exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/1 res=success'
----
time->Thu Oct 20 14:12:53 2011
type=SYSCALL msg=audit(1319112773.717:35252): arch=c000003e syscall=92 success=no exit=-1 a0=7f09891c64f0 a1=1f5 a2=1f5 a3=0 items=0 ppid=8927 pid=18105 auid=501 uid=0 gid=501 euid=0 suid=0 fsuid=0 egid=501 sgid=501 fsgid=501 tty=pts1 ses=6 comm="sudo" exe="/usr/bin/sudo" subj=staff_u:staff_r:staff_sudo_t:s0 key=(null)
type=AVC msg=audit(1319112773.717:35252): avc:  denied  { chown } for  pid=18105 comm="sudo" capability=0  scontext=staff_u:staff_r:staff_sudo_t:s0 tcontext=staff_u:staff_r:staff_sudo_t:s0 tclass=capability
----
time->Thu Oct 20 14:12:58 2011
type=USER_ROLE_CHANGE msg=audit(1319112778.915:35254): user pid=18105 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='newrole: old-context=staff_u:staff_r:staff_t:s0 new-context=staff_u:sysadm_r:sysadm_t:s0: exe="/usr/bin/sudo" hostname=? addr=? terminal=pts/1 res=success'
----
time->Thu Oct 20 14:12:58 2011
type=USER_ROLE_CHANGE msg=audit(1319112778.915:35256): user pid=18108 uid=0 auid=501 ses=6 subj=staff_u:staff_r:staff_sudo_t:s0 msg='newrole: old-context=staff_u:staff_r:staff_t:s0 new-context=staff_u:sysadm_r:sysadm_t:s0: exe="/usr/bin/sudo" hostname=? addr=? terminal=/dev/pts/1 res=success'
----
time->Thu Oct 20 14:12:58 2011
type=SYSCALL msg=audit(1319112778.915:35255): arch=c000003e syscall=92 success=no exit=-1 a0=7f09891c64f0 a1=0 a2=0 a3=38 items=0 ppid=8927 pid=18105 auid=501 uid=0 gid=501 euid=0 suid=0 fsuid=0 egid=501 sgid=501 fsgid=501 tty=pts1 ses=6 comm="sudo" exe="/usr/bin/sudo" subj=staff_u:staff_r:staff_sudo_t:s0 key=(null)
type=AVC msg=audit(1319112778.915:35255): avc:  denied  { chown } for  pid=18105 comm="sudo" capability=0  scontext=staff_u:staff_r:staff_sudo_t:s0 tcontext=staff_u:staff_r:staff_sudo_t:s0 tclass=capability

(In reply to comment #20)
> There are still some AVCs. I guess we need to allow chown for the sesh process.
> It's needed to change the owner of the temporary file before and after editing.

I meant the _sudo_ process.

Miroslav we should add this to policy.

    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Cause:
File manipulation done by sudoedit was performed under a wrong SELinux context.

Consequence:
Editing files using sudoedit was impossible when SELinux was in enforcing mode and if a SELinux context was specified by the sudoers rule allowing the user to use sudoedit.

Fix:
The code was modified in a way that allows a transition to the correct SELinux context and perform file manipulation under the correct SELinux context. 

Result:
The user is able to edit files using sudoedit with SELinux set to enforcing mode.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2012-0905.html