Re: SECURITY - data leakage due to incorrect strncpy implementation

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Martin Schwidefsky]

Hi Pete,
> I do not undestand Alan's position, if he is for it or against it.
> Anyway, in case you want it, here's what I wrote for s390.
> I wrote some userland tests, it seems to check out. BUT I warn you,
> someone better check my assembly.
Learning to write inline assembly? Nice, but it has one small
problem, the count in %r4 is not decremented for 0x00 byte.
Try my little patch.

blue skies,
  Martin.

diff -urN linux-2.6.1/arch/s390/lib/strncpy.S linux-2.6.1-s390/arch/s390/lib/strncpy.S
--- linux-2.6.1/arch/s390/lib/strncpy.S	Fri Jan  9 07:59:45 2004
+++ linux-2.6.1-s390/arch/s390/lib/strncpy.S	Thu Jan 29 09:53:02 2004
@@ -23,8 +23,13 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCT    4,strncpy_loop # R4 -= 1, jump to strncpy_loop if >  0
 strncpy_exit:
         BR      14
-
+strncpy_clear:
+	STC	0,0(1)
+	LA	1,1(1)
+strncpy_pad:
+	BRCT	4,strncpy_clear
+	BR	14
diff -urN linux-2.6.1/arch/s390/lib/strncpy64.S linux-2.6.1-s390/arch/s390/lib/strncpy64.S
--- linux-2.6.1/arch/s390/lib/strncpy64.S	Fri Jan  9 07:59:10 2004
+++ linux-2.6.1-s390/arch/s390/lib/strncpy64.S	Thu Jan 29 09:53:02 2004
@@ -23,8 +23,13 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCTG   4,strncpy_loop # R4 -= 1, jump to strncpy_loop if > 0
 strncpy_exit:
         BR      14
-
+strncpy_clear:
+	STC	0,0(1)
+	LA	1,1(1)
+strncpy_pad:
+	BRCTG	4,strncpy_clear
+	BR	14

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Pete Zaitcev]

On 11 Jul 2003 23:50:15 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Gwe, 2003-07-11 at 23:44, Linus Torvalds wrote:
> > On 11 Jul 2003, Alan Cox wrote:
> > > 
> > > Lots of kernel drivers rely on the libc definition of strncpy. 
> > 
> > But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
> > far as I know, the kernel strncpy() has done padding pretty much since day 
> > one.

>  * Note that unlike userspace strncpy, this does not %NUL-pad the buffer.
>  * However, the result is not %NUL-terminated if the source exceeds
>  * @count bytes.
>  */
> 
> Only x86 does the padding 

I do not undestand Alan's position, if he is for it or against it.
Anyway, in case you want it, here's what I wrote for s390.
I wrote some userland tests, it seems to check out. BUT I warn you,
someone better check my assembly.

-- Pete

diff -ur -X dontdiff linux-2.6.1/arch/s390/lib/strncpy64.S linux-2.6.1-s390/arch/s390/lib/strncpy64.S
--- linux-2.6.1/arch/s390/lib/strncpy64.S	2003-07-13 20:31:50.000000000 -0700
+++ linux-2.6.1-s390/arch/s390/lib/strncpy64.S	2004-01-28 18:48:27.000000000 -0800
@@ -23,8 +23,16 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCTG   4,strncpy_loop # R4 -= 1, jump to strncpy_loop if > 0
-strncpy_exit:
         BR      14
 
+strncpy_pad:
+	LTR     4,4
+        JZ      strncpy_exit   # 0 bytes -> nothing to do
+strncpy_padloop:
+	MVI	0(1),0
+	LA	1,1(1)
+	BRCTG	4,strncpy_padloop
+strncpy_exit:
+        BR      14
diff -ur -X dontdiff linux-2.6.1/arch/s390/lib/strncpy.S linux-2.6.1-s390/arch/s390/lib/strncpy.S
--- linux-2.6.1/arch/s390/lib/strncpy.S	2003-07-13 20:35:16.000000000 -0700
+++ linux-2.6.1-s390/arch/s390/lib/strncpy.S	2004-01-28 18:46:20.000000000 -0800
@@ -23,8 +23,16 @@
 	LA      3,1(3)
         STC     0,0(1)
 	LA      1,1(1)
-        JZ      strncpy_exit   # ICM inserted a 0x00
+        JZ      strncpy_pad    # ICM inserted a 0x00
         BRCT    4,strncpy_loop # R4 -= 1, jump to strncpy_loop if >  0
-strncpy_exit:
         BR      14
 
+strncpy_pad:
+	LTR	4,4
+	JZ	strncpy_exit
+strncpy_padloop:
+	MVI	0(1),0
+	LA	1,1(1)
+	BRCT	4,strncpy_padloop
+strncpy_exit:
+        BR      14

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Sad, 2003-07-12 at 22:28, Horst von Brand wrote:
> Perhaps there should be a strncpy_touser() to make it crystal clear that it
> _can't_ be "optimized" into strlcpy()

The direct to_user functions are not normally a problem. The data they fail
to clear is the users own data anyway.

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Horst von Brand]

Alan Cox <alan@lxorguk.ukuu.org.uk> said:

[...]

> 2.5 you have problems all over the place from wrong strlcpy conversions,
> but those are easy enough to clean up before 2.6.0

Perhaps there should be a strncpy_touser() to make it crystal clear that it
_can't_ be "optimized" into strlcpy()
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Paul Mackerras]

Alan Cox writes:

> Unknown: alpha, ppc

PPC doesn't clear the rest of the destination, I'll fix it.

Paul.

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Gwe, 2003-07-11 at 23:44, Linus Torvalds wrote:
> On 11 Jul 2003, Alan Cox wrote:
> > 
> > Lots of kernel drivers rely on the libc definition of strncpy. 
> 
> But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
> far as I know, the kernel strncpy() has done padding pretty much since day 
> one.

/**
 * strncpy - Copy a length-limited, %NUL-terminated string
 * @dest: Where to copy the string to
 * @src: Where to copy the string from
 * @count: The maximum number of bytes to copy
 *
 * Note that unlike userspace strncpy, this does not %NUL-pad the buffer.
 * However, the result is not %NUL-terminated if the source exceeds
 * @count bytes.
 */

Only x86 does the padding 

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Linus Torvalds]

On 11 Jul 2003, Alan Cox wrote:
> 
> Lots of kernel drivers rely on the libc definition of strncpy. 

But that's ok. We _do_ do the padding. I hated it when I wrote it, but as 
far as I know, the kernel strncpy() has done padding pretty much since day 
one.

Yes, strlcpy() conversion users need to be careful, but I think we mostly 
_were_ careful. Knock wood.

		Linus

Re: SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

Ok an update:

2.4 you have a problem if your port uses the lib/string.c
implementation. x86 does not and is ok (very nifty implementation of the
zeroing too)

That appears to be: 

Not vulnerable: x86 
Buggy generic: ARM, CRIS, IA-64, PA-RISC, S/390, SH64, SPARC, SPARC64,
X86-64
Buggy asm: m68k, mips (?), sh, 
Unknown: alpha, ppc

2.5 you have problems all over the place from wrong strlcpy conversions,
but those are easy enough to clean up before 2.6.0

SECURITY - data leakage due to incorrect strncpy implementation

[Alan Cox]

On Gwe, 2003-07-11 at 20:04, Mikulas Patocka wrote:
> What's the difference there? strlcpy always creates null-terminated
> string, strncpy doesn't. strncpy in kernel (unlike user strncpy) does not
> pad the whole destination buffer with zeros (see comment and
> implementation in lib/string.c), so I don't see any point why strncpy
> should be more secure.

Lots of kernel drivers rely on the libc definition of strncpy. 

Lets update the bug report to "2.4 and 2.5 both leak arbitary kernel data
to user space" tho thankfully in small pieces. Fix required. (bcc'd to Mark to 
assign a CAN number)

And for 2.4-ac I'm going to simply go make strncpy do what it says in the
book. For 2.5 the same is true and cleaner (since those who use strlcpy
properly don't take any performance hit). Actually it may make sense to 
backport strlcpy for those odd performance critical ones.

I don't think its that serious a bug - the odds of getting a critical bit of
someone elses data are remarkably low but it wants fixing.

Alan