[PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris]

This patch turns the case where we have a call into avc_has_perm with no
requested permissions into a BUG_ON.  All callers to this should be in
the kernel and thus should be a function we need to fix if we ever hit
this.  The /selinux/access permission checking it done directly in the
security server and not through the avc, so those requests which we
cannot control from userspace should not be able to trigger this BUG_ON.

Signed-off-by: Eric Paris <eparis@redhat.com>

---

 security/selinux/avc.c         |    4 +++-
 1 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 187964e..cb3f0ce 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -871,6 +871,8 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 	int rc = 0;
 	u32 denied;
 
+	BUG_ON(!requested);
+
 	rcu_read_lock();
 
 	node = avc_lookup(ssid, tsid, tclass, requested);
@@ -890,7 +892,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 
 	denied = requested & ~(p_ae->avd.allowed);
 
-	if (!requested || denied) {
+	if (denied) {
 		if (selinux_enforcing || (flags & AVC_STRICT))
 			rc = -EACCES;
 		else


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Stephen Smalley]

On Tue, 2008-03-11 at 14:19 -0400, Eric Paris wrote:
> This patch turns the case where we have a call into avc_has_perm with no
> requested permissions into a BUG_ON.  All callers to this should be in
> the kernel and thus should be a function we need to fix if we ever hit
> this.  The /selinux/access permission checking it done directly in the
> security server and not through the avc, so those requests which we
> cannot control from userspace should not be able to trigger this BUG_ON.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by:  Stephen D. Smalley <sds@tycho.nsa.gov>

> 
> ---
> 
>  security/selinux/avc.c         |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 187964e..cb3f0ce 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -871,6 +871,8 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
>  	int rc = 0;
>  	u32 denied;
>  
> +	BUG_ON(!requested);
> +
>  	rcu_read_lock();
>  
>  	node = avc_lookup(ssid, tsid, tclass, requested);
> @@ -890,7 +892,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
>  
>  	denied = requested & ~(p_ae->avd.allowed);
>  
> -	if (!requested || denied) {
> +	if (denied) {
>  		if (selinux_enforcing || (flags & AVC_STRICT))
>  			rc = -EACCES;
>  		else
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[James Morris]

On Tue, 11 Mar 2008, Eric Paris wrote:

> This patch turns the case where we have a call into avc_has_perm with no
> requested permissions into a BUG_ON.  All callers to this should be in
> the kernel and thus should be a function we need to fix if we ever hit
> this.  The /selinux/access permission checking it done directly in the
> security server and not through the avc, so those requests which we
> cannot control from userspace should not be able to trigger this BUG_ON.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> 

Applied to 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[James Morris]

On Wed, 12 Mar 2008, James Morris wrote:

> Applied to 
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm

Just saw this during boot.

[    8.238863] ------------[ cut here ]------------
[    8.239762] kernel BUG at security/selinux/avc.c:874!
[    8.239762] invalid opcode: 0000 [1] SMP 
[    8.239762] CPU 2 
[    8.239762] Modules linked in: ipv6 cpufreq_ondemand acpi_cpufreq freq_table dm_mirror dm_multipath kvm_intel kvm rtc
_cmos rtc_core serio_raw pcspkr rtc_lib e1000e button pata_acpi [last unloaded: microcode]
[    8.239762] Pid: 1795, comm: mdadm Not tainted 2.6.25-rc4 #19
[    8.239762] RIP: 0010:[<ffffffff80319c4c>]  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
[    8.239762] RSP: 0018:ffff81007ad3dc28  EFLAGS: 00010246
[    8.239762] RAX: ffff81007ad3dd58 RBX: 000000000000000a RCX: 0000000000000000
[    8.239762] RDX: 000000000000000a RSI: 000000000000001b RDI: 000000000000007a
[    8.239762] RBP: ffff81007ad3dcc8 R08: 0000000000000000 R09: ffff81007ad3dcf8
[    8.239762] R10: ffff81007ad3dbd4 R11: 0000000000000246 R12: ffff81007dc1e170
[    8.239762] R13: 000000000000001b R14: 000000000000001b R15: 000000000000007a
[    8.239762] FS:  00007fc4cfc1c6f0(0000) GS:ffff81007f8dd768(0000) knlGS:0000000000000000
[    8.239762] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    8.239762] CR2: 00000000004239b7 CR3: 000000007adb8000 CR4: 00000000000026e0
[    8.239762] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    8.239762] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[    8.239762] Process mdadm (pid: 1795, threadinfo ffff81007ad3c000, task ffff81007ba3a180)
[    8.239762] Stack:  ffff81007ad3dca8 ffff81007ad3dcf8 000000007ad3dcb8 ffff810000000000
[    8.239762]  ffff81007ad3dc68 0000007a802a54cd ffffffffffffffff ffffffef00000000
[    8.239762]  ffff810000000001 ffffffff802a7e65 ffff81007f81c168 ffff81007ad3dcb8
[    8.239762] Call Trace:
[    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
[    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
[    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
[    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
[    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
[    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
[    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
[    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
[    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
[    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
[    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
[    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
[    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
[    8.239762] 
[    8.239762] 
[    8.239762] Code: 41 89 ff 41 56 41 89 f6 41 55 41 54 53 89 d3 48 83 ec 78 89 8d 78 ff ff ff 44 89 85 74 ff ff ff 4c 89 8d 68 ff ff ff 85 c9 75 04 <0f> 0b eb fe b9 02 00 00 00 31 d2 49 c7 c1 50 9c 31 80 41 b8 01 
[    8.239762] RIP  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
[    8.239762]  RSP <ffff81007ad3dc28>
[    8.239769] ---[ end trace ca143223eefdc828 ]---


 
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris]

On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> On Wed, 12 Mar 2008, James Morris wrote:
> 
> > Applied to 
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> 
> Just saw this during boot.
> 
> [    8.238863] ------------[ cut here ]------------
> [    8.239762] kernel BUG at security/selinux/avc.c:874!
> [    8.239762] invalid opcode: 0000 [1] SMP 
> [    8.239762] CPU 2 
> [    8.239762] Modules linked in: ipv6 cpufreq_ondemand acpi_cpufreq freq_table dm_mirror dm_multipath kvm_intel kvm rtc
> _cmos rtc_core serio_raw pcspkr rtc_lib e1000e button pata_acpi [last unloaded: microcode]
> [    8.239762] Pid: 1795, comm: mdadm Not tainted 2.6.25-rc4 #19
> [    8.239762] RIP: 0010:[<ffffffff80319c4c>]  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
> [    8.239762] RSP: 0018:ffff81007ad3dc28  EFLAGS: 00010246
> [    8.239762] RAX: ffff81007ad3dd58 RBX: 000000000000000a RCX: 0000000000000000
> [    8.239762] RDX: 000000000000000a RSI: 000000000000001b RDI: 000000000000007a
> [    8.239762] RBP: ffff81007ad3dcc8 R08: 0000000000000000 R09: ffff81007ad3dcf8
> [    8.239762] R10: ffff81007ad3dbd4 R11: 0000000000000246 R12: ffff81007dc1e170
> [    8.239762] R13: 000000000000001b R14: 000000000000001b R15: 000000000000007a
> [    8.239762] FS:  00007fc4cfc1c6f0(0000) GS:ffff81007f8dd768(0000) knlGS:0000000000000000
> [    8.239762] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [    8.239762] CR2: 00000000004239b7 CR3: 000000007adb8000 CR4: 00000000000026e0
> [    8.239762] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    8.239762] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [    8.239762] Process mdadm (pid: 1795, threadinfo ffff81007ad3c000, task ffff81007ba3a180)
> [    8.239762] Stack:  ffff81007ad3dca8 ffff81007ad3dcf8 000000007ad3dcb8 ffff810000000000
> [    8.239762]  ffff81007ad3dc68 0000007a802a54cd ffffffffffffffff ffffffef00000000
> [    8.239762]  ffff810000000001 ffffffff802a7e65 ffff81007f81c168 ffff81007ad3dcb8
> [    8.239762] Call Trace:
> [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
> [    8.239762] 
> [    8.239762] 
> [    8.239762] Code: 41 89 ff 41 56 41 89 f6 41 55 41 54 53 89 d3 48 83 ec 78 89 8d 78 ff ff ff 44 89 85 74 ff ff ff 4c 89 8d 68 ff ff ff 85 c9 75 04 <0f> 0b eb fe b9 02 00 00 00 31 d2 49 c7 c1 50 9c 31 80 41 b8 01 
> [    8.239762] RIP  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
> [    8.239762]  RSP <ffff81007ad3dc28>
> [    8.239769] ---[ end trace ca143223eefdc828 ]---


How the heck does mdadm call sys_open without read or write?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Stephen Smalley]

On Tue, 2008-03-11 at 21:10 -0400, Eric Paris wrote:
> On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> > On Wed, 12 Mar 2008, James Morris wrote:
> > 
> > > Applied to 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> > 
> > Just saw this during boot.
> > 
> > [    8.238863] ------------[ cut here ]------------
> > [    8.239762] kernel BUG at security/selinux/avc.c:874!
> > [    8.239762] invalid opcode: 0000 [1] SMP 
> > [    8.239762] CPU 2 
> > [    8.239762] Modules linked in: ipv6 cpufreq_ondemand acpi_cpufreq freq_table dm_mirror dm_multipath kvm_intel kvm rtc
> > _cmos rtc_core serio_raw pcspkr rtc_lib e1000e button pata_acpi [last unloaded: microcode]
> > [    8.239762] Pid: 1795, comm: mdadm Not tainted 2.6.25-rc4 #19
> > [    8.239762] RIP: 0010:[<ffffffff80319c4c>]  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
> > [    8.239762] RSP: 0018:ffff81007ad3dc28  EFLAGS: 00010246
> > [    8.239762] RAX: ffff81007ad3dd58 RBX: 000000000000000a RCX: 0000000000000000
> > [    8.239762] RDX: 000000000000000a RSI: 000000000000001b RDI: 000000000000007a
> > [    8.239762] RBP: ffff81007ad3dcc8 R08: 0000000000000000 R09: ffff81007ad3dcf8
> > [    8.239762] R10: ffff81007ad3dbd4 R11: 0000000000000246 R12: ffff81007dc1e170
> > [    8.239762] R13: 000000000000001b R14: 000000000000001b R15: 000000000000007a
> > [    8.239762] FS:  00007fc4cfc1c6f0(0000) GS:ffff81007f8dd768(0000) knlGS:0000000000000000
> > [    8.239762] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > [    8.239762] CR2: 00000000004239b7 CR3: 000000007adb8000 CR4: 00000000000026e0
> > [    8.239762] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [    8.239762] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > [    8.239762] Process mdadm (pid: 1795, threadinfo ffff81007ad3c000, task ffff81007ba3a180)
> > [    8.239762] Stack:  ffff81007ad3dca8 ffff81007ad3dcf8 000000007ad3dcb8 ffff810000000000
> > [    8.239762]  ffff81007ad3dc68 0000007a802a54cd ffffffffffffffff ffffffef00000000
> > [    8.239762]  ffff810000000001 ffffffff802a7e65 ffff81007f81c168 ffff81007ad3dcb8
> > [    8.239762] Call Trace:
> > [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> > [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> > [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> > [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> > [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> > [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> > [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> > [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> > [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> > [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> > [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> > [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> > [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
> > [    8.239762] 
> > [    8.239762] 
> > [    8.239762] Code: 41 89 ff 41 56 41 89 f6 41 55 41 54 53 89 d3 48 83 ec 78 89 8d 78 ff ff ff 44 89 85 74 ff ff ff 4c 89 8d 68 ff ff ff 85 c9 75 04 <0f> 0b eb fe b9 02 00 00 00 31 d2 49 c7 c1 50 9c 31 80 41 b8 01 
> > [    8.239762] RIP  [<ffffffff80319c4c>] avc_has_perm_noaudit+0x31/0x43c
> > [    8.239762]  RSP <ffff81007ad3dc28>
> > [    8.239769] ---[ end trace ca143223eefdc828 ]---
> 
> 
> How the heck does mdadm call sys_open without read or write?

Indeed - that is a bug.

And without Eric's patch, it should have been turned into a denial for
the existing code, so it would have failed, just not as spectacularly.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris]

On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> On Wed, 12 Mar 2008, James Morris wrote:
> 
> > Applied to 
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> 
> Just saw this during boot.
> 
> [    8.238863] ------------[ cut here ]------------
> [    8.239762] kernel BUG at security/selinux/avc.c:874!

> [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80

Admittedly I don't understand all the code, but James, can you let me
know if this solves the problem?  I don't see this issue on my machine
and I don't know what opening a file with 'special' means.

/*
 * Note that while the flag value (low two bits) for sys_open means:
 *      00 - read-only
 *      01 - write-only
 *      10 - read-write
 *      11 - special
 * it is changed into
 *      00 - no permissions needed
 *      01 - read-permission
 *      10 - write-permission
 *      11 - read-write
 * for the internal routines (ie open_namei()/follow_link() etc). 00 is
 * used by symlinks.
 */

I noticed that the conversion of flags to f_mode in __dentry_open()
handles things a bit differently than do_filp_open().  This makes the
__dentry_open() handling like do_filp_open() so that 'special' gets
turned into RW instead of no permissions.

Time to go figure out what the heck 'special' means....

-Eric

---

diff --git a/fs/open.c b/fs/open.c
index 5419853..04a8efa 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -736,10 +736,15 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 {
 	struct inode *inode;
 	int error;
+	mode_t f_mode;
+
+	if ((flags+1) & O_ACCMODE)
+		f_mode = (flags+1) & O_ACCMODE;
+	else
+		f_mode = flags & O_ACCMODE;
 
 	f->f_flags = flags;
-	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
-				FMODE_PREAD | FMODE_PWRITE;
+	f->f_mode = f_mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
 	inode = dentry->d_inode;
 	if (f->f_mode & FMODE_WRITE) {
 		error = get_write_access(inode);



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris]

On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> On Wed, 12 Mar 2008, James Morris wrote:
> 
> > Applied to 
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> 
> Just saw this during boot.
> 
> [    8.238863] ------------[ cut here ]------------
> [    8.239762] kernel BUG at security/selinux/avc.c:874!

turns out this is easy to reproduce.

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main(void)
{
	open("/dev/null", 3);
	return 0;
}

I'm testing a new patch which once I full understand I'll send upstream
to see what they have to say...

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Stephen Smalley]

On Wed, 2008-03-12 at 11:31 -0400, Eric Paris wrote:
> On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> > On Wed, 12 Mar 2008, James Morris wrote:
> > 
> > > Applied to 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> > 
> > Just saw this during boot.
> > 
> > [    8.238863] ------------[ cut here ]------------
> > [    8.239762] kernel BUG at security/selinux/avc.c:874!
> 
> > [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> > [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> > [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> > [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> > [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> > [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> > [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> > [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> > [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> > [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> > [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> > [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> > [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
> 
> Admittedly I don't understand all the code, but James, can you let me
> know if this solves the problem?  I don't see this issue on my machine
> and I don't know what opening a file with 'special' means.
> 
> /*
>  * Note that while the flag value (low two bits) for sys_open means:
>  *      00 - read-only
>  *      01 - write-only
>  *      10 - read-write
>  *      11 - special
>  * it is changed into
>  *      00 - no permissions needed
>  *      01 - read-permission
>  *      10 - write-permission
>  *      11 - read-write
>  * for the internal routines (ie open_namei()/follow_link() etc). 00 is
>  * used by symlinks.
>  */
> 
> I noticed that the conversion of flags to f_mode in __dentry_open()
> handles things a bit differently than do_filp_open().  This makes the
> __dentry_open() handling like do_filp_open() so that 'special' gets
> turned into RW instead of no permissions.
> 
> Time to go figure out what the heck 'special' means....
> 
> -Eric
> 
> ---
> 
> diff --git a/fs/open.c b/fs/open.c
> index 5419853..04a8efa 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -736,10 +736,15 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
>  {
>  	struct inode *inode;
>  	int error;
> +	mode_t f_mode;
> +
> +	if ((flags+1) & O_ACCMODE)
> +		f_mode = (flags+1) & O_ACCMODE;
> +	else
> +		f_mode = flags & O_ACCMODE;
>  
>  	f->f_flags = flags;
> -	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
> -				FMODE_PREAD | FMODE_PWRITE;
> +	f->f_mode = f_mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
>  	inode = dentry->d_inode;
>  	if (f->f_mode & FMODE_WRITE) {
>  		error = get_write_access(inode);

Alternatively, if this is a valid state and is used internally by the
kernel, then possibly selinux_dentry_open() should just return 0
immediately if ((file->f_mode & (FMODE_READ | FMODE_WRITE) == 0), much
as selinux_inode_permission() returns 0 if the mask is 0 (existence
test).  Not sure though why we haven't encountered this before since we
should have been getting denials even before Eric's patch.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] SELinux: requesting no permissions in avc_has_perm_noaudit is a BUG()

[Eric Paris]

On Wed, 2008-03-12 at 13:18 -0400, Stephen Smalley wrote:
> On Wed, 2008-03-12 at 11:31 -0400, Eric Paris wrote:
> > On Wed, 2008-03-12 at 09:26 +1100, James Morris wrote:
> > > On Wed, 12 Mar 2008, James Morris wrote:
> > > 
> > > > Applied to 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm
> > > 
> > > Just saw this during boot.
> > > 
> > > [    8.238863] ------------[ cut here ]------------
> > > [    8.239762] kernel BUG at security/selinux/avc.c:874!
> > 
> > > [    8.239762]  [<ffffffff802a7e65>] ? link_path_walk+0xbd/0xcd
> > > [    8.239762]  [<ffffffff8031ab34>] avc_has_perm+0x2e/0x5e
> > > [    8.239762]  [<ffffffff8031b9ca>] inode_has_perm+0x6b/0x7a
> > > [    8.239762]  [<ffffffff8031f242>] selinux_dentry_open+0x6a/0x73
> > > [    8.239762]  [<ffffffff8031742e>] security_dentry_open+0x11/0x13
> > > [    8.239762]  [<ffffffff8029c69a>] __dentry_open+0xce/0x1d3
> > > [    8.239762]  [<ffffffff8029c838>] nameidata_to_filp+0x2e/0x40
> > > [    8.239762]  [<ffffffff8029c880>] do_filp_open+0x36/0x46
> > > [    8.239762]  [<ffffffff804a9da3>] ? _spin_unlock+0x26/0x2a
> > > [    8.239762]  [<ffffffff8029c5b1>] ? get_unused_fd_flags+0x113/0x121
> > > [    8.239762]  [<ffffffff8029c8e1>] do_sys_open+0x51/0xd2
> > > [    8.239762]  [<ffffffff8029c98b>] sys_open+0x1b/0x1d
> > > [    8.239762]  [<ffffffff8020bf7b>] system_call_after_swapgs+0x7b/0x80
> > 
> > Admittedly I don't understand all the code, but James, can you let me
> > know if this solves the problem?  I don't see this issue on my machine
> > and I don't know what opening a file with 'special' means.
> > 
> > /*
> >  * Note that while the flag value (low two bits) for sys_open means:
> >  *      00 - read-only
> >  *      01 - write-only
> >  *      10 - read-write
> >  *      11 - special
> >  * it is changed into
> >  *      00 - no permissions needed
> >  *      01 - read-permission
> >  *      10 - write-permission
> >  *      11 - read-write
> >  * for the internal routines (ie open_namei()/follow_link() etc). 00 is
> >  * used by symlinks.
> >  */
> > 
> > I noticed that the conversion of flags to f_mode in __dentry_open()
> > handles things a bit differently than do_filp_open().  This makes the
> > __dentry_open() handling like do_filp_open() so that 'special' gets
> > turned into RW instead of no permissions.
> > 
> > Time to go figure out what the heck 'special' means....
> > 
> > -Eric
> > 
> > ---
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 5419853..04a8efa 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -736,10 +736,15 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
> >  {
> >  	struct inode *inode;
> >  	int error;
> > +	mode_t f_mode;
> > +
> > +	if ((flags+1) & O_ACCMODE)
> > +		f_mode = (flags+1) & O_ACCMODE;
> > +	else
> > +		f_mode = flags & O_ACCMODE;
> >  
> >  	f->f_flags = flags;
> > -	f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
> > -				FMODE_PREAD | FMODE_PWRITE;
> > +	f->f_mode = f_mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> >  	inode = dentry->d_inode;
> >  	if (f->f_mode & FMODE_WRITE) {
> >  		error = get_write_access(inode);
> 
> Alternatively, if this is a valid state and is used internally by the
> kernel, then possibly selinux_dentry_open() should just return 0
> immediately if ((file->f_mode & (FMODE_READ | FMODE_WRITE) == 0), much
> as selinux_inode_permission() returns 0 if the mask is 0 (existence
> test).  Not sure though why we haven't encountered this before since we
> should have been getting denials even before Eric's patch.

We probably did start getting denials when we added the
security_dentry_open() call.  But it appears from my looking that this
is always a buggy program.  (I filed a bug against mdadm, number 437145)

Different paths, different flags:

sys_open
 do_sys_open
  do_filp_open
   open_namei
    vfs_permission
     permission
      security_inode_permission
       selinux_inode_permission

do_filp_open() has:
        if ((namei_flags+1) & O_ACCMODE)
                namei_flags++;
so flags of 11 get mapped to 11, which means read and write.


sys_open
 do_sys_open
  do_filp_open
   nameidata_to_filp
    __dentry_open
     security_dentry_open

dentry_open() has:
       f->f_mode = ((flags+1) & O_ACCMODE) | FMODE_LSEEK |
                               FMODE_PREAD | FMODE_PWRITE;
so flags of 11 get mapped to 00, which means no permission.

Before security_dentry_open I think we would have been fine.  Since then
I think buggy programs probably got a denial.  Maybe noone noticed with
mdadm since here is the code in question:

                close(0);
                open("/dev/null", 3);
                dup2(0,1);
                dup2(0,2);
                setsid();

Good error checking there....

I can't find anywhere in kernel that uses x & O_ACCMODE == O_ACCMODE;

And my latest patch seems to be booting find and not bugging the
security_dentry_open() call path.....

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.