| 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: <548D4B75.8060808@internode.on.net>
Date: Sun, 14 Dec 2014 19:03:57 +1030
From: Arthur Marsh <arthur.marsh@...ernode.on.net>
To: Al Viro <viro@...IV.linux.org.uk>
CC: Colin Watson <cjwatson@...ian.org>, 772807@...s.debian.org,
linux-kernel@...r.kernel.org, vapier@...too.org,
Joe Perches <joe@...ches.com>
Subject: Re: Bug#772807: binfmt-support: unable to close /proc/sys/fs/binfmt_misc/register:
Invalid argument
Al Viro wrote on 12/12/14 16:31:
> On Fri, Dec 12, 2014 at 02:51:55PM +1030, Arthur Marsh wrote:
>> 6b899c4e9a049dfca759d990bd53b14f81c3626c is the first bad commit
>> commit 6b899c4e9a049dfca759d990bd53b14f81c3626c
>> Author: Mike Frysinger <vapier@...too.org>
>> Date: Wed Dec 10 15:52:08 2014 -0800
>>
>> binfmt_misc: add comments & debug logs
>>
>> When trying to develop a custom format handler, the errors returned all
>> effectively get bucketed as EINVAL with no kernel messages. The other
>> errors (ENOMEM/EFAULT) are internal/obvious and basic. Thus any time a
>> bad handler is rejected, the developer has to walk the dense code and
>> try to guess where it went wrong. Needing to dive into kernel code is
>> itself a fairly high barrier for a lot of people.
>>
>> To improve this situation, let's deploy extensive pr_debug markers at
>> logical parse points, and add comments to the dense parsing logic. It
>> let's you see exactly where the parsing aborts, the string the kernel
>> received (useful when dealing with shell code), how it translated the
>> buffers to binary data, and how it will apply the mask at runtime.
>
> ... and looking at it shows the following garbage:
> p[-1] = '\0';
> - if (!e->magic[0])
> + if (p == e->magic)
>
> That code makes no sense whatsoever - if p *was* equal to e->magic, the
> assignment before it would be obviously broken. And a few lines later we
> have another chunk just like that.
>
> Moreover, if you look at further context, you'll see that it's
> e->magic = p;
> p = scanarg(p, del);
> if (!p)
> sod off
> p[-1] = '\0';
> if (p == e->magic)
> sod off
> Now, that condition could be true only if scanarg(p, del) would return p,
> right? Let's take a look at scanarg():
> static char *scanarg(char *s, char del)
> {
> char c;
>
> while ((c = *s++) != del) {
> if (c == '\\' && *s == 'x') {
> s++;
> if (!isxdigit(*s++))
> return NULL;
> if (!isxdigit(*s++))
> return NULL;
> }
> }
> return s;
> }
>
> See that s++ in the loop condition? There's no way in hell it would *not*
> increment s. If we return non-NULL, we know that c was equal to del *and*
> c is equal to previous_value_of_s[0], i.e. s[-1]. IOW, return value points
> to the byte following the first instance of delimeter starting at the argument.
>
> And p[-1] = '\0' replaces delimiter with NUL. IOW, the checks used to be
> correct. And got buggered.
>
> Subject: unfuck fs/binfmt_misc.c
>
> Undo some of the "prettifying" braindamage.
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> ---
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 70789e1..a6b6697 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -5,6 +5,8 @@
> *
> * binfmt_misc detects binaries via a magic or filename extension and invokes
> * a specified wrapper. See Documentation/binfmt_misc.txt for more details.
> + *
> + * Cetero censeo, checkpatch.pl esse delendam
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -131,9 +133,8 @@ static int load_misc_binary(struct linux_binprm *bprm)
> int retval;
> int fd_binary = -1;
>
> - retval = -ENOEXEC;
> if (!enabled)
> - goto ret;
> + return -ENOEXEC;
>
> /* to keep locking time low, we copy the interpreter string */
> read_lock(&entries_lock);
> @@ -142,12 +143,12 @@ static int load_misc_binary(struct linux_binprm *bprm)
> strlcpy(iname, fmt->interpreter, BINPRM_BUF_SIZE);
> read_unlock(&entries_lock);
> if (!fmt)
> - goto ret;
> + return -ENOEXEC;
>
> if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) {
> retval = remove_arg_zero(bprm);
> if (retval)
> - goto ret;
> + return retval;
> }
>
> if (fmt->flags & MISC_FMT_OPEN_BINARY) {
> @@ -157,10 +158,9 @@ static int load_misc_binary(struct linux_binprm *bprm)
> * to it
> */
> fd_binary = get_unused_fd_flags(0);
> - if (fd_binary < 0) {
> - retval = fd_binary;
> - goto ret;
> - }
> + if (fd_binary < 0)
> + return fd_binary;
> +
> fd_install(fd_binary, bprm->file);
>
> /* if the binary is not readable than enforce mm->dumpable=0
> @@ -219,14 +219,13 @@ static int load_misc_binary(struct linux_binprm *bprm)
> 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;
> + return retval;
> }
>
> /* Command parsers */
> @@ -250,6 +249,7 @@ static char *scanarg(char *s, char del)
> return NULL;
> }
> }
> + s[-1] = '\0';
> return s;
> }
>
> @@ -316,7 +316,7 @@ static Node *create_entry(const char __user *buffer, size_t count)
>
> memset(e, 0, sizeof(Node));
> if (copy_from_user(buf, buffer, count))
> - goto efault;
> + goto Efault;
>
> del = *p++; /* delimeter */
>
> @@ -329,13 +329,13 @@ static Node *create_entry(const char __user *buffer, size_t count)
> e->name = p;
> p = strchr(p, del);
> if (!p)
> - goto einval;
> + goto Einval;
> *p++ = '\0';
> if (!e->name[0] ||
> !strcmp(e->name, ".") ||
> !strcmp(e->name, "..") ||
> strchr(e->name, '/'))
> - goto einval;
> + goto Einval;
>
> pr_debug("register: name: {%s}\n", e->name);
>
> @@ -350,10 +350,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
> e->flags = (1 << Enabled) | (1 << Magic);
> break;
> default:
> - goto einval;
> + goto Einval;
> }
> if (*p++ != del)
> - goto einval;
> + goto Einval;
>
> if (test_bit(Magic, &e->flags)) {
> /* Handle the 'M' (magic) format. */
> @@ -362,21 +362,20 @@ static Node *create_entry(const char __user *buffer, size_t count)
> /* Parse the 'offset' field. */
> s = strchr(p, del);
> if (!s)
> - goto einval;
> + goto Einval;
> *s++ = '\0';
> e->offset = simple_strtoul(p, &p, 10);
> if (*p++)
> - goto einval;
> + goto Einval;
> pr_debug("register: offset: %#x\n", e->offset);
>
> /* Parse the 'magic' field. */
> e->magic = p;
> p = scanarg(p, del);
> if (!p)
> - goto einval;
> - p[-1] = '\0';
> - if (p == e->magic)
> - goto einval;
> + goto Einval;
> + if (!e->magic[0])
> + goto Einval;
> if (USE_DEBUG)
> print_hex_dump_bytes(
> KBUILD_MODNAME ": register: magic[raw]: ",
> @@ -386,9 +385,8 @@ static Node *create_entry(const char __user *buffer, size_t count)
> e->mask = p;
> p = scanarg(p, del);
> if (!p)
> - goto einval;
> - p[-1] = '\0';
> - if (p == e->mask) {
> + goto Einval;
> + if (!e->mask[0]) {
> e->mask = NULL;
> pr_debug("register: mask[raw]: none\n");
> } else if (USE_DEBUG)
> @@ -405,9 +403,9 @@ static Node *create_entry(const char __user *buffer, size_t count)
> e->size = string_unescape_inplace(e->magic, UNESCAPE_HEX);
> if (e->mask &&
> string_unescape_inplace(e->mask, UNESCAPE_HEX) != e->size)
> - goto einval;
> + goto Einval;
> if (e->size + e->offset > BINPRM_BUF_SIZE)
> - goto einval;
> + goto Einval;
> pr_debug("register: magic/mask length: %i\n", e->size);
> if (USE_DEBUG) {
> print_hex_dump_bytes(
> @@ -439,23 +437,23 @@ static Node *create_entry(const char __user *buffer, size_t count)
> /* Skip the 'offset' field. */
> p = strchr(p, del);
> if (!p)
> - goto einval;
> + goto Einval;
> *p++ = '\0';
>
> /* Parse the 'magic' field. */
> e->magic = p;
> p = strchr(p, del);
> if (!p)
> - goto einval;
> + goto Einval;
> *p++ = '\0';
> if (!e->magic[0] || strchr(e->magic, '/'))
> - goto einval;
> + goto Einval;
> pr_debug("register: extension: {%s}\n", e->magic);
>
> /* Skip the 'mask' field. */
> p = strchr(p, del);
> if (!p)
> - goto einval;
> + goto Einval;
> *p++ = '\0';
> }
>
> @@ -463,10 +461,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
> e->interpreter = p;
> p = strchr(p, del);
> if (!p)
> - goto einval;
> + goto Einval;
> *p++ = '\0';
> if (!e->interpreter[0])
> - goto einval;
> + goto Einval;
> pr_debug("register: interpreter: {%s}\n", e->interpreter);
>
> /* Parse the 'flags' field. */
> @@ -474,17 +472,17 @@ static Node *create_entry(const char __user *buffer, size_t count)
> if (*p == '\n')
> p++;
> if (p != buf + count)
> - goto einval;
> + goto Einval;
>
> return e;
>
> out:
> return ERR_PTR(err);
>
> -efault:
> +Efault:
> kfree(e);
> return ERR_PTR(-EFAULT);
> -einval:
> +Einval:
> kfree(e);
> return ERR_PTR(-EINVAL);
> }
>
I had to revert Al Viro's "Undo some of the "prettifying" braindamage."
patch above to apply the execveat() commit below that is now in Linus'
git head:
https://github.com/torvalds/linux/commit/51f39a1f0cea1cacf8c787f652f26dfee9611874
After that, trying to apply Al Viro's patch resulted in on failed hunk
and code that wouldn't build.
binfmt_misc.c building but not working doesn't break things for me, but
it would be good to get fixed.
Regards,
Arthur.
--
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