linux-kernel - Re: [PATCH 1/2] sysctl: expand use of proc_dointvec_minmax

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:	Fri, 22 Jan 2016 21:10:07 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Kees Cook <keescook@...omium.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Richard Weinberger <richard@....at>,
	Andy Lutomirski <luto@...capital.net>,
	Robert Święcki <robert@...ecki.net>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	David Howells <dhowells@...hat.com>,
	Miklos Szeredi <mszeredi@...e.cz>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 1/2] sysctl: expand use of proc_dointvec_minmax_sysadmin

Kees Cook <keescook@...omium.org> writes:

> Several sysctls expect a state where the highest value (in extra2) is
> locked once set for that boot. Yama does this, and kptr_restrict should
> be doing it. This extracts Yama's logic and adds it to the existing
> proc_dointvec_minmax_sysadmin, taking care to avoid the simple boolean
> states (which do not get locked). Since Yama wants to be checking a
> different capability, we build wrappers for both cases (CAP_SYS_ADMIN
> and CAP_SYS_PTRACE).

Sigh this sysctl appears susceptible to known attacks.

In my quick skim I believe this sysctl implementation that checks
capabilities is susceptible to attacks where the already open file
descriptor is set as stdout on a setuid root application.

Can we come up with an interface that isn't exploitable by an
application that will act as a setuid cat?

Eric

> -#ifdef CONFIG_PRINTK
> -static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
> -				void __user *buffer, size_t *lenp, loff_t *ppos)
> +int proc_dointvec_minmax_cap(int cap, struct ctl_table *table, int write,
> +			     void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	if (write && !capable(CAP_SYS_ADMIN))
> +	struct ctl_table table_copy;
> +	int value;
> +
> +	/* Require init capabilities to make changes. */
> +	if (write && !capable(cap))
>  		return -EPERM;
>  
> -	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +	/*
> +	 * To deal with const sysctl tables, we make a copy to perform
> +	 * the locking. When data is >1 and ==extra2, lock extra1 to
> +	 * extra2 to stop the value from being changed any further at
> +	 * runtime.
> +	 */
> +	table_copy = *table;
> +	value = *(int *)table_copy.data;
> +	if (value > 1 && value == *(int *)table_copy.extra2)
> +		table_copy.extra1 = table_copy.extra2;
> +
> +	return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>  }
> -#endif