* Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated
From: Mimi Zohar @ 2019-07-09 16:31 UTC (permalink / raw)
To: Jarkko Sakkinen, James Bottomley
Cc: Roberto Sassu, jgg, linux-integrity, linux-security-module,
keyrings, linux-kernel, crazyt2019+lml, tyhicks, nayna,
silviu.vlasceanu
In-Reply-To: <20190709162458.f4fjteokcmidv7w6@linux.intel.com>
On Tue, 2019-07-09 at 19:24 +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> > Not a criticism of your patch, but can we please stop doing this.
> > Single random number sources are horrendously bad practice because it
> > gives an attacker a single target to subvert. We should ensure the TPM
> > is plugged into the kernel RNG as a source and then take randomness
> > from the mixed pool so it's harder for an attacker because they have to
> > subvert all our sources to predict what came out.
>
> It is and I agree.
I still haven't quite figured out why the digests need to be
initialized to anything other than 0.
Mimi
^ permalink raw reply
* Re: RFC: BUG: overlayfs getxattr recursion leaves a poison sid.
From: Casey Schaufler @ 2019-07-09 16:33 UTC (permalink / raw)
To: Mark Salyzyn, linux-kernel, Stephen Smalley, Miklos Szeredi,
James Morris, Serge E. Hallyn, Paul Moore, Eric Paris, selinux,
kernel-team, Linux Security Module list
Cc: casey
In-Reply-To: <b5c3bc4a-eb39-d994-7723-947a464383a2@android.com>
On 7/9/2019 9:23 AM, Mark Salyzyn wrote:
> For EACCES return for getxattr, sid appears to be expected updated in parent node. For some accesses purely cosmetic for correct avc logging, and depending on kernel vintage for others (older than 4.4) the lack of the corrected sid in the parent overlay inode poisons the security cache and results in false denials.
>
> The avc denials would contain an (incorrect) unlabelled target references, we could fix this by copying up the sid to the parent inode. However the test (below) needs to refactored to the pleasure of the security, selinux and overlayfs maintainers. The security_socket_accept function is _close_, it will copy sid and class from the old socket to the new. Along those lines, we probably need to add a new security_copy_to_upper handler that takes the upper and lower dentries and ensures that the upper contains all the security information associated with the lower.
Please include the LSM (CCed) list on all LSM impacting discussions.
Your mailer mangled the patch. Please resend in plain text.
Thank you.
>
> Prototype adjustment (tested in 3.18 to ToT)
>
> int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name, { ssize_t res; const struct cred *old_cred; struct dentry *realdentry = ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry); old_cred = ovl_override_creds(dentry->d_sb); res = vfs_getxattr(realdentry, name, value, size); ovl_revert_creds(old_cred); + if (res == -EACCES) { + selinux_copy_sid(dentry, realdentry); return res; }
>
> . . .
>
> + void selinux_copy_sid(struct dentry *parent, struct dentry *child) + { + struct inode *pinode, *cinode; + struct inode_security_struct *pisec, *cisec; + + if (!parent || !child) + return; + pinode = parent->d_inode; + cinode = child->d_inode; + if (!pinode || !cinode) + return; + pisec = pinode->i_security; + cisec = cinode->i_security; + if (!pisec || !cisec) + return; + pisec->sid = cisec->sid; + } + EXPORT_SYMBOL_GPL(selinux_copy_sid);
>
> Sincerely -- Mark Salyzyn
>
>
^ permalink raw reply
* Re: RFC: BUG: overlayfs getxattr recursion leaves a poison sid.
From: Mark Salyzyn @ 2019-07-09 16:41 UTC (permalink / raw)
To: Casey Schaufler, linux-kernel, Stephen Smalley, Miklos Szeredi,
James Morris, Serge E. Hallyn, Paul Moore, Eric Paris, selinux,
kernel-team, Linux Security Module list
In-Reply-To: <7b6eb68e-44ae-5df8-9ebd-d334fc134938@schaufler-ca.com>
On 7/9/19 9:33 AM, Casey Schaufler wrote:
> On 7/9/2019 9:23 AM, Mark Salyzyn wrote:
>> For EACCES return for getxattr, sid appears to be expected updated in parent node. For some accesses purely cosmetic for correct avc logging, and depending on kernel vintage for others (older than 4.4) the lack of the corrected sid in the parent overlay inode poisons the security cache and results in false denials.
>>
>> The avc denials would contain an (incorrect) unlabelled target references, we could fix this by copying up the sid to the parent inode. However the test (below) needs to refactored to the pleasure of the security, selinux and overlayfs maintainers. The security_socket_accept function is _close_, it will copy sid and class from the old socket to the new. Along those lines, we probably need to add a new security_copy_to_upper handler that takes the upper and lower dentries and ensures that the upper contains all the security information associated with the lower.
> Please include the LSM (CCed) list on all LSM impacting discussions.
> Your mailer mangled the patch. Please resend in plain text.
>
> Thank you.
>
>> Prototype adjustment (tested in 3.18 to ToT)
(annoyed that Thunderbird let me down even after selecting text plain text)
int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const
char *name,
{
ssize_t res;
const struct cred *old_cred;
struct dentry *realdentry =
ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
old_cred = ovl_override_creds(dentry->d_sb);
res = vfs_getxattr(realdentry, name, value, size);
ovl_revert_creds(old_cred);
+ if (res == -EACCES)
+ selinux_copy_sid(dentry, realdentry);
return res;
}
. . .
+ void selinux_copy_sid(struct dentry *parent, struct dentry *child)
+ {
+ struct inode *pinode, *cinode;
+ struct inode_security_struct *pisec, *cisec;
+
+ if (!parent || !child)
+ return;
+ pinode = parent->d_inode;
+ cinode = child->d_inode;
+ if (!pinode || !cinode)
+ return;
+ pisec = pinode->i_security;
+ cisec = cinode->i_security;
+ if (!pisec || !cisec)
+ return;
+ pisec->sid = cisec->sid;
+ }
+ EXPORT_SYMBOL_GPL(selinux_copy_sid);
^ permalink raw reply
* Re: [PATCH v1 01/22] docs: Documentation/*.txt: rename all ReST files to *.rst
From: Rob Herring @ 2019-07-09 17:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, linux-fbdev, linux-ia64, kvm, linux-sh,
linux-pci, dri-devel, platform-driver-x86, kernel-hardening,
sparclinux, linux-arch, linux-s390, Jonathan Corbet, x86,
linux-security-module, devicetree, linux-watchdog,
Mauro Carvalho Chehab, linux-block, linux-gpio,
openipmi-developer, linux-arm-kernel, linaro-mm-sig, linux-parisc,
linux-mm, netdev, linux-wireless, linux-kernel, iommu,
linux-crypto
In-Reply-To: <6b6b6db8d6de9b66223dd6d4b43eb60ead4c71d7.1560891322.git.mchehab+samsung@kernel.org>
On Tue, Jun 18, 2019 at 06:05:25PM -0300, Mauro Carvalho Chehab wrote:
> Those files are actually at ReST format. Ok, currently, they
> don't belong to any place yet at the organized book series,
> but we don't want patches to break them as ReST files. So,
> rename them and add a :orphan: in order to shut up warning
> messages like those:
>
> ...
> Documentation/svga.rst: WARNING: document isn't included in any toctree
> Documentation/switchtec.rst: WARNING: document isn't included in any toctree
> ...
>
> Later patches will move them to a better place and remove the
> :orphan: markup.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>
> I had to remove the long list of maintainers got by
> getpatch.pl, as it was too long. I opted to keep only the
> mailing lists.
>
> Documentation/ABI/removed/sysfs-class-rfkill | 2 +-
> Documentation/ABI/stable/sysfs-class-rfkill | 2 +-
> Documentation/ABI/stable/sysfs-devices-node | 2 +-
> Documentation/ABI/testing/procfs-diskstats | 2 +-
> Documentation/ABI/testing/sysfs-block | 2 +-
> .../ABI/testing/sysfs-class-switchtec | 2 +-
> .../ABI/testing/sysfs-devices-system-cpu | 4 +-
> .../{DMA-API-HOWTO.txt => DMA-API-HOWTO.rst} | 2 +
> Documentation/{DMA-API.txt => DMA-API.rst} | 8 ++-
> .../{DMA-ISA-LPC.txt => DMA-ISA-LPC.rst} | 4 +-
> ...{DMA-attributes.txt => DMA-attributes.rst} | 2 +
> Documentation/{IPMI.txt => IPMI.rst} | 2 +
> .../{IRQ-affinity.txt => IRQ-affinity.rst} | 2 +
> .../{IRQ-domain.txt => IRQ-domain.rst} | 2 +
> Documentation/{IRQ.txt => IRQ.rst} | 2 +
> .../{Intel-IOMMU.txt => Intel-IOMMU.rst} | 2 +
> Documentation/PCI/pci.rst | 8 +--
> Documentation/{SAK.txt => SAK.rst} | 3 +-
> Documentation/{SM501.txt => SM501.rst} | 2 +
> Documentation/admin-guide/hw-vuln/l1tf.rst | 2 +-
> .../admin-guide/kernel-parameters.txt | 4 +-
> .../{atomic_bitops.txt => atomic_bitops.rst} | 3 +-
> Documentation/block/biodoc.txt | 2 +-
> .../{bt8xxgpio.txt => bt8xxgpio.rst} | 3 +-
> Documentation/{btmrvl.txt => btmrvl.rst} | 2 +
> ...-mapping.txt => bus-virt-phys-mapping.rst} | 54 +++++++++---------
> ...g-warn-once.txt => clearing-warn-once.rst} | 2 +
> Documentation/{cpu-load.txt => cpu-load.rst} | 2 +
> .../{cputopology.txt => cputopology.rst} | 2 +
> Documentation/{crc32.txt => crc32.rst} | 2 +
> Documentation/{dcdbas.txt => dcdbas.rst} | 2 +
> ...ging-modules.txt => debugging-modules.rst} | 2 +
> ...hci1394.txt => debugging-via-ohci1394.rst} | 2 +
> Documentation/{dell_rbu.txt => dell_rbu.rst} | 3 +-
> Documentation/device-mapper/statistics.rst | 4 +-
> .../devicetree/bindings/phy/phy-bindings.txt | 2 +-
Acked-by: Rob Herring <robh@kernel.org>
> Documentation/{digsig.txt => digsig.rst} | 2 +
> Documentation/driver-api/usb/dma.rst | 6 +-
> Documentation/driver-model/device.rst | 2 +-
> Documentation/{efi-stub.txt => efi-stub.rst} | 2 +
> Documentation/{eisa.txt => eisa.rst} | 2 +
> Documentation/fb/vesafb.rst | 2 +-
> Documentation/filesystems/sysfs.txt | 2 +-
> ...ex-requeue-pi.txt => futex-requeue-pi.rst} | 2 +
> .../{gcc-plugins.txt => gcc-plugins.rst} | 2 +
> Documentation/gpu/drm-mm.rst | 2 +-
> Documentation/{highuid.txt => highuid.rst} | 4 +-
> .../{hw_random.txt => hw_random.rst} | 2 +
> .../{hwspinlock.txt => hwspinlock.rst} | 2 +
> Documentation/ia64/irq-redir.rst | 2 +-
> .../{intel_txt.txt => intel_txt.rst} | 2 +
> .../{io-mapping.txt => io-mapping.rst} | 2 +
> .../{io_ordering.txt => io_ordering.rst} | 2 +
> Documentation/{iostats.txt => iostats.rst} | 2 +
> ...flags-tracing.txt => irqflags-tracing.rst} | 3 +-
> Documentation/{isa.txt => isa.rst} | 2 +
> Documentation/{isapnp.txt => isapnp.rst} | 2 +
> ...hreads.txt => kernel-per-CPU-kthreads.rst} | 4 +-
> Documentation/{kobject.txt => kobject.rst} | 6 +-
> Documentation/{kprobes.txt => kprobes.rst} | 3 +-
> Documentation/{kref.txt => kref.rst} | 2 +
> Documentation/laptops/thinkpad-acpi.rst | 6 +-
> Documentation/{ldm.txt => ldm.rst} | 5 +-
> Documentation/locking/rt-mutex.rst | 2 +-
> ...kup-watchdogs.txt => lockup-watchdogs.rst} | 2 +
> Documentation/{lsm.txt => lsm.rst} | 2 +
> Documentation/{lzo.txt => lzo.rst} | 2 +
> Documentation/{mailbox.txt => mailbox.rst} | 2 +
> Documentation/memory-barriers.txt | 6 +-
> ...hameleon-bus.txt => men-chameleon-bus.rst} | 2 +
> Documentation/networking/scaling.rst | 4 +-
> .../{nommu-mmap.txt => nommu-mmap.rst} | 2 +
> Documentation/{ntb.txt => ntb.rst} | 2 +
> Documentation/{numastat.txt => numastat.rst} | 3 +-
> Documentation/{padata.txt => padata.rst} | 2 +
> ...port-lowlevel.txt => parport-lowlevel.rst} | 2 +
> ...-semaphore.txt => percpu-rw-semaphore.rst} | 2 +
> Documentation/{phy.txt => phy.rst} | 2 +
> Documentation/{pi-futex.txt => pi-futex.rst} | 2 +
> Documentation/{pnp.txt => pnp.rst} | 13 +++--
> ...reempt-locking.txt => preempt-locking.rst} | 4 +-
> Documentation/{pwm.txt => pwm.rst} | 2 +
> Documentation/{rbtree.txt => rbtree.rst} | 54 +++++++++---------
> .../{remoteproc.txt => remoteproc.rst} | 4 +-
> Documentation/{rfkill.txt => rfkill.rst} | 2 +
> ...ust-futex-ABI.txt => robust-futex-ABI.rst} | 2 +
> ...{robust-futexes.txt => robust-futexes.rst} | 2 +
> Documentation/{rpmsg.txt => rpmsg.rst} | 2 +
> Documentation/{rtc.txt => rtc.rst} | 8 ++-
> Documentation/s390/vfio-ccw.rst | 6 +-
> Documentation/{sgi-ioc4.txt => sgi-ioc4.rst} | 2 +
> Documentation/{siphash.txt => siphash.rst} | 2 +
> .../{smsc_ece1099.txt => smsc_ece1099.rst} | 2 +
> .../{speculation.txt => speculation.rst} | 2 +
> .../{static-keys.txt => static-keys.rst} | 2 +
> Documentation/{svga.txt => svga.rst} | 2 +
> .../{switchtec.txt => switchtec.rst} | 4 +-
> .../{sync_file.txt => sync_file.rst} | 2 +
> Documentation/sysctl/kernel.txt | 4 +-
> Documentation/sysctl/vm.txt | 2 +-
> Documentation/{tee.txt => tee.rst} | 2 +
> .../{this_cpu_ops.txt => this_cpu_ops.rst} | 2 +
> Documentation/trace/kprobetrace.rst | 2 +-
> .../translations/ko_KR/memory-barriers.txt | 6 +-
> Documentation/translations/zh_CN/IRQ.txt | 4 +-
> .../translations/zh_CN/filesystems/sysfs.txt | 2 +-
> .../translations/zh_CN/io_ordering.txt | 4 +-
> ...access.txt => unaligned-memory-access.rst} | 2 +
> ...ed-device.txt => vfio-mediated-device.rst} | 4 +-
> Documentation/{vfio.txt => vfio.rst} | 2 +
> .../{video-output.txt => video-output.rst} | 3 +-
> Documentation/watchdog/hpwdt.rst | 2 +-
> Documentation/x86/topology.rst | 2 +-
> Documentation/{xillybus.txt => xillybus.rst} | 2 +
> Documentation/{xz.txt => xz.rst} | 2 +
> Documentation/{zorro.txt => zorro.rst} | 7 ++-
> MAINTAINERS | 56 +++++++++----------
> arch/Kconfig | 4 +-
> arch/arm/Kconfig | 2 +-
> arch/ia64/hp/common/sba_iommu.c | 12 ++--
> arch/ia64/sn/pci/pci_dma.c | 4 +-
> arch/parisc/Kconfig | 2 +-
> arch/parisc/kernel/pci-dma.c | 2 +-
> arch/sh/Kconfig | 2 +-
> arch/sparc/Kconfig | 2 +-
> arch/unicore32/include/asm/io.h | 2 +-
> arch/x86/Kconfig | 4 +-
> arch/x86/include/asm/dma-mapping.h | 4 +-
> arch/x86/kernel/amd_gart_64.c | 2 +-
> block/partitions/Kconfig | 2 +-
> drivers/base/core.c | 2 +-
> drivers/char/Kconfig | 4 +-
> drivers/char/hw_random/core.c | 2 +-
> drivers/char/ipmi/Kconfig | 2 +-
> drivers/char/ipmi/ipmi_si_hotmod.c | 2 +-
> drivers/char/ipmi/ipmi_si_intf.c | 2 +-
> drivers/dma-buf/Kconfig | 2 +-
> drivers/gpio/Kconfig | 2 +-
> drivers/parisc/sba_iommu.c | 16 +++---
> drivers/pci/switch/Kconfig | 2 +-
> drivers/platform/x86/Kconfig | 4 +-
> drivers/platform/x86/dcdbas.c | 2 +-
> drivers/platform/x86/dell_rbu.c | 2 +-
> drivers/pnp/isapnp/Kconfig | 2 +-
> drivers/vfio/Kconfig | 2 +-
> drivers/vfio/mdev/Kconfig | 2 +-
> include/asm-generic/bitops/atomic.h | 2 +-
> include/linux/dma-mapping.h | 2 +-
> include/linux/hw_random.h | 2 +-
> include/linux/io-mapping.h | 2 +-
> include/linux/jump_label.h | 2 +-
> include/linux/kobject.h | 2 +-
> include/linux/kobject_ns.h | 2 +-
> include/linux/rbtree.h | 2 +-
> include/linux/rbtree_augmented.h | 2 +-
> include/media/videobuf-dma-sg.h | 2 +-
> init/Kconfig | 2 +-
> kernel/dma/debug.c | 2 +-
> kernel/padata.c | 2 +-
> lib/Kconfig | 2 +-
> lib/Kconfig.debug | 2 +-
> lib/crc32.c | 2 +-
> lib/kobject.c | 4 +-
> lib/lzo/lzo1x_decompress_safe.c | 2 +-
> lib/xz/Kconfig | 2 +-
> mm/Kconfig | 2 +-
> mm/nommu.c | 2 +-
> samples/kprobes/kprobe_example.c | 2 +-
> samples/kprobes/kretprobe_example.c | 2 +-
> scripts/gcc-plugins/Kconfig | 2 +-
> security/Kconfig | 2 +-
> tools/include/linux/rbtree.h | 2 +-
> tools/include/linux/rbtree_augmented.h | 2 +-
> 173 files changed, 397 insertions(+), 242 deletions(-)
> rename Documentation/{DMA-API-HOWTO.txt => DMA-API-HOWTO.rst} (99%)
> rename Documentation/{DMA-API.txt => DMA-API.rst} (99%)
> rename Documentation/{DMA-ISA-LPC.txt => DMA-ISA-LPC.rst} (98%)
> rename Documentation/{DMA-attributes.txt => DMA-attributes.rst} (99%)
> rename Documentation/{IPMI.txt => IPMI.rst} (99%)
> rename Documentation/{IRQ-affinity.txt => IRQ-affinity.rst} (99%)
> rename Documentation/{IRQ-domain.txt => IRQ-domain.rst} (99%)
> rename Documentation/{IRQ.txt => IRQ.rst} (99%)
> rename Documentation/{Intel-IOMMU.txt => Intel-IOMMU.rst} (99%)
> rename Documentation/{SAK.txt => SAK.rst} (99%)
> rename Documentation/{SM501.txt => SM501.rst} (99%)
> rename Documentation/{atomic_bitops.txt => atomic_bitops.rst} (99%)
> rename Documentation/{bt8xxgpio.txt => bt8xxgpio.rst} (99%)
> rename Documentation/{btmrvl.txt => btmrvl.rst} (99%)
> rename Documentation/{bus-virt-phys-mapping.txt => bus-virt-phys-mapping.rst} (93%)
> rename Documentation/{clearing-warn-once.txt => clearing-warn-once.rst} (96%)
> rename Documentation/{cpu-load.txt => cpu-load.rst} (99%)
> rename Documentation/{cputopology.txt => cputopology.rst} (99%)
> rename Documentation/{crc32.txt => crc32.rst} (99%)
> rename Documentation/{dcdbas.txt => dcdbas.rst} (99%)
> rename Documentation/{debugging-modules.txt => debugging-modules.rst} (98%)
> rename Documentation/{debugging-via-ohci1394.txt => debugging-via-ohci1394.rst} (99%)
> rename Documentation/{dell_rbu.txt => dell_rbu.rst} (99%)
> rename Documentation/{digsig.txt => digsig.rst} (99%)
> rename Documentation/{efi-stub.txt => efi-stub.rst} (99%)
> rename Documentation/{eisa.txt => eisa.rst} (99%)
> rename Documentation/{futex-requeue-pi.txt => futex-requeue-pi.rst} (99%)
> rename Documentation/{gcc-plugins.txt => gcc-plugins.rst} (99%)
> rename Documentation/{highuid.txt => highuid.rst} (99%)
> rename Documentation/{hw_random.txt => hw_random.rst} (99%)
> rename Documentation/{hwspinlock.txt => hwspinlock.rst} (99%)
> rename Documentation/{intel_txt.txt => intel_txt.rst} (99%)
> rename Documentation/{io-mapping.txt => io-mapping.rst} (99%)
> rename Documentation/{io_ordering.txt => io_ordering.rst} (99%)
> rename Documentation/{iostats.txt => iostats.rst} (99%)
> rename Documentation/{irqflags-tracing.txt => irqflags-tracing.rst} (99%)
> rename Documentation/{isa.txt => isa.rst} (99%)
> rename Documentation/{isapnp.txt => isapnp.rst} (98%)
> rename Documentation/{kernel-per-CPU-kthreads.txt => kernel-per-CPU-kthreads.rst} (99%)
> rename Documentation/{kobject.txt => kobject.rst} (99%)
> rename Documentation/{kprobes.txt => kprobes.rst} (99%)
> rename Documentation/{kref.txt => kref.rst} (99%)
> rename Documentation/{ldm.txt => ldm.rst} (98%)
> rename Documentation/{lockup-watchdogs.txt => lockup-watchdogs.rst} (99%)
> rename Documentation/{lsm.txt => lsm.rst} (99%)
> rename Documentation/{lzo.txt => lzo.rst} (99%)
> rename Documentation/{mailbox.txt => mailbox.rst} (99%)
> rename Documentation/{men-chameleon-bus.txt => men-chameleon-bus.rst} (99%)
> rename Documentation/{nommu-mmap.txt => nommu-mmap.rst} (99%)
> rename Documentation/{ntb.txt => ntb.rst} (99%)
> rename Documentation/{numastat.txt => numastat.rst} (99%)
> rename Documentation/{padata.txt => padata.rst} (99%)
> rename Documentation/{parport-lowlevel.txt => parport-lowlevel.rst} (99%)
> rename Documentation/{percpu-rw-semaphore.txt => percpu-rw-semaphore.rst} (99%)
> rename Documentation/{phy.txt => phy.rst} (99%)
> rename Documentation/{pi-futex.txt => pi-futex.rst} (99%)
> rename Documentation/{pnp.txt => pnp.rst} (98%)
> rename Documentation/{preempt-locking.txt => preempt-locking.rst} (99%)
> rename Documentation/{pwm.txt => pwm.rst} (99%)
> rename Documentation/{rbtree.txt => rbtree.rst} (94%)
> rename Documentation/{remoteproc.txt => remoteproc.rst} (99%)
> rename Documentation/{rfkill.txt => rfkill.rst} (99%)
> rename Documentation/{robust-futex-ABI.txt => robust-futex-ABI.rst} (99%)
> rename Documentation/{robust-futexes.txt => robust-futexes.rst} (99%)
> rename Documentation/{rpmsg.txt => rpmsg.rst} (99%)
> rename Documentation/{rtc.txt => rtc.rst} (99%)
> rename Documentation/{sgi-ioc4.txt => sgi-ioc4.rst} (99%)
> rename Documentation/{siphash.txt => siphash.rst} (99%)
> rename Documentation/{smsc_ece1099.txt => smsc_ece1099.rst} (99%)
> rename Documentation/{speculation.txt => speculation.rst} (99%)
> rename Documentation/{static-keys.txt => static-keys.rst} (99%)
> rename Documentation/{svga.txt => svga.rst} (99%)
> rename Documentation/{switchtec.txt => switchtec.rst} (98%)
> rename Documentation/{sync_file.txt => sync_file.rst} (99%)
> rename Documentation/{tee.txt => tee.rst} (99%)
> rename Documentation/{this_cpu_ops.txt => this_cpu_ops.rst} (99%)
> rename Documentation/{unaligned-memory-access.txt => unaligned-memory-access.rst} (99%)
> rename Documentation/{vfio-mediated-device.txt => vfio-mediated-device.rst} (99%)
> rename Documentation/{vfio.txt => vfio.rst} (99%)
> rename Documentation/{video-output.txt => video-output.rst} (99%)
> rename Documentation/{xillybus.txt => xillybus.rst} (99%)
> rename Documentation/{xz.txt => xz.rst} (99%)
> rename Documentation/{zorro.txt => zorro.rst} (99%)
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-07-09 17:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190709162203.gzyvulah5u7eksip@linux.intel.com>
On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > >
> > > I still don't get why we need this whole mess and do not simply admit
> > > that there are two distinct roles:
> > >
> > > 1. Creator
> > > 2. User
> >
> > Because SELinux has existing concepts of EXECMEM and EXECMOD.
>
> What is the official documentation for those? I've only found some
> explanations from discussions and some RHEL sysadmin guides.
No clue. My knowledge was gleaned from the code and from Stephen's
feedback.
The high level breakdown:
- FILE__EXECUTE: required to gain X on a mapping to a regular file
- FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared
mapping to a regular file
- FILE__EXECMOD: required to gain W->X on a private mapping of a regular
file
- PROCESS__EXECMEM: required to gain WX on a private mapping to a regular
file, OR to gain X on an anonymous mapping.
Translating those to SGX, with a lot of input from Stephen, I ended up
with the following:
- FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
on an enclave page loaded from a regular file
- PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
required to gain W->X on an enclave page
- PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
an enclave page that is loaded from an
anonymous mapping
- PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
enclave page
> > That being said, we can do so without functional changes to the SGX uapi,
> > e.g. add reserved fields so that the initial uapi can be extended *if* we
> > decide to go with the "userspace provides maximal protections" path, and
> > use the EPCM permissions as the maximal protections for the initial
> > upstreaming.
> >
> > That'd give us a minimal implemenation for initial upstreaming and would
> > eliminate Cedric's blocking complaint. The "whole mess" of whitelisting,
> > blacklisting and SGX2 support would be deferred until post-upstreaming.
>
> I'd like that approach more too.
>
> /Jarkko
^ permalink raw reply
* Re: [PATCH v5 15/23] LSM: Specify which LSM to display
From: Stephen Smalley @ 2019-07-09 17:13 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190703212538.7383-16-casey@schaufler-ca.com>
On 7/3/19 5:25 PM, Casey Schaufler wrote:
> Create a new entry "display" in /proc/.../attr for controlling
> which LSM security information is displayed for a process.
> The name of an active LSM that supplies hooks for human readable
> data may be written to "display" to set the value. The name of
> the LSM currently in use can be read from "display".
> At this point there can only be one LSM capable of display
> active. A helper function lsm_task_display() to get the display
> slot for a task_struct.
As I explained previously, this is a security hole waiting to happen.
It still permits a process to affect the output of audit, alter the
result of reading or writing /proc/self/attr nodes even by
setuid/setgid/file-caps/context-changing programs, alter the contexts
generated in netlink messages delivered to other processes (I think?),
and possibly other effects beyond affecting the process' own view of things.
Before:
$ id
uid=1002(sds2) gid=1002(sds2) groups=1002(sds2)
context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
$ su
Password:
su: Authentication failure
syscall audit record:
type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64
syscall=openat
success=no exit=EACCES(Permission denied) a0=0xffffff9c
a1=0x560897e58e00 a2=O_
WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2
euid=root s
uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6
comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023
key=(null)
After:
$ id
uid=1002(sds2) gid=1002(sds2) groups=1002(sds2)
context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
$ echo apparmor > /proc/self/attr/display
$ su
Password:
su: Authentication failure
audit record:
type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64
syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c
a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426
auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2
sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su
subj==unconfined key=(null)
NB The subj= field of the SYSCALL audit record is longer accurate and is
potentially under the control of a process that would not be authorized
to set its subject label to that value by SELinux.
Now, let's play with userspace.
Before:
# id
uid=0(root) gid=0(root) groups=0(root)
context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
# passwd root
passwd: SELinux deny access due to security policy.
audit record:
type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693
uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023
msg='avc: denied { passwd } for
scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023
tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd
permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=?
terminal=pts/2'
type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693
uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023
msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd
hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
After:
# id
uid=0(root) gid=0(root) groups=0(root)
context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
# echo apparmor > /proc/self/attr/display
# passwd root
passwd: SELinux deny access due to security policy.
audit record:
type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083
uid=root auid=sds2 ses=7 subj==unconfined
msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd
hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2
res=failed'
Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit
record, and we further lose the USER_AVC record entirely because it
didn't even reach the point of the permission check due to not being
able to get the caller context.
The situation gets worse if the caller can set things up such that it
can set an attribute value for one security module that is valid and
privileged with respect to another security module. This isn't a
far-fetched scenario; AppArmor will default to running everything
unconfined, so as soon as you enable it, any root process can
potentially load a policy that defines contexts that look exactly like
SELinux contexts. Smack is even simpler; you can set any arbitrary
string you want as long as you are root (by default); no policy
required. So a root process that is confined by SELinux (or by AppAmor)
can suddenly forge arbitrary contexts in audit records or reads of
/proc/self/attr nodes or netlink messages or ..., just by virtue of
applying these patches and enabling another security module like Smack.
Or consider if ptags were ever made real and merged - by design, that's
all about setting arbitrary tags from userspace. Then there is the
separate issue of switching display to prevent attempts by a more
privileged program to set one of its attributes from taking effect.
Where have we seen that before - sendmail capabilities bug anyone? And
it is actually worse than that bug, because with the assistance of a
friendly security module, the write may actually succeed; it just won't
alter the SELinux context of the program or anything it creates!
This gets a NAK from me so long as it has these issues and setting the
display remains outside the control of any security module.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/proc/base.c | 1 +
> include/linux/lsm_hooks.h | 13 ++++
> security/security.c | 129 +++++++++++++++++++++++++++++++++-----
> 3 files changed, 126 insertions(+), 17 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ddef482f1334..7bf70e041315 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "fscreate", 0666),
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> + ATTR(NULL, "display", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index fe1fb7a69ee5..88ec3f3487ae 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2134,4 +2134,17 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
>
> extern int lsm_inode_alloc(struct inode *inode);
>
> +/**
> + * lsm_task_display - the "display LSM for this task
> + * @task: The task to report on
> + *
> + * Returns the task's display LSM slot.
> + */
> +static inline int lsm_task_display(struct task_struct *task)
> +{
> + int *display = task->security;
> +
> + return *display;
> +}
> +
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index 8927508b2142..f3a293e6ef5a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
>
> char *lsm_names;
> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
> + .lbs_task = sizeof(int), /* slot number for the "display" LSM */
> +};
>
> /* Boot-time LSM user choice */
> static __initdata const char *chosen_lsm_order;
> @@ -423,8 +425,10 @@ static int lsm_append(const char *new, char **result)
>
> /*
> * Current index to use while initializing the lsmblob secid list.
> + * Pointers to the LSM id structures for local use.
> */
> static int lsm_slot __lsm_ro_after_init;
> +static struct lsm_id *lsm_slotlist[LSMBLOB_ENTRIES];
>
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> @@ -444,6 +448,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> if (lsmid->slot == LSMBLOB_NEEDED) {
> if (lsm_slot >= LSMBLOB_ENTRIES)
> panic("%s Too many LSMs registered.\n", __func__);
> + lsm_slotlist[lsm_slot] = lsmid;
> lsmid->slot = lsm_slot++;
> init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
> lsmid->slot);
> @@ -564,6 +569,8 @@ int lsm_inode_alloc(struct inode *inode)
> */
> static int lsm_task_alloc(struct task_struct *task)
> {
> + int *display;
> +
> if (blob_sizes.lbs_task == 0) {
> task->security = NULL;
> return 0;
> @@ -572,6 +579,15 @@ static int lsm_task_alloc(struct task_struct *task)
> task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
> if (task->security == NULL)
> return -ENOMEM;
> +
> + /*
> + * The start of the task blob contains the "display" LSM slot number.
> + * Start with it set to the invalid slot number, indicating that the
> + * default first registered LSM be displayed.
> + */
> + display = task->security;
> + *display = LSMBLOB_INVALID;
> +
> return 0;
> }
>
> @@ -1563,14 +1579,24 @@ int security_file_open(struct file *file)
>
> int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> {
> + int *odisplay = current->security;
> + int *ndisplay;
> int rc = lsm_task_alloc(task);
>
> - if (rc)
> + if (unlikely(rc))
> return rc;
> +
> rc = call_int_hook(task_alloc, 0, task, clone_flags);
> - if (unlikely(rc))
> + if (unlikely(rc)) {
> security_task_free(task);
> - return rc;
> + return rc;
> + }
> +
> + ndisplay = task->security;
> + if (ndisplay && odisplay)
> + *ndisplay = *odisplay;
> +
> + return 0;
> }
>
> void security_task_free(struct task_struct *task)
> @@ -1967,10 +1993,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + int display = lsm_task_display(current);
> + int slot = 0;
> +
> + if (!strcmp(name, "display")) {
> + /*
> + * lsm_slot will be 0 if there are no displaying modules.
> + */
> + if (lsm_slot == 0)
> + return -EINVAL;
> + if (display != LSMBLOB_INVALID)
> + slot = display;
> + *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL);
> + if (*value)
> + return strlen(*value);
> + return -ENOMEM;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> + if (lsm == NULL && display != LSMBLOB_INVALID &&
> + display != hp->lsmid->slot)
> + continue;
> return hp->hook.getprocattr(p, name, value);
> }
> return -EINVAL;
> @@ -1980,10 +2025,46 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size)
> {
> struct security_hook_list *hp;
> + char *term;
> + char *cp;
> + int *display = current->security;
> + int rc = -EINVAL;
> + int slot = 0;
> +
> + if (!strcmp(name, "display")) {
> + /*
> + * lsm_slot will be 0 if there are no displaying modules.
> + */
> + if (lsm_slot == 0 || size == 0)
> + return -EINVAL;
> + cp = kzalloc(size + 1, GFP_KERNEL);
> + if (cp == NULL)
> + return -ENOMEM;
> + memcpy(cp, value, size);
> +
> + term = strchr(cp, ' ');
> + if (term == NULL)
> + term = strchr(cp, '\n');
> + if (term != NULL)
> + *term = '\0';
> +
> + for (slot = 0; slot < lsm_slot; slot++)
> + if (!strcmp(cp, lsm_slotlist[slot]->lsm)) {
> + *display = lsm_slotlist[slot]->slot;
> + rc = size;
> + break;
> + }
> +
> + kfree(cp);
> + return rc;
> + }
>
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> + if (lsm == NULL && *display != LSMBLOB_INVALID &&
> + *display != hp->lsmid->slot)
> + continue;
> return hp->hook.setprocattr(name, value, size);
> }
> return -EINVAL;
> @@ -2003,15 +2084,15 @@ EXPORT_SYMBOL(security_ismaclabel);
> int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
> {
> struct security_hook_list *hp;
> - int rc;
> + int display = lsm_task_display(current);
>
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> - rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot],
> - secdata, seclen);
> - if (rc != 0)
> - return rc;
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.secid_to_secctx(
> + blob->secid[hp->lsmid->slot],
> + secdata, seclen);
> }
> return 0;
> }
> @@ -2021,16 +2102,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob)
> {
> struct security_hook_list *hp;
> - int rc;
> + int display = lsm_task_display(current);
>
> lsmblob_init(blob, 0);
> hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> continue;
> - rc = hp->hook.secctx_to_secid(secdata, seclen,
> - &blob->secid[hp->lsmid->slot]);
> - if (rc != 0)
> - return rc;
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.secctx_to_secid(secdata, seclen,
> + &blob->secid[hp->lsmid->slot]);
> }
> return 0;
> }
> @@ -2038,7 +2118,15 @@ EXPORT_SYMBOL(security_secctx_to_secid);
>
> void security_release_secctx(char *secdata, u32 seclen)
> {
> - call_void_hook(release_secctx, secdata, seclen);
> + struct security_hook_list *hp;
> + int *display = current->security;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> + if (*display == LSMBLOB_INVALID ||
> + *display == hp->lsmid->slot) {
> + hp->hook.release_secctx(secdata, seclen);
> + return;
> + }
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> @@ -2163,8 +2251,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len)
> {
> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> - optval, optlen, len);
> + int display = lsm_task_display(current);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
> + list)
> + if (display == LSMBLOB_INVALID || display == hp->lsmid->slot)
> + return hp->hook.socket_getpeersec_stream(sock, optval,
> + optlen, len);
> + return -ENOPROTOOPT;
> }
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>
^ permalink raw reply
* Re: [PATCH v5 15/23] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-07-09 17:51 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, casey
In-Reply-To: <6f593ae9-4c79-7a04-41a3-c04ebd689658@tycho.nsa.gov>
On 7/9/2019 10:13 AM, Stephen Smalley wrote:
> On 7/3/19 5:25 PM, Casey Schaufler wrote:
>> Create a new entry "display" in /proc/.../attr for controlling
>> which LSM security information is displayed for a process.
>> The name of an active LSM that supplies hooks for human readable
>> data may be written to "display" to set the value. The name of
>> the LSM currently in use can be read from "display".
>> At this point there can only be one LSM capable of display
>> active. A helper function lsm_task_display() to get the display
>> slot for a task_struct.
>
> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things.
I would very much like some feedback regarding which of the
possible formats for putting multiple subject contexts in
audit records would be preferred:
lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy
lsm=selinux,smack subj=xyzzy_t,Xyzzy
subj="selinux='xyzzy_t',smack='Xyzzy'"
Or something else. Free bikeshedding!
I don't see how you have a problem with netlink. My look
at what's in the kernel didn't expose anything, but I am
willing to be educated.
>
> Before:
> $ id
> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
> $ su
> Password:
> su: Authentication failure
>
> syscall audit record:
> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat
> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_
> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s
> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null)
>
> After:
> $ id
> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
> $ echo apparmor > /proc/self/attr/display
> $ su
> Password:
> su: Authentication failure
>
> audit record:
> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null)
>
> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux.
It's still accurate, it's just not complete. It's a matter
of how best to complete it.
>
> Now, let's play with userspace.
>
> Before:
> # id
> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
> # passwd root
> passwd: SELinux deny access due to security policy.
>
> audit record:
> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2'
> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>
> After:
> # id
> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
> # echo apparmor > /proc/self/attr/display
> # passwd root
> passwd: SELinux deny access due to security policy.
>
> audit record:
> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>
> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context.
>
> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching
> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates!
>
> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module.
The issues you've raised around audit are meritorious.
Any suggestions regarding how to address them would be
quite welcome.
As far as the general objection to the display mechanism,
I am eager to understand what you might propose as an
alternative. We can't dismiss backward compatibility for
any of the modules. We can't preclude any module combination.
We can require user space changes for configurations that
were impossible before, just as the addition of SELinux to
a system required user space changes. Update libselinux
to check the display before using the attr interfaces and
you've addressed most of the issues.
^ permalink raw reply
* Re: [PATCH v5 15/23] LSM: Specify which LSM to display
From: Stephen Smalley @ 2019-07-09 18:12 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul,
linux-audit@redhat.com
In-Reply-To: <a3537a96-84d7-ad82-f76e-af0f44331c1b@schaufler-ca.com>
On 7/9/19 1:51 PM, Casey Schaufler wrote:
> On 7/9/2019 10:13 AM, Stephen Smalley wrote:
>> On 7/3/19 5:25 PM, Casey Schaufler wrote:
>>> Create a new entry "display" in /proc/.../attr for controlling
>>> which LSM security information is displayed for a process.
>>> The name of an active LSM that supplies hooks for human readable
>>> data may be written to "display" to set the value. The name of
>>> the LSM currently in use can be read from "display".
>>> At this point there can only be one LSM capable of display
>>> active. A helper function lsm_task_display() to get the display
>>> slot for a task_struct.
>>
>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things.
>
> I would very much like some feedback regarding which of the
> possible formats for putting multiple subject contexts in
> audit records would be preferred:
>
> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy
> lsm=selinux,smack subj=xyzzy_t,Xyzzy
> subj="selinux='xyzzy_t',smack='Xyzzy'"
(cc'd linux-audit mailing list)
>
> Or something else. Free bikeshedding!
>
> I don't see how you have a problem with netlink. My look
> at what's in the kernel didn't expose anything, but I am
> willing to be educated.
I haven't traced through it in detail, but it wasn't clear to me that
the security_secid_to_secctx() call always occurs in the context of the
receiving process (and hence use its display value). If not, then the
display of the sender can affect what is reported to the receiver;
hence, there is a forgery concern similar to the binder issue. It would
be cleaner if we didn't alter the default behavior of
security_secid_to_secctx() and security_secctx_to_secid() and instead
introduced new hooks for any case where we truly want the display to
take effect.
>
>>
>> Before:
>> $ id
>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>> $ su
>> Password:
>> su: Authentication failure
>>
>> syscall audit record:
>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat
>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_
>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s
>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null)
>>
>> After:
>> $ id
>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>> $ echo apparmor > /proc/self/attr/display
>> $ su
>> Password:
>> su: Authentication failure
>>
>> audit record:
>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null)
>>
>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux.
>
> It's still accurate, it's just not complete. It's a matter
> of how best to complete it.
>
>>
>> Now, let's play with userspace.
>>
>> Before:
>> # id
>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>> # passwd root
>> passwd: SELinux deny access due to security policy.
>>
>> audit record:
>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2'
>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>
>> After:
>> # id
>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>> # echo apparmor > /proc/self/attr/display
>> # passwd root
>> passwd: SELinux deny access due to security policy.
>>
>> audit record:
>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>
>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context.
>>
>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching
>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates!
>>
>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module.
>
> The issues you've raised around audit are meritorious.
> Any suggestions regarding how to address them would be
> quite welcome.
>
> As far as the general objection to the display mechanism,
> I am eager to understand what you might propose as an
> alternative. We can't dismiss backward compatibility for
> any of the modules. We can't preclude any module combination.
>
> We can require user space changes for configurations that
> were impossible before, just as the addition of SELinux to
> a system required user space changes. Update libselinux
> to check the display before using the attr interfaces and
> you've addressed most of the issues.
Either we ensure that setting of the display can only affect processes
in the same security equivalence class (same credentials) or the
security modules need to be able to control who can set the display. Or
both.
^ permalink raw reply
* Reminder: 2 open syzbot bugs in "security/integrity" subsystem
From: Eric Biggers @ 2019-07-09 19:15 UTC (permalink / raw)
To: linux-integrity, linux-security-module, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: linux-kernel, syzkaller-bugs
[This email was generated by a script. Let me know if you have any suggestions
to make it better, or if you want it re-generated with the latest status.]
Of the currently open syzbot reports against the upstream kernel, I've manually
marked 2 of them as possibly being bugs in the "security/integrity" subsystem.
I've listed these reports below, sorted by an algorithm that tries to list first
the reports most likely to be still valid, important, and actionable.
If you believe a bug is no longer valid, please close the syzbot report by
sending a '#syz fix', '#syz dup', or '#syz invalid' command in reply to the
original thread, as explained at https://goo.gl/tpsmEJ#status
If you believe I misattributed a bug to the "security/integrity" subsystem,
please let me know, and if possible forward the report to the correct people or
mailing list.
Here are the bugs:
--------------------------------------------------------------------------------
Title: possible deadlock in process_measurement
Last occurred: 34 days ago
Reported: 267 days ago
Branches: Mainline and others
Dashboard link: https://syzkaller.appspot.com/bug?id=aad04cfa9fddcc5588f8b28ddf739f9a3ebf5874
Original thread: https://lkml.kernel.org/lkml/0000000000003302870578477029@google.com/T/#u
This bug has a C reproducer.
No one replied to the original thread for this bug.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+5ab61747675a87ea359d@syzkaller.appspotmail.com
If you send any email or patch for this bug, please consider replying to the
original thread. For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/0000000000003302870578477029@google.com
--------------------------------------------------------------------------------
Title: INFO: task hung in process_measurement
Last occurred: 118 days ago
Reported: 281 days ago
Branches: Mainline and others
Dashboard link: https://syzkaller.appspot.com/bug?id=623c2e176b9d80b1872e7559e5b823b1ec4911b6
Original thread: https://lkml.kernel.org/lkml/00000000000033ebee0577262a98@google.com/T/#u
This bug has a C reproducer.
syzbot has bisected this bug, but I think the bisection result is incorrect.
The original thread for this bug received 1 reply, 106 days ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+cdc562bc26a2b2b0a94f@syzkaller.appspotmail.com
If you send any email or patch for this bug, please consider replying to the
original thread. For the git send-email command to use, or tips on how to reply
if the thread isn't in your mailbox, see the "Reply instructions" at
https://lkml.kernel.org/r/00000000000033ebee0577262a98@google.com
^ permalink raw reply
* Re: [GIT PULL] LSM: capabilities updates for v5.3
From: pr-tracker-bot @ 2019-07-09 19:55 UTC (permalink / raw)
To: James Morris; +Cc: Linus Torvalds, linux-security-module, linux-kernel
In-Reply-To: <alpine.LRH.2.21.1907090427070.13953@namei.org>
The pull request you sent on Tue, 9 Jul 2019 04:32:39 -0700 (PDT):
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lsm
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/9d22167f34305280c5dd57a74c21651da3c23015
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-09 20:41 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190709170917.GD25369@linux.intel.com>
On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
>> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
>>> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>>>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>>>
>>>> I still don't get why we need this whole mess and do not simply admit
>>>> that there are two distinct roles:
>>>>
>>>> 1. Creator
>>>> 2. User
>>>
>>> Because SELinux has existing concepts of EXECMEM and EXECMOD.
>>
>> What is the official documentation for those? I've only found some
>> explanations from discussions and some RHEL sysadmin guides.
>
> No clue. My knowledge was gleaned from the code and from Stephen's
> feedback.
>
>
> The high level breakdown:
>
> - FILE__EXECUTE: required to gain X on a mapping to a regular file
>
>
> - FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared
> mapping to a regular file
>
> - FILE__EXECMOD: required to gain W->X on a private mapping of a regular
> file
>
> - PROCESS__EXECMEM: required to gain WX on a private mapping to a regular
> file, OR to gain X on an anonymous mapping.
>
>
> Translating those to SGX, with a lot of input from Stephen, I ended up
> with the following:
>
> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> on an enclave page loaded from a regular file
>
> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> required to gain W->X on an enclave page
EXECMOD basically indicates a file containing self-modifying code. Your
ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
> an enclave page that is loaded from an
> anonymous mapping
>
> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
> enclave page
>
>
>
>>> That being said, we can do so without functional changes to the SGX uapi,
>>> e.g. add reserved fields so that the initial uapi can be extended *if* we
>>> decide to go with the "userspace provides maximal protections" path, and
>>> use the EPCM permissions as the maximal protections for the initial
>>> upstreaming.
>>>
>>> That'd give us a minimal implemenation for initial upstreaming and would
>>> eliminate Cedric's blocking complaint. The "whole mess" of whitelisting,
>>> blacklisting and SGX2 support would be deferred until post-upstreaming.
>>
>> I'd like that approach more too.
>>
>> /Jarkko
^ permalink raw reply
* Re: [PATCH 2/2] KEYS: Provide KEYCTL_GRANT_PERMISSION
From: Eric Biggers @ 2019-07-09 20:42 UTC (permalink / raw)
To: David Howells
Cc: keyrings, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <155862712317.24863.13455329541359881229.stgit@warthog.procyon.org.uk>
On Thu, May 23, 2019 at 04:58:43PM +0100, David Howells wrote:
> Provide a keyctl() operation to grant/remove permissions. The grant
> operation, wrapped by libkeyutils, looks like:
>
> int ret = keyctl_grant_permission(key_serial_t key,
> enum key_ace_subject_type type,
> unsigned int subject,
> unsigned int perm);
>
> Where key is the key to be modified, type and subject represent the subject
> to which permission is to be granted (or removed) and perm is the set of
> permissions to be granted. 0 is returned on success. SET_SECURITY
> permission is required for this.
>
> The subject type currently must be KEY_ACE_SUBJ_STANDARD for the moment
> (other subject types will come along later).
>
> For subject type KEY_ACE_SUBJ_STANDARD, the following subject values are
> available:
>
> KEY_ACE_POSSESSOR The possessor of the key
> KEY_ACE_OWNER The owner of the key
> KEY_ACE_GROUP The key's group
> KEY_ACE_EVERYONE Everyone
>
> perm lists the permissions to be granted:
>
> KEY_ACE_VIEW Can view the key metadata
> KEY_ACE_READ Can read the key content
> KEY_ACE_WRITE Can update/modify the key content
> KEY_ACE_SEARCH Can find the key by searching/requesting
> KEY_ACE_LINK Can make a link to the key
> KEY_ACE_SET_SECURITY Can set security
> KEY_ACE_INVAL Can invalidate
> KEY_ACE_REVOKE Can revoke
> KEY_ACE_JOIN Can join this keyring
> KEY_ACE_CLEAR Can clear this keyring
>
> If an ACE already exists for the subject, then the permissions mask will be
> overwritten; if perm is 0, it will be deleted.
>
> Currently, the internal ACL is limited to a maximum of 16 entries.
>
> For example:
>
> int ret = keyctl_grant_permission(key,
> KEY_ACE_SUBJ_STANDARD,
> KEY_ACE_OWNER,
> KEY_ACE_VIEW | KEY_ACE_READ);
>
> Signed-off-by: David Howells <dhowells@redhat.com>
Where is the documentation and tests for this? I want to add syzkaller
definitions for this, but there is no documentation (a commit message doesn't
count). I checked the 'next' branch of keyutils as well.
How is anyone supposed to use this if there is no documentation?
- Eric
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Xing, Cedric @ 2019-07-09 21:16 UTC (permalink / raw)
To: Sean Christopherson, Casey Schaufler
Cc: Dr. Greg, Stephen Smalley, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, sds@tycho.nsa.gov,
jarkko.sakkinen@linux.intel.com
In-Reply-To: <20190709015252.GC24799@linux.intel.com>
On 7/8/2019 6:52 PM, Sean Christopherson wrote:
> On Mon, Jul 08, 2019 at 05:02:00PM -0700, Casey Schaufler wrote:
>> On 7/7/2019 6:30 AM, Dr. Greg wrote:
>>> On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
>>>
>>> Good morning, I hope the weekend has been enjoyable for everyone.
>>>
>>>>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
>>>>>>> ...
>>>>>>> Guess this discussion will never end if we don't get into
>>>>>>> code. Guess it'd be more productive to talk over phone then come back
>>>>>>> to this thread with a conclusion. Will that be ok with you?
>>>>>> I don't think that a phone call is going to help. Talking code
>>>>>> issues tends to muddle them in my brain. If you can give me a few
>>>>>> days I will propose a rough version of how I think your code should
>>>>>> be integrated into the LSM environment. I'm spending more time
>>>>>> trying (unsuccessfully :( ) to discribe the issues in English than
>>>>>> it will probably take in C.
>>>>> While Casey is off writing his rosetta stone,
>>>> I'd hardly call it that. More of an effort to round the corners on
>>>> the square peg. And Cedric has some ideas on how to approach that.
>>> Should we infer from this comment that, of the two competing
>>> strategies, Cedric's is the favored architecture?
>>
>> With Cedric's latest patches I'd say there's only one
>> strategy. There's still some refinement to do, but we're
>> getting there.
>
> Dynamic tracking has an unsolvable race condition. If process A maps a
> page W and process B maps the same page X, then the result of W^X checks
> depends on the order of mprotect() calls between A and B.
I don't quite understand where the term "dynamic tracking" came from.
What's done in the patch is just to track which page was contributed by
which file. It's been done for all file mappings in Linux.
Back to the "race condition". A similar situation already exists in
SELinux, between EXECMOD and EXECMEM. Say a file does *not* have EXECMOD
but the calling process has EXECMEM. Then WX could be granted to a fresh
private mapping (because of EXECMEM). However, once the mapping has been
written to, X should have been revoked (because of lack of EXECMOD) but
will still be retained until dropped by an explicit mprotect().
Afterwards, mprotect(X) will be denied. That's not the same situation as
in this enclave case but they do share one thing in common - X should
have been revoked from an existing mapping but it isn't, which is just a
policy choice.
Nothing is unsolvable. Here are 2 options.
(1) We argue that it doesn't matter, similar to the EXECMOD/EXECMEM case
on regular file mappings described above; or
(2) EXECMOD is required for both W->X and X->W transitions, hence W
requested by the 2nd process will be denied because X has been granted
to the 1st process while EXECMOD is absent.
Please note that #2 is effectively the same concept as
PROCESS2__ENCLAVE_EXECDIRTY in Sean's patch, except that EXECMOD is per
file while ENCLAVE_EXECDIRTY is per process.
^ permalink raw reply
* Re: [PATCH v5 15/23] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-07-09 21:18 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul,
linux-audit@redhat.com, casey
In-Reply-To: <dbdcfb3d-a88a-67eb-a100-848f3335e9a3@tycho.nsa.gov>
On 7/9/2019 11:12 AM, Stephen Smalley wrote:
> On 7/9/19 1:51 PM, Casey Schaufler wrote:
>> On 7/9/2019 10:13 AM, Stephen Smalley wrote:
>>> On 7/3/19 5:25 PM, Casey Schaufler wrote:
>>>> Create a new entry "display" in /proc/.../attr for controlling
>>>> which LSM security information is displayed for a process.
>>>> The name of an active LSM that supplies hooks for human readable
>>>> data may be written to "display" to set the value. The name of
>>>> the LSM currently in use can be read from "display".
>>>> At this point there can only be one LSM capable of display
>>>> active. A helper function lsm_task_display() to get the display
>>>> slot for a task_struct.
>>>
>>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things.
>>
>> I would very much like some feedback regarding which of the
>> possible formats for putting multiple subject contexts in
>> audit records would be preferred:
>>
>> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy
>> lsm=selinux,smack subj=xyzzy_t,Xyzzy
>> subj="selinux='xyzzy_t',smack='Xyzzy'"
>
> (cc'd linux-audit mailing list)
>
>>
>> Or something else. Free bikeshedding!
>>
>> I don't see how you have a problem with netlink. My look
>> at what's in the kernel didn't expose anything, but I am
>> willing to be educated.
>
> I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect.
If the context is generated by security_secid_to_secctx() we
retain the slot number of the module that created it in lsmcontext.
We have to to ensure it is released correctly. If the potential
issue you're describing for netlink does in fact occur, we can check
the slot in lsmcontext to verify that it is the same.
security_secid_to_secctx() is called nowhere in net/netlink,
at least not that grep finds. Where are you seeing this potential
problem?
>
>>
>>> Before:
>>> $ id
>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>> $ su
>>> Password:
>>> su: Authentication failure
>>>
>>> syscall audit record:
>>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat
>>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_
>>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s
>>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null)
>>>
>>> After:
>>> $ id
>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>> $ echo apparmor > /proc/self/attr/display
>>> $ su
>>> Password:
>>> su: Authentication failure
>>>
>>> audit record:
>>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null)
>>>
>>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux.
>>
>> It's still accurate, it's just not complete. It's a matter
>> of how best to complete it.
>>
>>>
>>> Now, let's play with userspace.
>>>
>>> Before:
>>> # id
>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>> # passwd root
>>> passwd: SELinux deny access due to security policy.
>>>
>>> audit record:
>>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2'
>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>>
>>> After:
>>> # id
>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>> # echo apparmor > /proc/self/attr/display
>>> # passwd root
>>> passwd: SELinux deny access due to security policy.
>>>
>>> audit record:
>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>>
>>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context.
>>>
>>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching
>>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates!
>>>
>>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module.
>>
>> The issues you've raised around audit are meritorious.
>> Any suggestions regarding how to address them would be
>> quite welcome.
>>
>> As far as the general objection to the display mechanism,
>> I am eager to understand what you might propose as an
>> alternative. We can't dismiss backward compatibility for
>> any of the modules. We can't preclude any module combination.
>>
>> We can require user space changes for configurations that
>> were impossible before, just as the addition of SELinux to
>> a system required user space changes. Update libselinux
>> to check the display before using the attr interfaces and
>> you've addressed most of the issues.
>
> Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials)
In the process of trying to argue against your point I
may have come around to your thinking. There would still
be the case where a privileged program sets the display
and invokes an equally privileged program which is "tricked"
into setting the wrong attribute, but you have to put the
responsibility for use of privilege on someone, somewhere.
I will propose a solution in the next round.
> or the security modules need to be able to control who can set the display.
That's a mechanism for a module to opt-out of stacking,
and Paul has been pretty clear that he won't go for that.
> Or both.
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Xing, Cedric @ 2019-07-09 21:26 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-sgx, linux-security-module, selinux
In-Reply-To: <20190709013335.GB24799@linux.intel.com>
On 7/8/2019 6:33 PM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>> +static int enclave_mprotect(struct vm_area_struct *vma, size_t prot)
>> +{
>> + struct ema_map *m;
>> + int rc;
>> +
>> + /* is vma an enclave vma ? */
>> + if (!vma->vm_file)
>> + return 0;
>> + m = ema_get_map(vma->vm_file);
>> + if (!m)
>> + return 0;
>> +
>> + /* WX requires EXECMEM */
>> + if ((prot && PROT_WRITE) && (prot & PROT_EXEC)) {
>> + rc = avc_has_perm(&selinux_state, current_sid(), current_sid(),
>> + SECCLASS_PROCESS, PROCESS__EXECMEM, NULL);
>> + if (rc)
>> + return rc;
>> + }
>> +
>> + rc = ema_lock_map(m);
>> + if (rc)
>> + return rc;
>> +
>> + if ((prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC))
>> + rc = ema_apply_to_range(m, vma->vm_start, vma->vm_end,
>> + ema__chk_X_cb, vma->vm_file);
>> + if (!rc && (prot & PROT_WRITE) && !(vma->vm_flags & VM_WRITE))
>> + rc = ema_apply_to_range(m, vma->vm_start, vma->vm_end,
>> + ema__set_M_cb, NULL);
>
> Not tracking whether a page has been mapped X and having ema__chk_W_cb()
> allows an application to circumvent W^X policies by spinning up a helper
> process.
See my response in another email.
This problem has nothing to do with the architecture, but is just a
policy choice. Your patch of EXECDIRTY is another possible policy, by
combining (or *not* distinguishing) W->X and X->W into a single WX
"maximal protection".
> Ignoring that issue, this approach suffers from the same race condition I
> pointed out a while back[1]. If process A maps a page W and process B
> maps the same page X, then the result of ema__chk_X_cb() depends on the
> order of mprotect() calls between A and B.
>
> [1] https://lore.kernel.org/linux-security-module/20190614200123.GA32570@linux.intel.com/
You seem to be talking about the same problem in both places.
>> + ema_unlock_map(m);
>> +
>> + return rc;
>> +}
^ permalink raw reply
* Re: [PATCH v5 15/23] LSM: Specify which LSM to display
From: Stephen Smalley @ 2019-07-09 21:34 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul,
linux-audit@redhat.com
In-Reply-To: <1d62a67c-2096-5d8b-dad4-2e1c1c0ebc06@schaufler-ca.com>
On 7/9/19 5:18 PM, Casey Schaufler wrote:
> On 7/9/2019 11:12 AM, Stephen Smalley wrote:
>> On 7/9/19 1:51 PM, Casey Schaufler wrote:
>>> On 7/9/2019 10:13 AM, Stephen Smalley wrote:
>>>> On 7/3/19 5:25 PM, Casey Schaufler wrote:
>>>>> Create a new entry "display" in /proc/.../attr for controlling
>>>>> which LSM security information is displayed for a process.
>>>>> The name of an active LSM that supplies hooks for human readable
>>>>> data may be written to "display" to set the value. The name of
>>>>> the LSM currently in use can be read from "display".
>>>>> At this point there can only be one LSM capable of display
>>>>> active. A helper function lsm_task_display() to get the display
>>>>> slot for a task_struct.
>>>>
>>>> As I explained previously, this is a security hole waiting to happen. It still permits a process to affect the output of audit, alter the result of reading or writing /proc/self/attr nodes even by setuid/setgid/file-caps/context-changing programs, alter the contexts generated in netlink messages delivered to other processes (I think?), and possibly other effects beyond affecting the process' own view of things.
>>>
>>> I would very much like some feedback regarding which of the
>>> possible formats for putting multiple subject contexts in
>>> audit records would be preferred:
>>>
>>> lsm=selinux,subj=xyzzy_t lsm=smack,subj=Xyzzy
>>> lsm=selinux,smack subj=xyzzy_t,Xyzzy
>>> subj="selinux='xyzzy_t',smack='Xyzzy'"
>>
>> (cc'd linux-audit mailing list)
>>
>>>
>>> Or something else. Free bikeshedding!
>>>
>>> I don't see how you have a problem with netlink. My look
>>> at what's in the kernel didn't expose anything, but I am
>>> willing to be educated.
>>
>> I haven't traced through it in detail, but it wasn't clear to me that the security_secid_to_secctx() call always occurs in the context of the receiving process (and hence use its display value). If not, then the display of the sender can affect what is reported to the receiver; hence, there is a forgery concern similar to the binder issue. It would be cleaner if we didn't alter the default behavior of security_secid_to_secctx() and security_secctx_to_secid() and instead introduced new hooks for any case where we truly want the display to take effect.
>
> If the context is generated by security_secid_to_secctx() we
> retain the slot number of the module that created it in lsmcontext.
> We have to to ensure it is released correctly. If the potential
> issue you're describing for netlink does in fact occur, we can check
> the slot in lsmcontext to verify that it is the same.
>
> security_secid_to_secctx() is called nowhere in net/netlink,
> at least not that grep finds. Where are you seeing this potential
> problem?
Look under net/netfilter.
>
>>
>>>
>>>> Before:
>>>> $ id
>>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>>> $ su
>>>> Password:
>>>> su: Authentication failure
>>>>
>>>> syscall audit record:
>>>> type=SYSCALL msg=audit(07/09/2019 11:52:49.784:365) : arch=x86_64 syscall=openat
>>>> success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x560897e58e00 a2=O_
>>>> WRONLY a3=0x0 items=1 ppid=3258 pid=3781 auid=sds2 uid=sds2 gid=sds2 euid=root s
>>>> uid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null)
>>>>
>>>> After:
>>>> $ id
>>>> uid=1002(sds2) gid=1002(sds2) groups=1002(sds2) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>>> $ echo apparmor > /proc/self/attr/display
>>>> $ su
>>>> Password:
>>>> su: Authentication failure
>>>>
>>>> audit record:
>>>> type=SYSCALL msg=audit(07/09/2019 12:05:32.402:406) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x556b41e1ae00 a2=O_WRONLY a3=0x0 items=1 ppid=3258 pid=9426 auid=sds2 uid=sds2 gid=sds2 euid=root suid=root fsuid=root egid=sds2 sgid=sds2 fsgid=sds2 tty=pts2 ses=6 comm=su exe=/usr/bin/su subj==unconfined key=(null)
>>>>
>>>> NB The subj= field of the SYSCALL audit record is longer accurate and is potentially under the control of a process that would not be authorized to set its subject label to that value by SELinux.
>>>
>>> It's still accurate, it's just not complete. It's a matter
>>> of how best to complete it.
>>>
>>>>
>>>> Now, let's play with userspace.
>>>>
>>>> Before:
>>>> # id
>>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>>> # passwd root
>>>> passwd: SELinux deny access due to security policy.
>>>>
>>>> audit record:
>>>> type=USER_AVC msg=audit(07/09/2019 12:24:35.135:812) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='avc: denied { passwd } for scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tcontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023 tclass=passwd permissive=0 exe=/usr/bin/passwd sauid=root hostname=? addr=? terminal=pts/2'
>>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:24:35.135:813) : pid=12693 uid=root auid=sds2 ses=7 subj=staff_u:staff_r:passwd_t:s0-s0:c0.c1023 msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>>>
>>>> After:
>>>> # id
>>>> uid=0(root) gid=0(root) groups=0(root) context=staff_u:staff_r:staff_t:s0-s0:c0.c1023
>>>> # echo apparmor > /proc/self/attr/display
>>>> # passwd root
>>>> passwd: SELinux deny access due to security policy.
>>>>
>>>> audit record:
>>>> type=USER_CHAUTHTOK msg=audit(07/09/2019 12:28:41.349:832) : pid=13083 uid=root auid=sds2 ses=7 subj==unconfined msg='op=attempted-to-change-password id=root exe=/usr/bin/passwd hostname=moss-pluto.infosec.tycho.ncsc.mil addr=? terminal=pts/2 res=failed'
>>>>
>>>> Here we again get the wrong value for subj= in the USER_CHAUTHTOK audit record, and we further lose the USER_AVC record entirely because it didn't even reach the point of the permission check due to not being able to get the caller context.
>>>>
>>>> The situation gets worse if the caller can set things up such that it can set an attribute value for one security module that is valid and privileged with respect to another security module. This isn't a far-fetched scenario; AppArmor will default to running everything unconfined, so as soon as you enable it, any root process can potentially load a policy that defines contexts that look exactly like SELinux contexts. Smack is even simpler; you can set any arbitrary string you want as long as you are root (by default); no policy required. So a root process that is confined by SELinux (or by AppAmor) can suddenly forge arbitrary contexts in audit records or reads of /proc/self/attr nodes or netlink messages or ..., just by virtue of applying these patches and enabling another security module like Smack. Or consider if ptags were ever made real and merged - by design, that's all about setting arbitrary tags from userspace. Then there is the separate issue of switching
>>>> display to prevent attempts by a more privileged program to set one of its attributes from taking effect. Where have we seen that before - sendmail capabilities bug anyone? And it is actually worse than that bug, because with the assistance of a friendly security module, the write may actually succeed; it just won't alter the SELinux context of the program or anything it creates!
>>>>
>>>> This gets a NAK from me so long as it has these issues and setting the display remains outside the control of any security module.
>>>
>>> The issues you've raised around audit are meritorious.
>>> Any suggestions regarding how to address them would be
>>> quite welcome.
>>>
>>> As far as the general objection to the display mechanism,
>>> I am eager to understand what you might propose as an
>>> alternative. We can't dismiss backward compatibility for
>>> any of the modules. We can't preclude any module combination.
>>>
>>> We can require user space changes for configurations that
>>> were impossible before, just as the addition of SELinux to
>>> a system required user space changes. Update libselinux
>>> to check the display before using the attr interfaces and
>>> you've addressed most of the issues.
>>
>> Either we ensure that setting of the display can only affect processes in the same security equivalence class (same credentials)
>
> In the process of trying to argue against your point I
> may have come around to your thinking. There would still
> be the case where a privileged program sets the display
> and invokes an equally privileged program which is "tricked"
> into setting the wrong attribute, but you have to put the
> responsibility for use of privilege on someone, somewhere.
>
> I will propose a solution in the next round.
>
>> or the security modules need to be able to control who can set the display.
>
> That's a mechanism for a module to opt-out of stacking,
> and Paul has been pretty clear that he won't go for that.
It doesn't have to be used that way; it can just be used to limit the
set of authorized processes that can set the display to e.g. trusted
container runtimes.
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Xing, Cedric @ 2019-07-09 22:13 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
In-Reply-To: <decc7ae6-a89f-1ae5-6289-f3dcaa6390b0@schaufler-ca.com>
On 7/8/2019 4:53 PM, Casey Schaufler wrote:
> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>> In this scheme you use an ema LSM to manage your ema data.
>>> A quick sketch looks like:
>>>
>>> sgx_something_in() calls
>>> security_enclave_load() calls
>>> ema_enclave_load()
>>> selinux_enclave_load()
>>> otherlsm_enclave_load()
>>>
>>> Why is this better than:
>>>
>>> sgx_something_in() calls
>>> ema_enclave_load()
>>> security_enclave_load() calls
>>> selinux_enclave_load()
>>> otherlsm_enclave_load()
>>
>> Are you talking about moving EMA somewhere outside LSM?
>
> Yes. That's what I've been saying all along.
>
>> If so, where?
>
> I tried to make it obvious. Put the call to your EMA code
> on the line before you call security_enclave_load().
Sorry but I'm still confused.
EMA code is used by LSMs only. Making it callable from other parts of
the kernel IMHO is probably not a good idea. And more importantly I
don't understand the motivation behind it. Would you please elaborate?
>>>> +/**
>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>
>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>
>> Noted
>>
>>>> diff --git a/security/Makefile b/security/Makefile
>>>> index c598b904938f..b66d03a94853 100644
>>>> --- a/security/Makefile
>>>> +++ b/security/Makefile
>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>
>>> The config option and the file name ought to match,
>>> or at least be closer.
>>
>> Just trying to match file names as "capability" uses commoncap.c.
>
> Fine, then you should be using CONFIG_SECURITY_EMA.
>
>>
>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>
> Make
> CONFIG_SECURITY_EMA
> depends on CONFIG_INTEL_SGX
>
> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>
> CONFIG_SECURITY_EMA
> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
Your suggestions are reasonable. Given such config change wouldn't
affect any code, can we do it later, e.g., when additional TEEs come
online and make use of these new hooks? After all,
security_enclave_init() will need amendment anyway as one of its current
parameters is of type 'struct sgx_sigstruct', which will need to be
replaced with something more generic. At the time being, I'd like to
keep things intuitive so as not to confuse reviewers.
>>
>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>
>>> Put this in a subdirectory. Please.
>>
>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>
> commoncap is not optional. It is a base part of the
> security subsystem. ema is optional.
Alright. I'd move it into a sub-folder and rename it to ema.c. Would you
be ok with that?
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-07-09 22:25 UTC (permalink / raw)
To: Xing, Cedric
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <512391ba-fe0d-e758-ae32-09660c1264f7@intel.com>
On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
> On 7/9/2019 10:09 AM, Sean Christopherson wrote:
> >Translating those to SGX, with a lot of input from Stephen, I ended up
> >with the following:
> >
> > - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> > on an enclave page loaded from a regular file
> >
> > - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> > required to gain W->X on an enclave page
>
> EXECMOD basically indicates a file containing self-modifying code. Your
> ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
then it needs PROCESS2__ENCLAVE_EXECDIRTY.
FILE__EXECMOD on /dev/sgx/enclave is a process permission masquerading as
a file permission, let's call it what it is.
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-09 23:11 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190709222534.GG25369@linux.intel.com>
On 7/9/2019 3:25 PM, Sean Christopherson wrote:
> On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote:
>> On 7/9/2019 10:09 AM, Sean Christopherson wrote:
>>> Translating those to SGX, with a lot of input from Stephen, I ended up
>>> with the following:
>>>
>>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>>> on an enclave page loaded from a regular file
>>>
>>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>>> required to gain W->X on an enclave page
>>
>> EXECMOD basically indicates a file containing self-modifying code. Your
>> ENCLAVE_EXECDIRTY is however a process permission, which is illogical.
>
> How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page,
> then it needs PROCESS2__ENCLAVE_EXECDIRTY
Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file
has self-modifying code, hence W->X transition should be considered
"normal" and allowed, regardless which process that file is loaded into.
The same thing for enclaves here. Whether an enclave contains
self-modifying code is specific to that enclave, regardless which
process it is loaded into.
But what are you doing is quite the opposite, and that's I mean by
"illogical".
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Casey Schaufler @ 2019-07-10 0:10 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
In-Reply-To: <1f5b5fc1-9a95-9748-f9dc-0486c6ae30d8@intel.com>
On 7/9/2019 3:13 PM, Xing, Cedric wrote:
> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>> In this scheme you use an ema LSM to manage your ema data.
>>>> A quick sketch looks like:
>>>>
>>>> sgx_something_in() calls
>>>> security_enclave_load() calls
>>>> ema_enclave_load()
>>>> selinux_enclave_load()
>>>> otherlsm_enclave_load()
>>>>
>>>> Why is this better than:
>>>>
>>>> sgx_something_in() calls
>>>> ema_enclave_load()
>>>> security_enclave_load() calls
>>>> selinux_enclave_load()
>>>> otherlsm_enclave_load()
>>>
>>> Are you talking about moving EMA somewhere outside LSM?
>>
>> Yes. That's what I've been saying all along.
>>
>>> If so, where?
>>
>> I tried to make it obvious. Put the call to your EMA code
>> on the line before you call security_enclave_load().
>
> Sorry but I'm still confused.
>
> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
LSM modules implement additional access control restrictions.
The EMA code does not do that, it provides management of data
that is used by security modules. It is not one itself. VFS
also performs this role, but no one would consider making VFS
a security module.
>>>>> +/**
>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>
>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>
>>> Noted
>>>
>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>> index c598b904938f..b66d03a94853 100644
>>>>> --- a/security/Makefile
>>>>> +++ b/security/Makefile
>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>
>>>> The config option and the file name ought to match,
>>>> or at least be closer.
>>>
>>> Just trying to match file names as "capability" uses commoncap.c.
>>
>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>
>>>
>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>
>> Make
>> CONFIG_SECURITY_EMA
>> depends on CONFIG_INTEL_SGX
>>
>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>
>> CONFIG_SECURITY_EMA
>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>
> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
That doesn't make the current options any less confusing,
and it will be easier to make the change now than at some
point in the future.
> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
Reviewers (including me) are already confused by the inconsistency.
>
>>>
>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>
>>>> Put this in a subdirectory. Please.
>>>
>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>
>> commoncap is not optional. It is a base part of the
>> security subsystem. ema is optional.
>
> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
Sounds fine.
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Xing, Cedric @ 2019-07-10 0:55 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
In-Reply-To: <34552999-160e-4f60-8d7e-37f51c895ef4@schaufler-ca.com>
On 7/9/2019 5:10 PM, Casey Schaufler wrote:
> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>> A quick sketch looks like:
>>>>>
>>>>> sgx_something_in() calls
>>>>> security_enclave_load() calls
>>>>> ema_enclave_load()
>>>>> selinux_enclave_load()
>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Why is this better than:
>>>>>
>>>>> sgx_something_in() calls
>>>>> ema_enclave_load()
>>>>> security_enclave_load() calls
>>>>> selinux_enclave_load()
>>>>> otherlsm_enclave_load()
>>>>
>>>> Are you talking about moving EMA somewhere outside LSM?
>>>
>>> Yes. That's what I've been saying all along.
>>>
>>>> If so, where?
>>>
>>> I tried to make it obvious. Put the call to your EMA code
>>> on the line before you call security_enclave_load().
>>
>> Sorry but I'm still confused.
>>
>> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
>
> LSM modules implement additional access control restrictions.
> The EMA code does not do that, it provides management of data
> that is used by security modules. It is not one itself. VFS
> also performs this role, but no one would consider making VFS
> a security module.
You are right. EMA is more like a helper library than a real LSM. But
the practical problem is, it has a piece of initialization code, to
basically request some space in the file blob from the LSM
infrastructure. That cannot be done by any LSMs at runtime. So it has to
either be done in LSM infrastructure directly, or make itself an LSM to
make its initialization function invoked by LSM infrastructure
automatically. You have objected to the former, so I switched to the
latter. Are you now objecting to the latter as well? Then what are you
suggesting, really?
VFS is a completely different story. It's the file system abstraction so
it has a natural place to live in the kernel, and its initialization
doesn't depend on the LSM infrastructure. EMA on the other hand, shall
belong to LSM because it is both produced and consumed within LSM.
And, Stephen, do you have an opinion on this?
>>>>>> +/**
>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>
>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>
>>>> Noted
>>>>
>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>> --- a/security/Makefile
>>>>>> +++ b/security/Makefile
>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>
>>>>> The config option and the file name ought to match,
>>>>> or at least be closer.
>>>>
>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>
>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>
>>>>
>>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>>
>>> Make
>>> CONFIG_SECURITY_EMA
>>> depends on CONFIG_INTEL_SGX
>>>
>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>
>>> CONFIG_SECURITY_EMA
>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>
>> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
>
> That doesn't make the current options any less confusing,
> and it will be easier to make the change now than at some
> point in the future.
>
>> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
>
> Reviewers (including me) are already confused by the inconsistency.
OK. Let me make this change.
>>>>
>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>
>>>>> Put this in a subdirectory. Please.
>>>>
>>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>>
>>> commoncap is not optional. It is a base part of the
>>> security subsystem. ema is optional.
>>
>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
>
> Sounds fine.
This is another part that confuses me. Per you comment here, I think you
are OK with EMA being part of LSM (I mean, living somewhere under
security/). But your other comment of calling ema_enclave_load()
alongside security_enclave_load() made me think EMA and LSM were
separate. What do you want really?
^ permalink raw reply
* Re: [PATCH 1/2] KEYS: Replace uid/gid/perm permissions checking with an ACL
From: Eric Biggers @ 2019-07-10 1:15 UTC (permalink / raw)
To: David Howells
Cc: keyrings, linux-security-module, linux-fsdevel, linux-kernel
In-Reply-To: <155862710731.24863.14013725058582750710.stgit@warthog.procyon.org.uk>
On Thu, May 23, 2019 at 04:58:27PM +0100, David Howells wrote:
> Replace the uid/gid/perm permissions checking on a key with an ACL to allow
> the SETATTR and SEARCH permissions to be split. This will also allow a
> greater range of subjects to represented.
>
This patch broke 'keyctl new_session', and hence broke all the fscrypt tests:
$ keyctl new_session
keyctl_session_to_parent: Permission denied
Output of 'keyctl show' is
$ keyctl show
Session Keyring
605894913 --alswrv 0 0 keyring: _ses
189223103 ----s-rv 0 0 \_ user: invocation_id
- Eric
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Dr. Greg @ 2019-07-10 1:28 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Cedric Xing, Andy Lutomirski, Jethro Beekman, Stephen Smalley
In-Reply-To: <20190708172930.GA20791@linux.intel.com>
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
Good evening to everyone.
> That being said, we can do so without functional changes to the SGX
> uapi, e.g. add reserved fields so that the initial uapi can be
> extended *if* we decide to go with the "userspace provides maximal
> protections" path, and use the EPCM permissions as the maximal
> protections for the initial upstreaming.
>
> That'd give us a minimal implemenation for initial upstreaming and
> would eliminate Cedric's blocking complaint. The "whole mess" of
> whitelisting, blacklisting and SGX2 support would be deferred until
> post-upstreaming.
Are we convinced the 'mess' will be any easier to clean up after the
driver is upstreamed?
The primary problem is that we haven't addressed the issue of what
this technology is designed to do and its implications with respect to
the kernel. As a result we are attempting to implement controls which
we are comfortable with and understand rather then those that are
relevant.
Have a good evening.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC Implementing SGX secured and modeled
4206 N. 19th Ave. intelligent network endpoints.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Courage is not the absence of fear, but rather the judgement that
something else is more important than fear."
-- Ambrose Redmoon
^ permalink raw reply
* Re: [PATCH 1/2] KEYS: Replace uid/gid/perm permissions checking with an ACL
From: Eric Biggers @ 2019-07-10 1:35 UTC (permalink / raw)
To: David Howells, keyrings, linux-security-module, linux-fsdevel,
linux-kernel
In-Reply-To: <20190710011559.GA7973@sol.localdomain>
On Tue, Jul 09, 2019 at 06:16:01PM -0700, Eric Biggers wrote:
> On Thu, May 23, 2019 at 04:58:27PM +0100, David Howells wrote:
> > Replace the uid/gid/perm permissions checking on a key with an ACL to allow
> > the SETATTR and SEARCH permissions to be split. This will also allow a
> > greater range of subjects to represented.
> >
>
> This patch broke 'keyctl new_session', and hence broke all the fscrypt tests:
>
> $ keyctl new_session
> keyctl_session_to_parent: Permission denied
>
> Output of 'keyctl show' is
>
> $ keyctl show
> Session Keyring
> 605894913 --alswrv 0 0 keyring: _ses
> 189223103 ----s-rv 0 0 \_ user: invocation_id
>
> - Eric
... and this also broke loading in-kernel X.509 certificates. See the other
thread: https://lore.kernel.org/lkml/27671.1562384658@turing-police/T/#u
- Eric
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-10 2:04 UTC (permalink / raw)
To: Dr. Greg, Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Stephen Smalley
In-Reply-To: <20190710012811.GA18755@wind.enjellic.com>
On 7/9/2019 6:28 PM, Dr. Greg wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
>
> Good evening to everyone.
>
>> That being said, we can do so without functional changes to the SGX
>> uapi, e.g. add reserved fields so that the initial uapi can be
>> extended *if* we decide to go with the "userspace provides maximal
>> protections" path, and use the EPCM permissions as the maximal
>> protections for the initial upstreaming.
>>
>> That'd give us a minimal implemenation for initial upstreaming and
>> would eliminate Cedric's blocking complaint. The "whole mess" of
>> whitelisting, blacklisting and SGX2 support would be deferred until
>> post-upstreaming.
>
> Are we convinced the 'mess' will be any easier to clean up after the
> driver is upstreamed?
>
> The primary problem is that we haven't addressed the issue of what
> this technology is designed to do and its implications with respect to
> the kernel. As a result we are attempting to implement controls which
> we are comfortable with and understand rather then those that are
> relevant.
I don't think it's about easier or harder to clean up the mess, but a
divide-and-conquer strategy. After all, SGX and LSM are kind of
orthogonal as long as SGX doesn't compromise the protection provided by LSM.
Let's step back and look at what started this lengthy discussion. The
primary problem of v20 was that the SGX module allows executable enclave
pages to be created from non-executable regular pages, which could be
exploited by adversaries to grant executable permissions to pages that
would otherwise be denied without SGX. And that could be fixed simply by
capping EPCM permissions to whatever allowed on the source page, without
touching LSM. Of course the drawback is loss of functionality - i.e.
self-modifying enclaves cannot be loaded unless the calling process has
EXECMEM. But that should suffice, as most SGX1 applications don't
contain self-modifying code anyway.
Then we could switch our focus back to LSM and work out what's relevant,
especially for SGX2 and beyond.
> Have a good evening.
>
> Dr. Greg
>
> As always,
> Dr. Greg Wettstein, Ph.D, Worker
> IDfusion, LLC Implementing SGX secured and modeled
> 4206 N. 19th Ave. intelligent network endpoints.
> Fargo, ND 58102
> PH: 701-281-1686 EMAIL: greg@idfusion.net
> ------------------------------------------------------------------------------
> "Courage is not the absence of fear, but rather the judgement that
> something else is more important than fear."
> -- Ambrose Redmoon
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox