linux-kernel - Re: [PATCH] string: Fix strscpy() uninitialized data copy bug

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: <20151006072151.GA10672@gmail.com>
Date:	Tue, 6 Oct 2015 09:21:51 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Chris Metcalf <cmetcalf@...hip.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] string: Fix strscpy() uninitialized data copy bug


* Chris Metcalf <cmetcalf@...hip.com> wrote:

> Unfortunately using memset() like that will break on big-endian machines.

doh ... and I somehow convinced myself that it was endian safe ;-)

> [...]  I always have to go back and play around with the word-at-a-time.h 
> definitions to get this right, but I think it's possible that the "data" itself 
> has the mask to clear the unwanted bytes, i.e. you could do something like the 
> following (untested).
> 
> I'm still not totally convinced it's necessary, as programmers should generally 
> assume anything beyond the end of a copied string is garbage anyway, and since 
> we're not copying it to userspace we're not exposing any possibly secure data.
> 
> Races shouldn't be a concern either since, after all, there is already a window 
> where we may have overwritten the NUL end of an earlier shorter string, and now 
> a racy copy from the partially-written dest buf could walk right off the end of 
> the buffer itself, so you'd already better not be doing that.
> 
> But, all that said, I'm not opposed to a simple fix to avoid carrying along the 
> uninitialized bytes from beyond the end of the source string, since it does seem 
> a bit cleaner, even if I can't put my finger in a reason why it would actually 
> matter.

So it would matter for more advanced sharing ABIs: for example if there's an 
mlock()-ed area registered on the kernel side as well as kernel accessible memory, 
and if we do an strscpy() to such a target area, we don't want to leak 
uninitialized data to user-space.

(This is not theoretical, the perf ring-buffer is such a construct for example.)

So IMHO this is a quality of implementation issue that we should fix.

> diff --git a/lib/string.c b/lib/string.c
> index 8dbb7b1eab50..ba64f4e0382d 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -203,12 +203,13 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  		unsigned long c, data;
>  		c = *(unsigned long *)(src+res);
> -		*(unsigned long *)(dest+res) = c;
>  		if (has_zero(c, &data, &constants)) {
>  			data = prep_zero_mask(c, data, &constants);
>  			data = create_zero_mask(data);
> +			*(unsigned long *)(dest+res) = c & data;
>  			return res + find_zero(data);
>  		}
> +		*(unsigned long *)(dest+res) = c;
>  		res += sizeof(unsigned long);
>  		count -= sizeof(unsigned long);
>  		max -= sizeof(unsigned long);

Looks good to me!

Thanks,

	Ingo
--
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/