| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201112070859.pB78xn7x007845@www262.sakura.ne.jp>
Date: Wed, 07 Dec 2011 17:59:49 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: viro@...IV.linux.org.uk
Cc: john.johansen@...onical.com, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, torvalds@...ux-foundation.org
Subject: Re: [git pull] apparmor fix for __d_path() misuse
Al Viro wrote:
> BTW, what your current code does if you have a file bound on another
> file, open it, umount -l it, let the dust settle and then do some operation
> that triggers tomoyo_get_absolute_path() on it? Because you'll be getting
> a vfsmount/dentry pair that has
> * dentry == vfsmount->mnt_root
> * vfsmount->mnt_parent == vfsmount
> * dentry->d_inode being a non-directory
> and there is nothing whatsoever in what remains of the pathname. Not a single
> component. IOW, you'll get "/" in buf. Might be good in a testsuite - is
> there any code in security/tomoyo that would be relying on assumption that
> only directory might have a name entirely without components?
TOMOYO assumes that only directory ends with '/'.
I wrote a test program and compared among below cases.
current code
current code + your patch
current code + your patch + my patch (attached at the bottom)
---------- test1.c ----------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#define MNT_DETACH 2
static void die(const char *msg)
{
fprintf(stderr, "%s\n", msg);
exit(1);
}
int main(int argc, char *argv[])
{
int fd;
if (mount("/tmp/", "/tmp", "tmpfs", 0, NULL))
die("Can't mount");
fd = open("/tmp/file1", O_WRONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't create /tmp/file1");
close(fd);
fd = open("/tmp/file2", O_WRONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't create /tmp/file2");
close(fd);
if (mount("/tmp/file1", "/tmp/file2", NULL, MS_BIND, 0))
die("Can't bind mount");
fd = open("/tmp/file2", O_RDONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't open /tmp/file2");
if (umount2("/tmp/file2", MNT_DETACH))
die("Can't unmount 2");
ioctl(fd, 0);
close(fd);
return 0;
}
---------- test1.c ----------
Below are the policy checked by TOMOYO.
---- linux b835c0f4 ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file ioctl / 0x0
3: file mount /tmp/ /tmp/ tmpfs 0x0
4: file mount /tmp/file1 /tmp/file2 --bind 0x0
5: file read /tmp/file2
6: file unmount /tmp/file2
7: file write /tmp/file1
8: file write /tmp/file2
ioctl() handled but its pathname is wrong.
---- linux b835c0f4 with your patch ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file mount /tmp/ /tmp/ tmpfs 0x0
3: file mount /tmp/file1 /tmp/file2 --bind 0x0
4: file read /tmp/file2
5: file unmount /tmp/file2
6: file write /tmp/file1
7: file write /tmp/file2
ioctl() failed (i.e. regression) because tomoyo_get_absolute_path() failed.
The error message was
ERROR: Out of memory at tomoyo_realpath_from_path.
---- linux b835c0f4 with your patch and my patch ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file ioctl tmpfs:/file1 0x0
3: file mount /tmp/ /tmp/ tmpfs 0x0
4: file mount /tmp/file1 /tmp/file2 --bind 0x0
5: file read /tmp/file2
6: file unmount /tmp/file2
7: file write /tmp/file1
8: file write /tmp/file2
ioctl() handled using pathname returned by tomoyo_get_local_path().
> The real question is what pathname do you _want_ in this situation. Define
> that and we'll be able to do something about it;
Among above three results, the last one will be the best.
[PATCH] TOMOYO: Fix pathname handling of disconnected paths.
Current tomoyo_realpath_from_path() implementation returns strange pathname
when calculating pathname of a file which belongs to lazy unmounted tree.
Use local pathname rather than strange absolute pathname in that case.
Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -293,8 +293,16 @@ char *tomoyo_realpath_from_path(struct path *path)
pos = tomoyo_get_local_path(path->dentry, buf,
buf_len - 1);
/* Get absolute name for the rest. */
- else
+ else {
pos = tomoyo_get_absolute_path(path, buf, buf_len - 1);
+ /*
+ * Fall back to local name if absolute name is not
+ * available.
+ */
+ if (pos == ERR_PTR(-EINVAL))
+ pos = tomoyo_get_local_path(path->dentry, buf,
+ buf_len - 1);
+ }
encode:
if (IS_ERR(pos))
continue;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists