LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Aleksa Sarai <cyphar@cyphar.com>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	kernel test robot <rong.a.chen@intel.com>,
	Eric Biggers <ebiggers3@gmail.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, linux-api@vger.kernel.org,
	linux-kernel@vger.kernel.org, lkp@lists.01.org,
	ltp@lists.linux.it
Subject: Re: [exec] 166d03c9ec: ltp.execveat02.fail
Date: Thu, 4 Jun 2020 19:57:23 -0700
Message-ID: <202006041910.9EF0C602@keescook> (raw)
In-Reply-To: <20200525091420.GI12456@shao2-debian>

On Mon, May 25, 2020 at 05:14:20PM +0800, kernel test robot wrote:
> execveat02.c:64: PASS: execveat() fails as expected: EBADF (9)
> execveat02.c:64: PASS: execveat() fails as expected: EINVAL (22)
> execveat02.c:61: FAIL: execveat() fails unexpectedly, expected: ELOOP: EACCES (13)
> execveat02.c:64: PASS: execveat() fails as expected: ENOTDIR (20)

tl;dr: I think this test is correct, and I think I see a way to improve
the offending patch series to do the right thing.


Okay, the LTP is checking for ELOOP on trying to exec a symlink:

...
 *    3) execveat() fails and returns ELOOP if the file identified by dirfd and
 *       pathname is a symbolic link and flag includes AT_SYMLINK_NOFOLLOW.
...
#define TESTDIR "testdir"
#define TEST_APP "execveat_errno"
...
#define TEST_SYMLINK "execveat_symlink"
...
#define TEST_ERL_SYMLINK TESTDIR"/"TEST_SYMLINK
...
        sprintf(app_sym_path, "%s/%s", cur_dir_path, TEST_ERL_SYMLINK);
...
        SAFE_SYMLINK(TEST_REL_APP, TEST_ERL_SYMLINK);

        fd = SAFE_OPEN(TEST_REL_APP, O_PATH);
...
static struct tcase {
        int *fd;
        char *pathname;
        int flag;
        int exp_err;
} tcases[] = {
...
        {&fd, app_sym_path, AT_SYMLINK_NOFOLLOW, ELOOP},
...
};
...
                TEST(execveat(*tc->fd, tc->pathname, argv, environ, tc->flag));

This is testing the exec _of_ a symlink under AT_SYMLINK_NOFOLLOW.

The execve(2) manpage says:

       ELOOP  Too many symbolic links were encountered in resolving
              pathname or  the  name  of  a script or ELF interpreter.

       ELOOP  The maximum recursion limit was reached during recursive
	      script interpretation (see "Interpreter scripts", above).
	      Before Linux 3.8, the error produced for this case was ENOEXEC.

Which actually doesn't mention this case. open(2) says:

       ELOOP  Too many symbolic links were encountered in resolving pathname.

       ELOOP  pathname was a symbolic link, and flags specified O_NOFOLLOW
	      but not O_PATH.

(but O_NOFOLLOW is limited to file creation. linkat(2) lists the AT_*
flags, and applied to openat, this seems to track: attempting to
execat where the final element is a symlink should fail with ELOOP,
though the manpage does warn that this makes it indistinguishable from
symlink loops -- the first item listed in the execve manpage for
ELOOP...)

Regardless, this does seem to be the "correct" result, as opening for
exec or opening just normally should really get the same error code.

The call path for execve looks like this:

    do_open_execat()
        struct open_flags open_exec_flags = {
            .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
	    .acc_mode = MAY_READ | MAY_EXEC,
	    ...
        do_filp_open(dfd, filename, open_flags)
            path_openat(nameidata, open_flags, flags)
                file = alloc_empty_file(open_flags, current_cred());
		open_last_lookups(nd, file, open_flags)
		    step_into(nd, ...)
		        /* stop on symlink without LOOKUP_FOLLOW */
                do_open(nameidata, file, open_flags)
                    /* new location of FMODE_EXEC vs S_ISREG() test */
                    may_open(path, acc_mode, open_flag)
			/* test for S_IFLNK */
                        inode_permission(inode, MAY_OPEN | acc_mode)
                            security_inode_permission(inode, acc_mode)
                    vfs_open(path, file)
                        do_dentry_open(file, path->dentry->d_inode, open)
                            /* old location of FMODE_EXEC vs S_ISREG() test */
                            security_file_open(f)
                            open()

The step_into() is what kicks back out without LOOKUP_FOLLOW, so we're
left holding a symlink (S_IFMT inode). In do_open(), there is a set of
checks via may_open() which checks for S_IFMT and rejects it:

        switch (inode->i_mode & S_IFMT) {
        case S_IFLNK:
                return -ELOOP;

So that's the case LTP was testing for.

The patch in -next ("exec: relocate S_ISREG() check")[1], moves the regular
file requirement up before may_open(), for all the reasons mentioned in
the commit log (and the next patch[2]).

When I was originally trying to determine the best place for where the
checks should live, may_open() really did seem like the right place, but I
recognized that it was examining path characteristics (which was good) but
it didn't have the file, and that seemed to be an intentional separation.

What is needed in may_open() would be the "how was this file opened?"
piece of information: file->f_mode & FMODE_EXEC. However, in looking at
this again now, I wonder if it might be possible to use the MAY_EXEC
from the acc_mode? It seems the old check (in do_dentry_open() had no
access to the acc_mode, so it was forced to use the FMODE_EXEC signal
instead.

(I actually think this remains a bit of a design problem: path-based LSMs,
which see the opened file in security_file_open(), also don't have access
to acc_mode and must depend on the FMODE_* flags...)

I will respin the series to use acc_mode and move the tests into
may_open().

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=vfs/fmode_exec/v1&id=36aaecef88b472ad5e0a408ffdb5b2e46f1478f6
[2] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=vfs/fmode_exec/v1&id=a2f303b9f24ae2ecdc57f3db93e49b2b869893aa

-- 
Kees Cook

  parent reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-18  5:54 [PATCH 0/4] Relocate execve() sanity checks Kees Cook
2020-05-18  5:54 ` [PATCH 1/4] exec: Change uselib(2) IS_SREG() failure to EACCES Kees Cook
2020-05-18 13:02   ` Christian Brauner
2020-05-18 14:43     ` Jann Horn
2020-05-18 14:46       ` Christian Brauner
2020-05-18 23:57         ` Eric W. Biederman
2020-05-19  8:11           ` Christian Brauner
2020-05-19  8:37           ` Andreas Schwab
2020-05-19 11:56             ` Eric W. Biederman
2020-05-19 12:12               ` Andreas Schwab
2020-05-19 12:28                 ` Eric W. Biederman
2020-05-19 13:29                   ` Christian Brauner
2020-05-19 14:49                     ` Eric W. Biederman
2020-05-19 13:13               ` Christian Brauner
2020-05-19 14:32                 ` Geert Uytterhoeven
2020-05-19 14:47                   ` Christian Brauner
2020-05-18  5:54 ` [PATCH 2/4] exec: Relocate S_ISREG() check Kees Cook
     [not found]   ` <20200525091420.GI12456@shao2-debian>
2020-06-04 22:45     ` [exec] 166d03c9ec: ltp.execveat02.fail Kees Cook
2020-06-05  2:57     ` Kees Cook [this message]
2020-05-18  5:54 ` [PATCH 3/4] exec: Relocate path_noexec() check Kees Cook
2020-05-18  5:54 ` [PATCH 4/4] fs: Include FMODE_EXEC when converting flags to f_mode Kees Cook
2020-05-19 15:06 ` [PATCH 0/4] Relocate execve() sanity checks Eric W. Biederman
2020-05-19 16:26   ` Kees Cook
2020-05-19 17:41     ` Eric W. Biederman
2020-05-19 17:56       ` Kees Cook
2020-05-19 18:42         ` Eric W. Biederman
2020-05-19 21:17           ` Kees Cook
2020-05-19 22:58             ` John Johansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202006041910.9EF0C602@keescook \
    --to=keescook@chromium.org \
    --cc=akpm@linux-foundation.org \
    --cc=cyphar@cyphar.com \
    --cc=dvyukov@google.com \
    --cc=ebiggers3@gmail.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=lkp@lists.01.org \
    --cc=ltp@lists.linux.it \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=rong.a.chen@intel.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git