linux-kernel - Re: [PATCH] kernel: sysctl: ignore invalid taint bits introduced via kernel.tainted and taint the kernel with TAINT

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: <20200511231045.GV11244@42.do-not-panic.com>
Date:   Mon, 11 May 2020 23:10:45 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Rafael Aquini <aquini@...hat.com>, Tso Ted <tytso@....edu>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        keescook@...omium.org, yzaikin@...gle.com
Subject: Re: [PATCH] kernel: sysctl: ignore invalid taint bits introduced via
 kernel.tainted and taint the kernel with TAINT_USER on writes

On Mon, May 11, 2020 at 05:59:04PM -0400, Rafael Aquini wrote:
> The sysctl knob allows any user with SYS_ADMIN capability to
> taint the kernel with any arbitrary value, but this might
> produce an invalid flags bitset being committed to tainted_mask.
> 
> This patch introduces a simple way for proc_taint() to ignore
> any eventual invalid bit coming from the user input before
> committing those bits to the kernel tainted_mask, as well as
> it makes clear use of TAINT_USER flag to mark the kernel
> tainted by user everytime a taint value is written
> to the kernel.tainted sysctl.
> 
> Signed-off-by: Rafael Aquini <aquini@...hat.com>
> ---
>  kernel/sysctl.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8a176d8727a3..f0a4fb38ac62 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2623,17 +2623,32 @@ static int proc_taint(struct ctl_table *table, int write,
>  		return err;
>  
>  	if (write) {
> +		int i;
> +
> +		/*
> +		 * Ignore user input that would make us committing
> +		 * arbitrary invalid TAINT flags in the loop below.
> +		 */
> +		tmptaint &= (1UL << TAINT_FLAGS_COUNT) - 1;

This looks good but we don't pr_warn() of information lost on intention.

> +
>  		/*
>  		 * Poor man's atomic or. Not worth adding a primitive
>  		 * to everyone's atomic.h for this
>  		 */
> -		int i;
>  		for (i = 0; i < BITS_PER_LONG && tmptaint >> i; i++) {
>  			if ((tmptaint >> i) & 1)
>  				add_taint(i, LOCKDEP_STILL_OK);
>  		}
> +
> +		/*
> +		 * Users with SYS_ADMIN capability can include any arbitrary
> +		 * taint flag by writing to this interface. If that's the case,
> +		 * we also need to mark the kernel "tainted by user".
> +		 */
> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);

I'm in favor of this however I'd like to hear from Ted on if it meets
the original intention. I would think he had a good reason not to add
it here.

   Luis

>  	}
>  
> +
>  	return err;
>  }
>  
> -- 
> 2.25.4
>