linux-kernel - Re: [PATCH] binfmt_script: do not leave interp on stack

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]

Message-ID: <20121012185037.GJ24964@outflux.net>
Date:	Fri, 12 Oct 2012 11:50:37 -0700
From:	Kees Cook <keescook@...omium.org>
To:	halfdog <me@...fdog.net>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Randy Dunlap <rdunlap@...otime.net>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] binfmt_script: do not leave interp on stack

On Thu, Oct 11, 2012 at 07:32:40PM -0700, Kees Cook wrote:
> +	/*
> +	 * Since bprm is already modified, we cannot continue if the the
> +	 * handlers for starting the new interpreter have failed.
> +	 * Make sure that we do not return -ENOEXEC, as that would
> +	 * allow searching for handlers to continue.
> +	 */
> +	if (retval == -ENOEXEC)
> +		retval = -EINVAL;

After looking at this some more, I wonder if this should be -ELOOP
instead? Or maybe that should happen if/when the recursion depth problem is
fixed?

This is much more obvious, instead of "Invalid argument":
$ ./dotest.sh 
file-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAfile-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA: bad interpreter: Too many levels of symbolic links


More importantly, I also wonder if interp handling to just be
changed to be an allocation that needs to be cleaned up, as done with
argv? Right now interp just points to the filename argument handed to
do_execve. Especially since it looks like binfmt_misc is vulnerable
to this as well, since it runs the risk of getting -ENOEXEC from
search_binary_handler, leaving bprm->interp pointing into the stack,
only to get it recalled after module loading attempts:

static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
{
...
        char iname[BINPRM_BUF_SIZE];
...
        bprm->interp = iname;   /* for binfmt_script */
...
        retval = search_binary_handler (bprm, regs);
        if (retval < 0)
                goto _error;
...
_ret:
        return retval;
_error:
        if (fd_binary > 0)
                sys_close(fd_binary);
        bprm->interp_flags = 0;
        bprm->interp_data = 0;
        goto _ret;
}

:(

-Kees

-- 
Kees Cook
Chrome OS Security
--
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/