linux-kernel - [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: <20151005163641.GA11635@gmail.com>
Date:	Mon, 5 Oct 2015 18:36:41 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Alexey Dobriyan <adobriyan@...il.com>,
	Chris Metcalf <cmetcalf@...hip.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: [PATCH] string: Fix strscpy() uninitialized data copy bug


* Ingo Molnar <mingo@...nel.org> wrote:

> A slightly more paranoid version would be:
> 
> 		c = *(unsigned long *)(src+res);
>  
> 		if (has_zero(c, &data, &constants)) {
>  			unsigned int zero_pos;
>  
> 			data = prep_zero_mask(c, data, &constants); data = 
> 			create_zero_mask(data);
>  
>  			zero_pos = find_zero(data);
> 
> 			/* Clear out undefined data within the final word after 
> 			the NUL: */
>  			memset((void *)&c + zero_pos, 0, sizeof(long)-zero_pos);
> 
> 			*(unsigned long *)(dest+res) = c;
>  
> 			return res+zero_pos;
> 		}
> 		*(unsigned long *)(dest+res) = c;
> 
> This would solve any theoretical races in the _target_ buffer: if the target 
> buffer may be copied to user-space in a racy fashion and we don't ever want it 
> to have undefined data, then this variant does the tail-zeroing of the final 
> word in the temporary copy, not in the target buffer.
> 
> Still untested.

So the patch below got tested a bit more seriously, with the strscpy() based 
strlcpy() patch I sent earlier: at least a typical Fedora bootup with a few 
thousand strlcpy() uses does not crash in any obvious way.

Still needs review to make sure I have not missed anything ...

Thanks,

	Ingo

===================>
>From 946bab4d7138e5db53c5f1759e97809ebdf89551 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@...nel.org>
Date: Mon, 5 Oct 2015 18:30:37 +0200
Subject: [PATCH] string: Fix strscpy() uninitialized data copy bug

Alexey Dobriyan noticed that our new strscpy() implementation will copy 
potentially out of range or uninitialized data from post the end of the
source string.

Fix this by zeroing out the tail of the final word of the copy.

Reported-by: Alexey Dobriyan <adobriyan@...il.com>
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
 lib/string.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/string.c b/lib/string.c
index 6b89c035df74..548f52b7a145 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -177,12 +177,24 @@ 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)) {
+			unsigned int zero_pos;
+
 			data = prep_zero_mask(c, data, &constants);
 			data = create_zero_mask(data);
+
+			zero_pos = find_zero(data);
+
+			/* Clear out undefined data within the final word after the NUL (if any): */
+			memset((void *)&c + zero_pos, 0, sizeof(long)-zero_pos);
+
+			*(unsigned long *)(dest+res) = c;
+
 			return res + find_zero(data);
 		}
+		*(unsigned long *)(dest+res) = c;
+
 		res += sizeof(unsigned long);
 		count -= sizeof(unsigned long);
 		max -= sizeof(unsigned long);
--
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/