linux-kernel - Re: [git pull] apparmor fix for __d

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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/