From: Eric W. Biederman <hidden> Date: 2019-07-09 00:14:01
Al Viro [off-list ref] writes:
On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
quoted
On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote:
quoted
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).
I see no point whatsoever trying to duplicate ->sb_mount() on the top level
of move_mount(2). When and if sane checks are agreed upon for that thing,
they certainly should be used both for MS_MOVE case of mount(2) and for
move_mount(2). And that'll come for free from calling those in do_move_mount().
They won't be the first thing called in mount(2) - we demultiplex first,
decide that we have a move and do pathname resolution on source. And that's
precisely what we need to avoid the TOCTOU there.
I'm sorry, but this "run the hook at the very beginning, the modules know
better what they want, just give them as close to syscall arguments as
possible before even looking at the flags" model is wrong, plain and simple.
As for the MS_MOVE checks, the arguments are clear enough (two struct path *,
same as what we pass to do_move_mount()). AFAICS, only tomoyo and
apparmor are trying to do anything for MS_MOVE in ->sb_mount(), and both
look like it should be easy enough to implement what said checks intend
to do (probably - assuming that aa_move_mount() doesn't depend upon
having its kern_path() inside the __begin_current_label_crit_section()/
__end_current_label_crit_section() pair; looks like it shouldn't be,
but that's for apparmor folks to decide).
That's really for LSM folks, though - I've given up on convincing
(or even getting a rationale out of) them on anything related to hook
semantics years ago.
I have found the LSM folks in recent years to be rather reasonable,
especially when something concrete has been proposed.
A quick look suggests that the new security_mount_move is a reasonable
hook for the mount_move check.
Tetsuo, do you think you can implement the checks you need for Tomoyo
for mount_move on top of the new security_mount_move?
Al is proposing that similar hooks be added for the other subcases of
mount so that less racy hooks can be implemented. Tetsuo do you have
any problem with that?
Tetsuo whatever the virtues of this patchset getting merged into Linus's
tree it is merged now, so the only thing we can do now is roll up our
sleeves go through everything and fix the regressions/bugs/issues that
have emerged with the new mount api.
Eric
Tetsuo, do you think you can implement the checks you need for Tomoyo
for mount_move on top of the new security_mount_move?
I hope "Yes", for I'm not aware what will become possible with the new mount
syscalls. For example, if it becomes possible to use multiple source device
files or directories, TOMOYO is not ready to check multiple source device
files or directories, for currently TOMOYO uses
file mount $DEVICE $MOUNTPOINT $FILESYSTEM $OPTIONS
( https://tomoyo.osdn.jp/2.6/policy-specification/domain-policy-syntax.html#file_mount )
syntax. If only optional part (any arguments which were previously passed via
"const void *data" of mount() syscall) differs, current syntax can be reused.
Al is proposing that similar hooks be added for the other subcases of
mount so that less racy hooks can be implemented. Tetsuo do you have
any problem with that?
I welcome improvements that get rid of calling kern_path() from LSM hooks.
In the past, I incorrectly implemented which MS_* flag should be checked
(commit df91e49477a9be15 ("TOMOYO: Fix mount flags checking order.")).
If LSM hooks for mount manipulation are divided into multiple LSM hooks
(along with changing the argument from "const char *" to "const struct path *"),
such mistakes can be avoided.
Tetsuo whatever the virtues of this patchset getting merged into Linus's
tree it is merged now, so the only thing we can do now is roll up our
sleeves go through everything and fix the regressions/bugs/issues that
have emerged with the new mount api.
For now, can we apply this patch for 5.2-stable ?
From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 9 Jul 2019 19:05:49 +0900
Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
I did not see AppArmor patch for this problem in 5.3-rc1.
John, are you OK with this patch for 5.2-stable and 5.3 ?
On 2019/07/09 19:51, Tetsuo Handa wrote:
quoted hunk
For now, can we apply this patch for 5.2-stable ?
quoted
From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 9 Jul 2019 19:05:49 +0900
Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
From: John Johansen <john.johansen@canonical.com> Date: 2019-07-23 04:16:58
On 7/22/19 3:12 AM, Tetsuo Handa wrote:
I did not see AppArmor patch for this problem in 5.3-rc1.
John, are you OK with this patch for 5.2-stable and 5.3 ?
yes, I have some larger mount rework and clean-up to do and an apparmor
patch for this is waiting on that. Looking at the thread I am wondering
where my previous reply is, probably lost in a mail client crash, I have
had a few of those lately.
Acked-by: John Johansen <john.johansen@canonical.com>
On 2019/07/09 19:51, Tetsuo Handa wrote:
quoted
For now, can we apply this patch for 5.2-stable ?
quoted
From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 9 Jul 2019 19:05:49 +0900
Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
Al, will you send this patch to 5.3-rcX via vfs.git tree?
On 2019/07/23 13:16, John Johansen wrote:
On 7/22/19 3:12 AM, Tetsuo Handa wrote:
quoted
I did not see AppArmor patch for this problem in 5.3-rc1.
John, are you OK with this patch for 5.2-stable and 5.3 ?
yes, I have some larger mount rework and clean-up to do and an apparmor
patch for this is waiting on that. Looking at the thread I am wondering
where my previous reply is, probably lost in a mail client crash, I have
had a few of those lately.
Acked-by: John Johansen <john.johansen@canonical.com>
quoted
On 2019/07/09 19:51, Tetsuo Handa wrote:
quoted
For now, can we apply this patch for 5.2-stable ?
quoted
From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 9 Jul 2019 19:05:49 +0900
Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: John Johansen <john.johansen@canonical.com>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2019-08-22 03:51:45
[bringing a private thread back to the lists]
There's a bug in interplay of fsmount() and d_absolute_path().
Namely, the check in d_absolute_path() treats the
not-yet-attached mount as "reached absolute root".
AFAICS, the right fix is this
@@ -116,8 +116,10 @@ static int prepend_path(const struct path *path,vfsmnt=&mnt->mnt;continue;}-if(!error)-error=is_mounted(vfsmnt)?1:2;+if(is_mounted(vfsmnt)&&!is_anon_ns(mnt->mnt_ns))+error=1;// absolute root+else+error=2;// detached or not attached yetbreak;}parent=dentry->d_parent;
but that would slightly change the behaviour in another case.
Namely, nfs4 mount-time temporary namespaces. There we have
the following: mount -t nfs4 server:/foo/bar/baz /mnt
will
* set a temporary namespace, matching the mount tree as
exported by server
* mount the root export there
* traverse foo/bar/baz in that namespace, triggering
automounts when we cross the filesystem boundaries on server.
* grab whatever we'd arrived at; that's what we'll
be mounting.
* dissolve the temp namespace.
If you trigger some LSM hook (e.g. in permission checks on
that pathname traversal) for objects in that temp namespace,
do you want d_absolute_path() to succeed (and give a pathname
relative to server's root export), or should it rather fail?
AFAICS, apparmor and tomoyo are the only things that might
care either way; I would go with "fail, it's not an absolute
path" (and that's what the patch above will end up doing),
but it's really up to you.
It definitely ought to fail for yet-to-be-attached case, though;
it doesn't, and that's a bug that needs to be fixed. Mea culpa.
[bringing a private thread back to the lists]
There's a bug in interplay of fsmount() and d_absolute_path().
Namely, the check in d_absolute_path() treats the
not-yet-attached mount as "reached absolute root".