* [PATCH 5/5] EVM: use obj-y for non-modular objects
From: Masahiro Yamada @ 2019-07-26 2:10 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn, linux-kernel,
linux-security-module
In-Reply-To: <20190726021058.4212-1-yamada.masahiro@socionext.com>
CONFIG_EVM is a boolean option, so none of these objects is linked
into a module.
All the objects in this directory are compiled only when CONFIG_EVM=y
since this directory is guarded by the parent Makefile:
obj-$(CONFIG_EVM) += evm/
So, there is no point in creating the composite object, evm.o
Flatten the code into the obj-$(CONFIG_...) form.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
security/integrity/evm/Makefile | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/security/integrity/evm/Makefile b/security/integrity/evm/Makefile
index a56f5613be79..ace8e4ef5a96 100644
--- a/security/integrity/evm/Makefile
+++ b/security/integrity/evm/Makefile
@@ -2,7 +2,5 @@
#
# Makefile for building the Extended Verification Module(EVM)
#
-obj-$(CONFIG_EVM) += evm.o
-
-evm-y := evm_main.o evm_crypto.o evm_secfs.o
-evm-$(CONFIG_FS_POSIX_ACL) += evm_posix_acl.o
+obj-y := evm_main.o evm_crypto.o evm_secfs.o
+obj-$(CONFIG_FS_POSIX_ACL) += evm_posix_acl.o
--
2.17.1
^ permalink raw reply related
* [PATCH 2/5] integrity: remove pointless subdir-$(CONFIG_...)
From: Masahiro Yamada @ 2019-07-26 2:10 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Masahiro Yamada, Dave Howells, James Morris, Josh Boyer,
Martin Schwidefsky, Nayna Jain, Serge E. Hallyn, linux-kernel,
linux-security-module
In-Reply-To: <20190726021058.4212-1-yamada.masahiro@socionext.com>
The ima/ and evm/ sub-directories contain built-in objects, so
obj-$(CONFIG_...) is the correct way to descend into them.
subdir-$(CONFIG_...) is redundant.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
security/integrity/Makefile | 2 --
1 file changed, 2 deletions(-)
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index b6d6273a4176..35e6ca773734 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -14,7 +14,5 @@ integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
platform_certs/load_uefi.o
integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
-subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
-subdir-$(CONFIG_EVM) += evm
obj-$(CONFIG_EVM) += evm/
--
2.17.1
^ permalink raw reply related
* [PATCH 1/5] integrity: remove unneeded, broken attempt to add -fshort-wchar
From: Masahiro Yamada @ 2019-07-26 2:10 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Masahiro Yamada, Dave Howells, James Morris, Josh Boyer,
Martin Schwidefsky, Nayna Jain, Serge E. Hallyn, linux-kernel,
linux-security-module
In-Reply-To: <20190726021058.4212-1-yamada.masahiro@socionext.com>
I guess commit 15ea0e1e3e18 ("efi: Import certificates from UEFI Secure
Boot") attempted to add -fshort-wchar for building load_uefi.o, but it
has never worked as intended.
load_uefi.o is created in the platform_certs/ sub-directory. If you
really want to add -fshort-wchar, the correct code is:
$(obj)/platform_certs/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
or, in a more Kbuild-ish way:
CFLAGS_load_uefi.o := -fshort-wchar
But, you do not need to fix it.
Commit 8c97023cf051 ("Kbuild: use -fshort-wchar globally") had already
added -fshort-wchar globally. This code was unneeded in the first place.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
security/integrity/Makefile | 1 -
1 file changed, 1 deletion(-)
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 19faace69644..b6d6273a4176 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -13,7 +13,6 @@ integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyrin
integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
platform_certs/load_uefi.o
integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
-$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
subdir-$(CONFIG_IMA) += ima
obj-$(CONFIG_IMA) += ima/
--
2.17.1
^ permalink raw reply related
* [PATCH 4/5] IMA: use obj-y for non-modular objects
From: Masahiro Yamada @ 2019-07-26 2:10 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Masahiro Yamada, Dmitry Kasatkin, James Morris, Serge E. Hallyn,
linux-kernel, linux-security-module
In-Reply-To: <20190726021058.4212-1-yamada.masahiro@socionext.com>
CONFIG_IMA is a boolean option, so none of these objects is linked
into a module.
All the objects in this directory are compiled only when CONFIG_IMA=y
since this directory is guarded by the parent Makefile:
obj-$(CONFIG_IMA) += ima/
So, there is no point in creating the composite object, ima.o
Flatten the code into the obj-$(CONFIG_...) form.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
security/integrity/ima/Makefile | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..5517486c9154 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -4,10 +4,8 @@
# Measurement Architecture(IMA).
#
-obj-$(CONFIG_IMA) += ima.o
-
-ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
+obj-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_template.o ima_template_lib.o
-ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
-ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
+obj-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+obj-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
--
2.17.1
^ permalink raw reply related
* [PATCH] test_meminit: use GFP_ATOMIC in RCU critical section
From: Alexander Potapenko @ 2019-07-25 12:17 UTC (permalink / raw)
Cc: Alexander Potapenko, Kees Cook, Andrew Morton, linux-kernel,
linux-mm, linux-security-module
kmalloc() shouldn't sleep while in RCU critical section, therefore
use GFP_ATOMIC instead of GFP_KERNEL.
The bug has been spotted by the 0day kernel testing robot.
Fixes: 7e659650cbda ("lib: introduce test_meminit module")
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Cc: linux-security-module@vger.kernel.org
---
lib/test_meminit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/test_meminit.c b/lib/test_meminit.c
index 62d19f270cad..9729f271d150 100644
--- a/lib/test_meminit.c
+++ b/lib/test_meminit.c
@@ -222,7 +222,7 @@ static int __init do_kmem_cache_size(size_t size, bool want_ctor,
* Copy the buffer to check that it's not wiped on
* free().
*/
- buf_copy = kmalloc(size, GFP_KERNEL);
+ buf_copy = kmalloc(size, GFP_ATOMIC);
if (buf_copy)
memcpy(buf_copy, buf, size);
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* Re: [PATCH V36 27/29] tracefs: Restrict tracefs when the kernel is locked down
From: Steven Rostedt @ 2019-07-25 2:23 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Matthew Garrett
In-Reply-To: <20190718194415.108476-28-matthewgarrett@google.com>
On Thu, 18 Jul 2019 12:44:13 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> @@ -387,6 +412,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> struct dentry *parent, void *data,
> const struct file_operations *fops)
> {
> + struct file_operations *proxy_fops;
> struct dentry *dentry;
> struct inode *inode;
>
> @@ -402,8 +428,18 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> if (unlikely(!inode))
> return failed_creating(dentry);
>
> + proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
> + if (!proxy_fops)
I believe we need "iput(inode);" here. Or move the allocation before
the inode allocation and free it on inode failure.
-- Steve
> + return failed_creating(dentry);
> +
> + if (!fops)
> + fops = &tracefs_file_operations;
> +
> + dentry->d_fsdata = (void *)fops;
> + memcpy(proxy_fops, fops, sizeof(*proxy_fops));
> + proxy_fops->open = default_open_file;
> inode->i_mode = mode;
> - inode->i_fop = fops ? fops : &tracefs_file_operations;
> + inode->i_fop = proxy_fops;
> inode->i_private = data;
> d_instantiate(dentry, inode);
> fsnotify_create(dentry->d_parent->d_inode, dentry);
^ permalink raw reply
* Re: [RFC PATCH] security, capability: pass object information to security_capable
From: Paul Moore @ 2019-07-24 20:12 UTC (permalink / raw)
To: Stephen Smalley, Nicholas Franck
Cc: Casey Schaufler, selinux, linux-security-module, luto,
James Morris, keescook, Serge Hallyn, john.johansen, mortonm
In-Reply-To: <e8de4a1c-7e18-fc20-e372-67bbaa93fd42@tycho.nsa.gov>
On Fri, Jul 12, 2019 at 2:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> That is part of what we want, but not the entire picture. But with
> respect to audit, the problem today is that one sees SELinux
> dac_read_search, dac_override, etc denials with no indication as to the
> particular file, which is unfortunate both from a security auditing
> perspective and from a policy development perspective. The only option
> today to gain that information is by enabling system call audit and
> setting at least one audit filter so that the audit framework will
> collect that information and include it in the audit records that are
> emitted upon syscall exit after any such AVC denial.
It might be worth looking into adding some functionality to allow the
LSMs to have some control about when/where to enable auditing. While
selectively adding records once the denial is triggered isn't going to
be possible (it's too late to collect some of the information we would
want, e.g. file information), it should be possible to allow the LSM
to trigger per-process audit record collection without too much
trouble. We could potentially take it a step further and on those
processes which have had auditing enabled by the LSM we could allow
the LSM to select when these additional records would be emitted (e.g.
on access denial); you would still incur the collection overhead, but
the logs would be much less.
> And even when one can enable
> syscall audit, one must correlate the syscall audit record to the
> associated AVC record to identify the object rather than having the
> information directly included in the same record.
We've been doing a lot of work to associate related records so that
you don't have to deal with the problem you're describing. If you are
still seeing problematic records on a modern kernel I would like to
hear about it.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* KASAN: use-after-free Read in keyring_compare_object
From: syzbot @ 2019-07-24 15:46 UTC (permalink / raw)
To: dhowells, jmorris, keyrings, linux-kernel, linux-security-module,
serge, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: abdfd52a Merge tag 'armsoc-defconfig' of git://git.kernel...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11be4894600000
kernel config: https://syzkaller.appspot.com/x/.config?x=b8e53b1e149c0183
dashboard link: https://syzkaller.appspot.com/bug?extid=9a02c5074e2307825994
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
userspace arch: i386
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+9a02c5074e2307825994@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in keyring_compare_object+0x1cb/0x220
/security/keys/keyring.c:314
Read of size 8 at addr ffff88806b52f130 by task syz-executor.1/11908
CPU: 1 PID: 11908 Comm: syz-executor.1 Not tainted 5.2.0+ #64
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack /lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 /lib/dump_stack.c:113
print_address_description.cold+0xd4/0x306 /mm/kasan/report.c:351
__kasan_report.cold+0x1b/0x36 /mm/kasan/report.c:482
kasan_report+0x12/0x17 /mm/kasan/common.c:612
__asan_report_load8_noabort+0x14/0x20 /mm/kasan/generic_report.c:132
keyring_compare_object+0x1cb/0x220 /security/keys/keyring.c:314
assoc_array_find+0x14b/0x1f0 /lib/assoc_array.c:331
search_keyring /security/keys/keyring.c:655 [inline]
search_nested_keyrings+0xb15/0xea0 /security/keys/keyring.c:722
keyring_search_rcu+0x1b4/0x290 /security/keys/keyring.c:926
search_cred_keyrings_rcu+0x17d/0x2e0 /security/keys/process_keys.c:480
search_process_keyrings_rcu+0x1d/0x320 /security/keys/process_keys.c:544
request_key_and_link+0x264/0x12b0 /security/keys/request_key.c:602
__do_sys_request_key /security/keys/keyctl.c:223 [inline]
__se_sys_request_key /security/keys/keyctl.c:168 [inline]
__ia32_sys_request_key+0x288/0x430 /security/keys/keyctl.c:168
do_syscall_32_irqs_on /arch/x86/entry/common.c:332 [inline]
do_fast_syscall_32+0x27b/0xdb3 /arch/x86/entry/common.c:403
entry_SYSENTER_compat+0x70/0x7f /arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7f309c9
Code: d3 83 c4 10 5b 5e 5d c3 ba 80 96 98 00 eb a9 8b 04 24 c3 8b 34 24 c3
8b 3c 24 c3 90 90 90 90 90 90 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90
90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:00000000f5d2c0cc EFLAGS: 00000296 ORIG_RAX: 000000000000011f
RAX: ffffffffffffffda RBX: 0000000020000000 RCX: 00000000200001c0
RDX: 0000000020000200 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
Allocated by task 20965:
save_stack+0x23/0x90 /mm/kasan/common.c:69
set_track /mm/kasan/common.c:77 [inline]
__kasan_kmalloc /mm/kasan/common.c:487 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 /mm/kasan/common.c:460
kasan_slab_alloc+0xf/0x20 /mm/kasan/common.c:495
slab_post_alloc_hook /mm/slab.h:520 [inline]
slab_alloc /mm/slab.c:3319 [inline]
kmem_cache_alloc+0x121/0x710 /mm/slab.c:3483
kmem_cache_zalloc /./include/linux/slab.h:738 [inline]
key_alloc+0x426/0x1110 /security/keys/key.c:276
key_create_or_update+0x651/0xbe0 /security/keys/key.c:924
__do_sys_add_key /security/keys/keyctl.c:132 [inline]
__se_sys_add_key /security/keys/keyctl.c:72 [inline]
__ia32_sys_add_key+0x2c2/0x4f0 /security/keys/keyctl.c:72
do_syscall_32_irqs_on /arch/x86/entry/common.c:332 [inline]
do_fast_syscall_32+0x27b/0xdb3 /arch/x86/entry/common.c:403
entry_SYSENTER_compat+0x70/0x7f /arch/x86/entry/entry_64_compat.S:139
Freed by task 2951:
save_stack+0x23/0x90 /mm/kasan/common.c:69
set_track /mm/kasan/common.c:77 [inline]
__kasan_slab_free+0x102/0x150 /mm/kasan/common.c:449
kasan_slab_free+0xe/0x10 /mm/kasan/common.c:457
__cache_free /mm/slab.c:3425 [inline]
kmem_cache_free+0x86/0x320 /mm/slab.c:3693
key_gc_unused_keys.constprop.0+0x192/0x5b0 /security/keys/gc.c:157
key_garbage_collector+0x3f3/0x940 /security/keys/gc.c:292
process_one_work+0x9af/0x1740 /kernel/workqueue.c:2269
worker_thread+0x98/0xe40 /kernel/workqueue.c:2415
kthread+0x361/0x430 /kernel/kthread.c:255
ret_from_fork+0x24/0x30 /arch/x86/entry/entry_64.S:352
The buggy address belongs to the object at ffff88806b52f040
which belongs to the cache key_jar of size 304
The buggy address is located 240 bytes inside of
304-byte region [ffff88806b52f040, ffff88806b52f170)
The buggy address belongs to the page:
page:ffffea0001ad4bc0 refcount:1 mapcount:0 mapping:ffff88821bc461c0
index:0xffff88806b52f7c0
flags: 0x1fffc0000000200(slab)
raw: 01fffc0000000200 ffffea00019b3648 ffffea0001a834c8 ffff88821bc461c0
raw: ffff88806b52f7c0 ffff88806b52f040 0000000100000005 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88806b52f000: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff88806b52f080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff88806b52f100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc
^
ffff88806b52f180: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
ffff88806b52f200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Roberto Sassu @ 2019-07-24 15:34 UTC (permalink / raw)
To: Rob Landley, hpa, Arvind Sankar
Cc: Mimi Zohar, viro, linux-security-module, linux-integrity,
initramfs, linux-api, linux-fsdevel, linux-kernel, bug-cpio,
zohar, silviu.vlasceanu, dmitry.kasatkin, takondra, kamensky,
arnd, james.w.mcmechan
In-Reply-To: <f85ed711-f583-51cd-34e2-80018a592280@huawei.com>
Is there anything I didn't address in this patch set, that is delaying
the review? I would appreciate if you can give me a feedback, positive
or negative.
Thanks a lot!
Roberto
On 7/15/2019 6:54 PM, Roberto Sassu wrote:
> Rob, Peter, Arvind, did you have the chance to have a look at this
> version of the patch set?
>
> Thanks
>
> Roberto
>
>
> On 7/1/2019 4:31 PM, Mimi Zohar wrote:
>> On Mon, 2019-07-01 at 16:42 +0300, Roberto Sassu wrote:
>>> On 6/30/2019 6:39 PM, Mimi Zohar wrote:
>>>> On Wed, 2019-06-26 at 10:15 +0200, Roberto Sassu wrote:
>>>>> On 6/3/2019 8:32 PM, Rob Landley wrote:
>>>>>> On 6/3/19 4:31 AM, Roberto Sassu wrote:
>>>>>>>> This patch set aims at solving the following use case: appraise
>>>>>>>> files from
>>>>>>>> the initial ram disk. To do that, IMA checks the signature/hash
>>>>>>>> from the
>>>>>>>> security.ima xattr. Unfortunately, this use case cannot be
>>>>>>>> implemented
>>>>>>>> currently, as the CPIO format does not support xattrs.
>>>>>>>>
>>>>>>>> This proposal consists in including file metadata as additional
>>>>>>>> files named
>>>>>>>> METADATA!!!, for each file added to the ram disk. The CPIO
>>>>>>>> parser in the
>>>>>>>> kernel recognizes these special files from the file name, and
>>>>>>>> calls the
>>>>>>>> appropriate parser to add metadata to the previously extracted
>>>>>>>> file. It has
>>>>>>>> been proposed to use bit 17:16 of the file mode as a way to
>>>>>>>> recognize files
>>>>>>>> with metadata, but both the kernel and the cpio tool declare the
>>>>>>>> file mode
>>>>>>>> as unsigned short.
>>>>>>>
>>>>>>> Any opinion on this patch set?
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Roberto
>>>>>>
>>>>>> Sorry, I've had the window open since you posted it but haven't
>>>>>> gotten around to
>>>>>> it. I'll try to build it later today.
>>>>>>
>>>>>> It does look interesting, and I have no objections to the basic
>>>>>> approach. I
>>>>>> should be able to add support to toybox cpio over a weekend once
>>>>>> I've got the
>>>>>> kernel doing it to test against.
>>>>>
>>>>> Ok.
>>>>>
>>>>> Let me give some instructions so that people can test this patch set.
>>>>>
>>>>> To add xattrs to the ram disk embedded in the kernel it is sufficient
>>>>> to set CONFIG_INITRAMFS_FILE_METADATA="xattr" and
>>>>> CONFIG_INITRAMFS_SOURCE="<file with xattr>" in the kernel
>>>>> configuration.
>>>>>
>>>>> To add xattrs to the external ram disk, it is necessary to patch cpio:
>>>>>
>>>>> https://github.com/euleros/cpio/commit/531cabc88e9ecdc3231fad6e4856869baa9a91ef
>>>>>
>>>>> (xattr-v1 branch)
>>>>>
>>>>> and dracut:
>>>>>
>>>>> https://github.com/euleros/dracut/commit/a2dee56ea80495c2c1871bc73186f7b00dc8bf3b
>>>>>
>>>>> (digest-lists branch)
>>>>>
>>>>> The same modification can be done for mkinitramfs (add '-e xattr'
>>>>> to the
>>>>> cpio command line).
>>>>>
>>>>> To simplify the test, it would be sufficient to replace only the cpio
>>>>> binary and the dracut script with the modified versions. For
>>>>> dracut, the
>>>>> patch should be applied to the local dracut (after it has been renamed
>>>>> to dracut.sh).
>>>>>
>>>>> Then, run:
>>>>>
>>>>> dracut -e xattr -I <file with xattr> (add -f to overwrite the ram
>>>>> disk)
>>>>>
>>>>> Xattrs can be seen by stopping the boot process for example by adding
>>>>> rd.break to the kernel command line.
>>>>
>>>> A simple way of testing, without needing any changes other than the
>>>> kernel patches, is to save the dracut temporary directory by supplying
>>>> "--keep" on the dracut command line, calling
>>>> usr/gen_initramfs_list.sh, followed by usr/gen_init_cpio with the "-e
>>>> xattr" option.
>>>
>>> Alternatively, follow the instructions to create the embedded ram disk
>>> with xattrs, and use the existing external ram disk created with dracut
>>> to check if xattrs are created.
>>
>> True, but this alternative is for those who normally use dracut to
>> create an initramfs, but don't want to update cpio or dracut.
>>
>> Mimi
>>
>
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
^ permalink raw reply
* Re: Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Eric Biggers @ 2019-07-24 5:00 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-security-module, syzkaller-bugs
In-Reply-To: <737a46de-cfb0-6220-c796-563ffd3ff325@i-love.sakura.ne.jp>
On Wed, Jul 24, 2019 at 01:54:40PM +0900, Tetsuo Handa wrote:
> On 2019/07/24 13:34, Eric Biggers wrote:
> > On Wed, Jul 24, 2019 at 12:18:47PM +0900, Tetsuo Handa wrote:
> >>> --------------------------------------------------------------------------------
> >>> Title: KASAN: invalid-free in tomoyo_realpath_from_path
> >>> Last occurred: 57 days ago
> >>> Reported: 56 days ago
> >>> Branches: net-next
> >>> Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
> >>> Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
> >>
> >> This cannot be a TOMOYO's bug. We are waiting for a reproducer but
> >> no crash occurred since then. Maybe it is time to close as invalid.
> >
> > Maybe. Did you check for stack buffer overflows in the functions that
> > tomoyo_realpath_from_path() calls? Perhaps something is corrupting the 'buf'
> > variable in the parent's stack frame.
> >
>
> What do you mean? If this crash were a stack buffer overflow, this crash
> should have already occurred again.
>
Well not necessarily, it could be very rare.
That being said, it was only seen on net-next and only once; so it could have
been caused by some broken patch elsewhere in the kernel that was only present
for a short time.
So if you aren't going to do anything else with this, please just go ahead and
invalidate it.
> Since the "buf" variable is a local variable, it cannot be shared between
> two threads. Since "buf" is assigned as
>
> buf = kmalloc(buf_len, GFP_NOFS);
>
> and nobody else is reassigning "buf",
>
> kfree(buf);
>
> can't become an invalid free.
>
- Eric
^ permalink raw reply
* Re: Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Tetsuo Handa @ 2019-07-24 4:54 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-security-module, syzkaller-bugs
In-Reply-To: <20190724043450.GZ643@sol.localdomain>
On 2019/07/24 13:34, Eric Biggers wrote:
> On Wed, Jul 24, 2019 at 12:18:47PM +0900, Tetsuo Handa wrote:
>>> --------------------------------------------------------------------------------
>>> Title: KASAN: invalid-free in tomoyo_realpath_from_path
>>> Last occurred: 57 days ago
>>> Reported: 56 days ago
>>> Branches: net-next
>>> Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
>>> Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
>>
>> This cannot be a TOMOYO's bug. We are waiting for a reproducer but
>> no crash occurred since then. Maybe it is time to close as invalid.
>
> Maybe. Did you check for stack buffer overflows in the functions that
> tomoyo_realpath_from_path() calls? Perhaps something is corrupting the 'buf'
> variable in the parent's stack frame.
>
What do you mean? If this crash were a stack buffer overflow, this crash
should have already occurred again.
Since the "buf" variable is a local variable, it cannot be shared between
two threads. Since "buf" is assigned as
buf = kmalloc(buf_len, GFP_NOFS);
and nobody else is reassigning "buf",
kfree(buf);
can't become an invalid free.
^ permalink raw reply
* Re: Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Eric Biggers @ 2019-07-24 4:34 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-security-module, syzkaller-bugs
In-Reply-To: <7800c28f-bf1c-56cd-c82e-b1ff174ccbc8@i-love.sakura.ne.jp>
On Wed, Jul 24, 2019 at 12:18:47PM +0900, Tetsuo Handa wrote:
> > --------------------------------------------------------------------------------
> > Title: KASAN: invalid-free in tomoyo_realpath_from_path
> > Last occurred: 57 days ago
> > Reported: 56 days ago
> > Branches: net-next
> > Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
> > Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
>
> This cannot be a TOMOYO's bug. We are waiting for a reproducer but
> no crash occurred since then. Maybe it is time to close as invalid.
Maybe. Did you check for stack buffer overflows in the functions that
tomoyo_realpath_from_path() calls? Perhaps something is corrupting the 'buf'
variable in the parent's stack frame.
- Eric
^ permalink raw reply
* Re: Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Tetsuo Handa @ 2019-07-24 3:18 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-security-module, syzkaller-bugs
In-Reply-To: <20190724024251.GF643@sol.localdomain>
On 2019/07/24 11:42, Eric Biggers wrote:
> --------------------------------------------------------------------------------
> Title: KASAN: use-after-free Read in tomoyo_realpath_from_path
> Last occurred: 28 days ago
> Reported: 48 days ago
> Branches: Mainline and others
> Dashboard link: https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
> Original thread: https://lkml.kernel.org/lkml/0000000000004f43fa058a97f4d3@google.com/T/#u
A patch is available, but I can't find a chance to setup my git tree for sending
a pull request for the patch.
> --------------------------------------------------------------------------------
> Title: KASAN: invalid-free in tomoyo_realpath_from_path
> Last occurred: 57 days ago
> Reported: 56 days ago
> Branches: net-next
> Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
> Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
This cannot be a TOMOYO's bug. We are waiting for a reproducer but
no crash occurred since then. Maybe it is time to close as invalid.
^ permalink raw reply
* Reminder: 1 open syzbot bug in "security/integrity" subsystem
From: Eric Biggers @ 2019-07-24 2:49 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 1 of them as possibly being a bug in the "security/integrity" subsystem.
If you believe this 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 this bug to the "security/integrity" subsystem,
please let me know, and if possible forward the report to the correct people or
mailing list.
Here is the bug:
--------------------------------------------------------------------------------
Title: INFO: task hung in process_measurement
Last occurred: 133 days ago
Reported: 295 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, 120 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
* Reminder: 1 open syzbot bug in "security/smack" subsystem
From: Eric Biggers @ 2019-07-24 2:47 UTC (permalink / raw)
To: linux-security-module, Casey Schaufler, 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 1 of them as possibly being a bug in the "security/smack" subsystem.
If you believe this 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 this bug to the "security/smack" subsystem,
please let me know, and if possible forward the report to the correct people or
mailing list.
Here is the bug:
--------------------------------------------------------------------------------
Title: possible deadlock in ext4_evict_inode
Last occurred: 281 days ago
Reported: 320 days ago
Branches: Mainline
Dashboard link: https://syzkaller.appspot.com/bug?id=9eda6092f146cb23cb9109f675a2e2cb743ee48b
Original thread: https://lkml.kernel.org/lkml/00000000000091615e0575368e33@google.com/T/#u
This bug has a syzkaller reproducer only.
The original thread for this bug received 2 replies; the last was 320 days ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0eefc1e06a77d327a056@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/00000000000091615e0575368e33@google.com
^ permalink raw reply
* Reminder: 2 open syzbot bugs in "security/tomoyo" subsystem
From: Eric Biggers @ 2019-07-24 2:42 UTC (permalink / raw)
To: linux-security-module, Kentaro Takeda, Tetsuo Handa, 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/tomoyo" 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/tomoyo" subsystem, please
let me know, and if possible forward the report to the correct people or mailing
list.
Here are the bugs:
--------------------------------------------------------------------------------
Title: KASAN: use-after-free Read in tomoyo_realpath_from_path
Last occurred: 28 days ago
Reported: 48 days ago
Branches: Mainline and others
Dashboard link: https://syzkaller.appspot.com/bug?id=73d590010454403d55164cca23bd0565b1eb3b74
Original thread: https://lkml.kernel.org/lkml/0000000000004f43fa058a97f4d3@google.com/T/#u
This bug has a syzkaller reproducer only.
The original thread for this bug has received 7 replies; the last was 31 days
ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+0341f6a4d729d4e0acf1@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/0000000000004f43fa058a97f4d3@google.com
--------------------------------------------------------------------------------
Title: KASAN: invalid-free in tomoyo_realpath_from_path
Last occurred: 57 days ago
Reported: 56 days ago
Branches: net-next
Dashboard link: https://syzkaller.appspot.com/bug?id=e9e5a1d41c3fb5d0f79aeea0e4cd535f160a6702
Original thread: https://lkml.kernel.org/lkml/000000000000785e9d0589ec359a@google.com/T/#u
Unfortunately, this bug does not have a reproducer.
The original thread for this bug has received 1 reply, 56 days ago.
If you fix this bug, please add the following tag to the commit:
Reported-by: syzbot+9742b1c6c7aedf18beda@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/000000000000785e9d0589ec359a@google.com
^ permalink raw reply
* Re: [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around
From: Al Viro @ 2019-07-23 23:30 UTC (permalink / raw)
To: James Morris
Cc: Eric W. Biederman, Tetsuo Handa, David Howells, linux-api,
linux-fsdevel, torvalds, linux-security-module
In-Reply-To: <alpine.LRH.2.21.1907240744080.16974@namei.org>
On Wed, Jul 24, 2019 at 07:45:07AM +1000, James Morris wrote:
> On Mon, 8 Jul 2019, Al Viro wrote:
>
> > On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
> > > On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote:
> > >
> > > > Al you do realize that the TOCTOU you are talking about comes the system
> > > > call API. TOMOYO can only be faulted for not playing in their own
> > > > sandbox and not reaching out and fixing the vfs implementation details.
> >
> > PS: the fact that mount(2) has been overloaded to hell and back (including
> > MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount()
> > and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2.
> >
> > In all the years since the introduction of ->sb_mount() I've seen zero
> > questions from LSM folks regarding a sane place for those checks. What I have
> > seen was "we want it immediately upon the syscall entry, let the module
> > figure out what to do" in reply to several times I tried to tell them "folks,
> > it's called in a bad place; you want the checks applied to objects, not to
> > raw string arguments".
> >
> > As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount();
> > there are other hooks also in the game for remounts and new mounts).
>
> What are your recommendations for placing these checks?
For MS_MOVE: do_move_mount(), after lock_mount(), when the mount tree is stable
and pathnames are resolved.
For MS_BIND: do_loopback(), ditto.
Incidentally, for pivot_root(2) I would also suggest moving that past the
lock_mount(), for the same reasons.
For propagation flag changes: do_change_type(), after namespace_lock().
For per-mount flag changes: do_reconfigure_mnt(), possibly after having
grabbed ->s_umount.
For fs remount: IMO it should be handled purely in ->sb_remount().
For new mount: really depends upon the filesystem type, I'm afraid. There's
nothing type-independent that can be done - in the best case you can say
"no mounting block filesystems from that device"; note that for NFS the
meaning of the argument is entirely different and you *can* have something
like foo.local.org: as a name of symlink (or directory), so blanket "you can
mount foo.local.org:/srv/nfs/blah" is asking for trouble -
mount -t ext4 foo.local.org:/srv/nfs/blah /mnt can bloody well end up
successfully mounting a very untrusted usb disk.
Note, BTW, that things like cramfs can be given
* mtd:mtd_device_name
* mtd<decimal number>
* block device pathname
The last one needs to be resolved/canonicalized/whatnot.
The other two must *NOT* - there's nothing to stop the
attacker from ln -s /dev/mtdblock0 ./mtd12 and confusing
the fsck out of your LSM (it would assume that we are
trying to get mtd0 when in reality it'll mount mtd12).
The rules really need to be type-dependent; ->sb_set_mnt_opts() has the
state after the fs has been initialized to work with, but anything trying
to stop the attempt to set the damn thing up in the first place (as
current ->sb_mount() would) must be called from the inside of individual
->get_tree()/->mount() instance (or a helper used by such).
^ permalink raw reply
* Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
From: Thiago Jung Bauermann @ 2019-07-23 22:39 UTC (permalink / raw)
To: Philipp Rudo
Cc: Jessica Yu, linux-integrity, linux-security-module, keyrings,
linux-crypto, linuxppc-dev, linux-doc, linux-kernel, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, David Howells,
David Woodhouse, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Heiko Carstens, linux-s390
In-Reply-To: <20190705150000.372345b0@laptop-ibm>
Hello Philipp,
Philipp Rudo <prudo@linux.ibm.com> writes:
> Hi Thiago,
>
> On Thu, 04 Jul 2019 15:57:34 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
>> Hello Philipp,
>>
>> Philipp Rudo <prudo@linux.ibm.com> writes:
>>
>> > Hi Thiago,
>> >
>> >
>> > On Thu, 04 Jul 2019 03:42:57 -0300
>> > Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>> >
>> >> Jessica Yu <jeyu@kernel.org> writes:
>> >>
>> >> > +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:
>> >> >>IMA will use the module_signature format for append signatures, so export
>> >> >>the relevant definitions and factor out the code which verifies that the
>> >> >>appended signature trailer is valid.
>> >> >>
>> >> >>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
>> >> >>and be able to use mod_check_sig() without having to depend on either
>> >> >>CONFIG_MODULE_SIG or CONFIG_MODULES.
>> >> >>
>> >> >>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> >> >>Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> >> >>Cc: Jessica Yu <jeyu@kernel.org>
>> >> >>---
>> >> >> include/linux/module.h | 3 --
>> >> >> include/linux/module_signature.h | 44 +++++++++++++++++++++++++
>> >> >> init/Kconfig | 6 +++-
>> >> >> kernel/Makefile | 1 +
>> >> >> kernel/module.c | 1 +
>> >> >> kernel/module_signature.c | 46 ++++++++++++++++++++++++++
>> >> >> kernel/module_signing.c | 56 +++++---------------------------
>> >> >> scripts/Makefile | 2 +-
>> >> >> 8 files changed, 106 insertions(+), 53 deletions(-)
>> >> >>
>> >> >>diff --git a/include/linux/module.h b/include/linux/module.h
>> >> >>index 188998d3dca9..aa56f531cf1e 100644
>> >> >>--- a/include/linux/module.h
>> >> >>+++ b/include/linux/module.h
>> >> >>@@ -25,9 +25,6 @@
>> >> >> #include <linux/percpu.h>
>> >> >> #include <asm/module.h>
>> >> >>
>> >> >>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
>> >> >>-#define MODULE_SIG_STRING "~Module signature appended~\n"
>> >> >>-
>> >> >
>> >> > Hi Thiago, apologies for the delay.
>> >>
>> >> Hello Jessica, thanks for reviewing the patch!
>> >>
>> >> > It looks like arch/s390/kernel/machine_kexec_file.c also relies on
>> >> > MODULE_SIG_STRING being defined, so module_signature.h will need to be
>> >> > included there too, otherwise we'll run into a compilation error.
>> >>
>> >> Indeed. Thanks for spotting that. The patch below fixes it. It's
>> >> identical to the previous version except for the changes in
>> >> arch/s390/kernel/machine_kexec_file.c and their description in the
>> >> commit message. I'm also copying some s390 people in this email.
>> >
>> > to me the s390 part looks good but for one minor nit.
>>
>> Thanks for the prompt review!
>>
>> > In arch/s390/Kconfig KEXEC_VERIFY_SIG currently depends on
>> > SYSTEM_DATA_VERIFICATION. I'd prefer when you update this to the new
>> > MODULE_SIG_FORMAT. It shouldn't make any difference right now, as we don't
>> > use mod_check_sig in our code path. But it could cause problems in the future,
>> > when more code might be shared.
>>
>> Makes sense. Here is the updated patch with the Kconfig change.
>>
>
> The patch looks good now.
Thanks! Can I add your Reviewed-by?
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: James Morris @ 2019-07-23 21:46 UTC (permalink / raw)
To: Simon McVittie
Cc: Paul Moore, Casey Schaufler, Steve Grubb, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list
In-Reply-To: <20190723140634.GA30188@horizon>
On Tue, 23 Jul 2019, Simon McVittie wrote:
> On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
> > On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > I suggest that if supporting dbus well is assisted by
> > > making reasonable restrictions on what constitutes a valid LSM
> > > "context" that we have a good reason.
> >
> > I continue to believe that restrictions on the label format are a bad
> > idea
>
> Does this include the restriction "the label does not include \0",
> which is an assumption that dbus is already relying on since I checked
> it in the thread around
> <https://marc.info/?l=linux-security-module&m=142323508321029&w=2>?
> Or is that restriction so fundamental that it's considered OK?
Security labels are strings, so this is implied.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around
From: James Morris @ 2019-07-23 21:45 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, Tetsuo Handa, David Howells, linux-api,
linux-fsdevel, torvalds, linux-security-module
In-Reply-To: <20190708202124.GX17978@ZenIV.linux.org.uk>
On Mon, 8 Jul 2019, Al Viro wrote:
> On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
> > On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote:
> >
> > > Al you do realize that the TOCTOU you are talking about comes the system
> > > call API. TOMOYO can only be faulted for not playing in their own
> > > sandbox and not reaching out and fixing the vfs implementation details.
>
> PS: the fact that mount(2) has been overloaded to hell and back (including
> MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount()
> and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2.
>
> In all the years since the introduction of ->sb_mount() I've seen zero
> questions from LSM folks regarding a sane place for those checks. What I have
> seen was "we want it immediately upon the syscall entry, let the module
> figure out what to do" in reply to several times I tried to tell them "folks,
> it's called in a bad place; you want the checks applied to objects, not to
> raw string arguments".
>
> As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount();
> there are other hooks also in the game for remounts and new mounts).
What are your recommendations for placing these checks?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [RFC PATCH v3] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-23 19:27 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient for the needs of SELinux. This is due to the fact
that read access should not necessarily imply access to information about
when another process reads from a file. Furthermore, fanotify watches grant
more power to an application in the form of permission events. While
notification events are solely, unidirectional (i.e. they only pass
information to the receiving application), permission events are blocking.
Permission events make a request to the receiving application which will
then reply with a decision as to whether or not that action may be
completed. This causes the issue of the watching application having the
ability to exercise control over the triggering process. Without drawing a
distinction within the permission check, the ability to read would imply
the greater ability to control an application. Additionally, mount and
superblock watches apply to all files within the same mount or superblock.
Read access to one file should not necessarily imply the ability to watch
all files accessed within a given mount or superblock.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target path has been resolved and are provided with the
path struct, the mask of requested notification events, and the type of
object on which the mark is being set (inode, superblock, or mount). The
mask and obj_type have already been translated into common FS_* values
shared by the entirety of the fs notification infrastructure. The path
struct is passed rather than just the inode so that the mount is available,
particularly for mount watches. This also allows for use of the hook by
pathname-based security modules. However, since the hook is intended for
use even by inode based security modules, it is not placed under the
CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security
modules would need to enable all of the path hooks, even though they do not
use any of them.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Security modules wishing to provide complete control over fanotify must
also implement a security_file_open hook that validates that the access
requested by the watching application is authorized. Fanotify has the issue
that it returns a file descriptor with the file mode specified during
fanotify_init() to the watching process on event. This is already covered
by the LSM security_file_open hook if the security module implements
checking of the requested file mode there. Otherwise, a watching process
can obtain escalated access to a file for which it has not been authorized.
The selinux_path_notify hook implementation works by adding five new file
permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
(descriptions about which will follow), and one new filesystem permission:
watch (which is applied to superblock checks). The hook then decides which
subset of these permissions must be held by the requesting application
based on the contents of the provided mask and the obj_type. The
selinux_file_open hook already checks the requested file mode and therefore
ensures that a watching process cannot escalate its access through
fanotify.
The watch, watch_mount, and watch_sb permissions are the baseline
permissions for setting a watch on an object and each are a requirement for
any watch to be set on a file, mount, or superblock respectively. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch, watch_mount, or watch_sb
permission. Superblock watches further require the filesystem watch
permission to the superblock. As there is no labeled object in view for
mounts, there is no specific check for mount watches beyond watch_mount to
the inode. Such a check could be added in the future, if a suitable labeled
object existed representing the mount.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
---
v3:
- Renames mark_type to obj_type for clarity
- Hook now receives path and not inode
v2:
- Adds support for mark_type
- Adds watch_sb and watch_mount file permissions
- Adds watch as new filesystem permission
- LSM hook now recieves mark_type argument
- Changed LSM hook logic to implement checks for corresponding mark_types
- Adds missing hook description comment
- Fixes extrainous whitespace
- Updates patch description based on feedback
fs/notify/dnotify/dnotify.c | 15 +++++++--
fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
fs/notify/inotify/inotify_user.c | 13 ++++++--
include/linux/lsm_hooks.h | 9 +++++-
include/linux/security.h | 10 ++++--
security/security.c | 6 ++++
security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 5 +--
8 files changed, 120 insertions(+), 12 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..87a7f61bc91c 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,17 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_path_notify(&filp->f_path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +314,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..b83f27021f54 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask)
{
int ret;
+ unsigned int obj_type;
pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
dfd, filename, flags);
@@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ switch (flags & FANOTIFY_MARK_TYPE_BITS) {
+ case FAN_MARK_MOUNT:
+ obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+ break;
+ case FAN_MARK_FILESYSTEM:
+ obj_type = FSNOTIFY_OBJ_TYPE_SB;
+ break;
+ case FAN_MARK_INODE:
+ obj_type = FSNOTIFY_OBJ_TYPE_INODE;
+ break;
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = security_path_notify(path, mask, obj_type);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..e0d593c2d437 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_path_notify(path, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..ead98af9c602 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -339,6 +339,9 @@
* Check for permission to change root directory.
* @path contains the path structure.
* Return 0 if permission is granted.
+ * @path_notify:
+ * Check permissions before setting a watch on events as defined by @mask,
+ * on an object at @path, whose type is defined by @obj_type.
* @inode_readlink:
* Check the permission to read the symbolic link.
* @dentry contains the dentry structure for the file link.
@@ -1535,7 +1538,9 @@ union security_list_options {
int (*path_chown)(const struct path *path, kuid_t uid, kgid_t gid);
int (*path_chroot)(const struct path *path);
#endif
-
+ /* Needed for inode based security check */
+ int (*path_notify)(const struct path *path, u64 mask,
+ unsigned int obj_type);
int (*inode_alloc_security)(struct inode *inode);
void (*inode_free_security)(struct inode *inode);
int (*inode_init_security)(struct inode *inode, struct inode *dir,
@@ -1860,6 +1865,8 @@ struct security_hook_heads {
struct hlist_head path_chown;
struct hlist_head path_chroot;
#endif
+ /* Needed for inode based modules as well */
+ struct hlist_head path_notify;
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..7d9c1da1f659 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -259,7 +259,8 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode,
struct qstr *name,
const struct cred *old,
struct cred *new);
-
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type);
int security_inode_alloc(struct inode *inode);
void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -387,7 +388,6 @@ int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
-
void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -621,6 +621,12 @@ static inline int security_move_mount(const struct path *from_path,
return 0;
}
+static inline int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return 0;
+}
+
static inline int security_inode_alloc(struct inode *inode)
{
return 0;
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..30687e1366b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -871,6 +871,12 @@ int security_move_mount(const struct path *from_path, const struct path *to_path
return call_int_hook(move_mount, 0, from_path, to_path);
}
+int security_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ return call_int_hook(path_notify, 0, path, mask, obj_type);
+}
+
int security_inode_alloc(struct inode *inode)
{
int rc = lsm_inode_alloc(inode);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..a1aaf79421df 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,8 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
+#include <linux/fanotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_path_notify(const struct path *path, u64 mask,
+ unsigned int obj_type)
+{
+ int ret;
+ u32 perm;
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_PATH;
+ ad.u.path = *path;
+
+ /*
+ * Set permission needed based on the type of mark being set.
+ * Performs an additional check for sb watches.
+ */
+ switch (obj_type) {
+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+ perm = FILE__WATCH_MOUNT;
+ break;
+ case FSNOTIFY_OBJ_TYPE_SB:
+ perm = FILE__WATCH_SB;
+ ret = superblock_has_perm(current_cred(), path->dentry->d_sb,
+ FILESYSTEM__WATCH, &ad);
+ if (ret)
+ return ret;
+ break;
+ case FSNOTIFY_OBJ_TYPE_INODE:
+ perm = FILE__WATCH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ // check if the mask is requesting ability to set a blocking watch
+ if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
+ perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
+
+ // is the mask asking to watch file reads?
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS; // check that permission as well
+
+ return path_has_perm(current_cred(), path, perm);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(path_notify, selinux_path_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..32e9b03be3dd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,8 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_mount", "watch_sb", \
+ "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
{ "filesystem",
{ "mount", "remount", "unmount", "getattr",
"relabelfrom", "relabelto", "associate", "quotamod",
- "quotaget", NULL } },
+ "quotaget", "watch", NULL } },
{ "file",
{ COMMON_FILE_PERMS,
"execute_no_trans", "entrypoint", NULL } },
--
2.21.0
^ permalink raw reply related
* Re: [Non-DoD Source] Re: [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-07-23 18:49 UTC (permalink / raw)
To: Aaron Goidel
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <c74ad814-f188-37c6-9b3a-51178b538a2b@tycho.nsa.gov>
On Tue, Jul 23, 2019 at 7:17 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>
> On 7/18/19 12:16 PM, Amir Goldstein wrote:
> > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index a90bb19dcfa2..9e3137badb6b 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> >> };
> >>
> >> static int fanotify_find_path(int dfd, const char __user *filename,
> >> - struct path *path, unsigned int flags)
> >> + struct path *path, unsigned int flags, __u64 mask)
> >> {
> >> int ret;
> >> + unsigned int mark_type;
> >>
> >> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> >> dfd, filename, flags);
> >> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
> >>
> >> /* you can only watch an inode if you have read permissions on it */
> >> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> >> + if (ret) {
> >> + path_put(path);
> >> + goto out;
> >> + }
> >> +
> >> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> >> + case FAN_MARK_MOUNT:
> >> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> >> + break;
> >> + case FAN_MARK_FILESYSTEM:
> >> + mark_type = FSNOTIFY_OBJ_TYPE_SB;
> >> + break;
> >> + case FAN_MARK_INODE:
> >> + mark_type = FSNOTIFY_OBJ_TYPE_INODE;
> >> + break;
> >> + default:
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +
> >> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
> >
> > If you prefer 3 hooks security_{inode,mount,sb}_notify()
> > please place them in fanotify_add_{inode,mount,sb}_mark().
> >
> > If you prefer single hook with path argument, please pass path
> > down to fanotify_add_mark() and call security_path_notify() from there,
> > where you already have the object type argument.
> >
> I'm not clear on why you want me to move the hook call down to
> fanotify_add_mark(). I'd prefer to keep it adjacent to the existing
> inode_permission() call so that all the security checking occurs from
> one place.
Fine.
> Moving it down requires adding a path arg to that entire call
> chain, even though it wouldn't otherwise be needed.
That doesn't matter.
> And that raises the
> question of whether to continue passing the mnt_sb, mnt, or inode
> separately or just extract all those from the path inside of
> fanotify_add_*_mark().
You lost me. The major issue I have is with passing @inode argument
to hook for adding a mount watch. Makes no sense to me as @inode
may be accessed from any mount and without passing @path to hook
this information is lost.
>
> It also seems to destroy the parallelism with fanotify_remove_*_mark().
I don't know what that means.
> I also don't see any real benefit in splitting into three separate
> hooks, especially as some security modules will want the path or inode
> even for the mount or superblock cases, since they may have no relevant
> security information for vfsmounts or superblocks.
OK. that is an argument for single hook with @path argument.
That is fine by me.
Thanks,
Amir.
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-23 17:32 UTC (permalink / raw)
To: Simon McVittie, Paul Moore
Cc: James Morris, Steve Grubb, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list, casey
In-Reply-To: <20190723140634.GA30188@horizon>
On 7/23/2019 7:06 AM, Simon McVittie wrote:
> On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
>> On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> I suggest that if supporting dbus well is assisted by
>>> making reasonable restrictions on what constitutes a valid LSM
>>> "context" that we have a good reason.
>> I continue to believe that restrictions on the label format are a bad
>> idea
> Does this include the restriction "the label does not include \0",
> which is an assumption that dbus is already relying on since I checked
> it in the thread around
> <https://marc.info/?l=linux-security-module&m=142323508321029&w=2>?
> Or is that restriction so fundamental that it's considered OK?
>
> (Other user-space tools like ls -Z and ps -Z also rely on that assumption
> by printing security contexts with %s, as far as I know.)
The "-Z" options for ls and ps are unfortunately hard coded for SELinux.
For applications to be general in the presence of LSMs you are correct
that there need to be some assumptions.
> dbus does not require a way to multiplex multiple LSMs' labels in a
> printable text string, so from my point of view, multiplexed labels do
> not necessarily have to be in what Casey calls the "Hideous" format,
> or in any text format at all: anything with documented rules for parsing
> (including the unescaping that readers are expected to apply, if there
> is any) would be fine. Based on the assumption of no "\0", I previously
> suggested a "\0"-delimited encoding similar to /proc/self/cmdline, which
> would not need any escaping/unescaping:
>
> "apparmor\0" <apparmor label> "\0"
> "selinux\0" <SELinux label> "\0"
> ...
> "\0" (or this could be omitted since it's redundant with the length)
>
> which would be fine (indeed, actually easier than the "Hideous" format)
> from dbus' point of view.
I am an advocate of a single string due to the preponderance of
scripting language programing in today's world. It would be easy to provide
a library function to transform the "Hideous" format into the "cmdline"
format or, I'll admit, the other way round. I'm not so set in my opinion
that if it came down to "cmdline" or nothing I wouldn't cave in.
> dbus does not strictly need reading security labels for sockets or
> processes to be atomic, either: it would be OK if we can get the complete
> list of LSM labels *somehow*, possibly in O(number of LSMs) rather than
> O(1) syscalls (although I'd prefer O(1)).
Stephen Smalley did an excellent job of outlining the dangers of
using the proposed "display" mechanism with multiple calls to
get the complete attribute set. Adding a new interface that gets
them all at once addresses that set of problems.
> However, the getsockopt() interface only lets the kernel return one thing
> per socket option, and I assume the networking maintainers probably don't
> want to have to add SO_PEERAPPARMOR, SO_PEERSELINUX... for each LSM -
Or a getsockopt() option that takes an LSM name and returns the value
for that module. You could do any of those, but you still end up with O(n)
and a need to know in advance what security modules to look for.
> so this part at least would probably be easier as a single blob in some
> text or binary format.
For the long term I agree. I still have to deal with legacy services
and applications that won't be updated in the foreseeable future, which
is why the old interfaces can't be updated. New interfaces are required.
I'm open to discussion on details, including format.
> smcv
^ permalink raw reply
* Re: [Non-DoD Source] Re: [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-23 16:16 UTC (permalink / raw)
To: Amir Goldstein
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <CAOQ4uxjCR76nbV_Lmoegaq6NqovWZD-XWEVS-X3e=BtDdjKkXQ@mail.gmail.com>
On 7/18/19 12:16 PM, Amir Goldstein wrote:
> On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index a90bb19dcfa2..9e3137badb6b 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
>> };
>>
>> static int fanotify_find_path(int dfd, const char __user *filename,
>> - struct path *path, unsigned int flags)
>> + struct path *path, unsigned int flags, __u64 mask)
>> {
>> int ret;
>> + unsigned int mark_type;
>>
>> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
>> dfd, filename, flags);
>> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>>
>> /* you can only watch an inode if you have read permissions on it */
>> ret = inode_permission(path->dentry->d_inode, MAY_READ);
>> + if (ret) {
>> + path_put(path);
>> + goto out;
>> + }
>> +
>> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
>> + case FAN_MARK_MOUNT:
>> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
>> + break;
>> + case FAN_MARK_FILESYSTEM:
>> + mark_type = FSNOTIFY_OBJ_TYPE_SB;
>> + break;
>> + case FAN_MARK_INODE:
>> + mark_type = FSNOTIFY_OBJ_TYPE_INODE;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
>
> If you prefer 3 hooks security_{inode,mount,sb}_notify()
> please place them in fanotify_add_{inode,mount,sb}_mark().
>
> If you prefer single hook with path argument, please pass path
> down to fanotify_add_mark() and call security_path_notify() from there,
> where you already have the object type argument.
>
I'm not clear on why you want me to move the hook call down to
fanotify_add_mark(). I'd prefer to keep it adjacent to the existing
inode_permission() call so that all the security checking occurs from
one place. Moving it down requires adding a path arg to that entire call
chain, even though it wouldn't otherwise be needed. And that raises the
question of whether to continue passing the mnt_sb, mnt, or inode
separately or just extract all those from the path inside of
fanotify_add_*_mark().
It also seems to destroy the parallelism with fanotify_remove_*_mark().
I also don't see any real benefit in splitting into three separate
hooks, especially as some security modules will want the path or inode
even for the mount or superblock cases, since they may have no relevant
security information for vfsmounts or superblocks.
--
Aaron
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Simon McVittie @ 2019-07-23 14:06 UTC (permalink / raw)
To: Paul Moore
Cc: Casey Schaufler, James Morris, Steve Grubb, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list
In-Reply-To: <CAHC9VhSbg7BxPSA4NkQV3_1zx6cj3ej25e6fJ2FBWX9fU104rg@mail.gmail.com>
On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
> On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > I suggest that if supporting dbus well is assisted by
> > making reasonable restrictions on what constitutes a valid LSM
> > "context" that we have a good reason.
>
> I continue to believe that restrictions on the label format are a bad
> idea
Does this include the restriction "the label does not include \0",
which is an assumption that dbus is already relying on since I checked
it in the thread around
<https://marc.info/?l=linux-security-module&m=142323508321029&w=2>?
Or is that restriction so fundamental that it's considered OK?
(Other user-space tools like ls -Z and ps -Z also rely on that assumption
by printing security contexts with %s, as far as I know.)
dbus does not require a way to multiplex multiple LSMs' labels in a
printable text string, so from my point of view, multiplexed labels do
not necessarily have to be in what Casey calls the "Hideous" format,
or in any text format at all: anything with documented rules for parsing
(including the unescaping that readers are expected to apply, if there
is any) would be fine. Based on the assumption of no "\0", I previously
suggested a "\0"-delimited encoding similar to /proc/self/cmdline, which
would not need any escaping/unescaping:
"apparmor\0" <apparmor label> "\0"
"selinux\0" <SELinux label> "\0"
...
"\0" (or this could be omitted since it's redundant with the length)
which would be fine (indeed, actually easier than the "Hideous" format)
from dbus' point of view.
dbus does not strictly need reading security labels for sockets or
processes to be atomic, either: it would be OK if we can get the complete
list of LSM labels *somehow*, possibly in O(number of LSMs) rather than
O(1) syscalls (although I'd prefer O(1)).
However, the getsockopt() interface only lets the kernel return one thing
per socket option, and I assume the networking maintainers probably don't
want to have to add SO_PEERAPPARMOR, SO_PEERSELINUX... for each LSM -
so this part at least would probably be easier as a single blob in some
text or binary format.
smcv
^ 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