[PATCH] isdnloop: NUL-terminate strings from userspace

[PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

Both the in-kernel and BSD strlcpy() require that the source string is
NUL terminated. We could use strncpy() + explicitly terminate the result,
but this relies on src and dest having the same size, so the safest thing
to do seems to explicitly terminate the source string before doing the
strlcpy().

Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: stable@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 02125e6..50cd348 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;
 	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
 		return -EFAULT;
+
+	/*
+	 * Null terminate strings from userspace so we don't have to worry
+	 * about this later on.
+	 */
+	for (i = 0; i < 3; i++)
+		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
+
 	spin_lock_irqsave(&card->isdnloop_lock, flags);
 	switch (sdef.ptype) {
 	case ISDN_PTYPE_EURO:
-- 
1.7.10.4

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Hannes Frederic Sowa]

On Tue, Apr 01, 2014 at 12:08:18PM +0200, Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated. We could use strncpy() + explicitly terminate the result,
> but this relies on src and dest having the same size, so the safest thing
> to do seems to explicitly terminate the source string before doing the
> strlcpy().
> 
> Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> index 02125e6..50cd348 100644
> --- a/drivers/isdn/isdnloop/isdnloop.c
> +++ b/drivers/isdn/isdnloop/isdnloop.c
> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>  		return -EBUSY;
>  	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>  		return -EFAULT;
> +
> +	/*
> +	 * Null terminate strings from userspace so we don't have to worry
> +	 * about this later on.
> +	 */
> +	for (i = 0; i < 3; i++)
> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
> +

Looking down the problem, it seems the problem is that the strlen in strlcpy
could read beyond the input buffer?

To prevent this problem in other parts of the kernel wouldn't it be better to
replace the strlen with strnlen in strlcpy?

Bye,

  Hannes

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Vegard Nossum]

On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> On Tue, Apr 01, 2014 at 12:08:18PM +0200, Vegard Nossum wrote:
>> Both the in-kernel and BSD strlcpy() require that the source string is
>> NUL terminated. We could use strncpy() + explicitly terminate the result,
>> but this relies on src and dest having the same size, so the safest thing
>> to do seems to explicitly terminate the source string before doing the
>> strlcpy().
>>
>> Fixes: f9a23c84486ed35 ("isdnloop: use strlcpy() instead of strcpy()")
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>   drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
>> index 02125e6..50cd348 100644
>> --- a/drivers/isdn/isdnloop/isdnloop.c
>> +++ b/drivers/isdn/isdnloop/isdnloop.c
>> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>>   		return -EBUSY;
>>   	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>>   		return -EFAULT;
>> +
>> +	/*
>> +	 * Null terminate strings from userspace so we don't have to worry
>> +	 * about this later on.
>> +	 */
>> +	for (i = 0; i < 3; i++)
>> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
>> +
>
> Looking down the problem, it seems the problem is that the strlen in strlcpy
> could read beyond the input buffer?
>
> To prevent this problem in other parts of the kernel wouldn't it be better to
> replace the strlen with strnlen in strlcpy?

Sorry, I should have included the link to the previous thread: 
https://lkml.org/lkml/2014/3/7/712

I only resent (adding netdev to Cc) to get this into David Miller's 
patch queue.

As you can see from the previous discussion, we _could_ change the Linux 
kernel's definition of strlcpy(), but I wouldn't recommend it for the 
following reasons:

1. Both BSD man page and BSD implementation _require_ the source string 
to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
kernel would probably be a bad idea and cause even more confusion that 
what we already have.

2. Even if we changed strlcpy() to use strnlen(), it would still be 
unsafe if the source string is not 0-terminated and the source buffer is 
shorter than the destination buffer. That's because the size passed to 
strlcpy() is conceptually the length of the _destination_ buffer, not 
the source string.

I'm not against changing strlcpy() per se (changing to strnlen() might 
be a performance improvement), but we shouldn't use that as an excuse to 
use the interface incorrectly.


Vegard

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Hannes Frederic Sowa]

On Tue, Apr 01, 2014 at 12:46:37PM +0200, Vegard Nossum wrote:
> On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> >Looking down the problem, it seems the problem is that the strlen in 
> >strlcpy
> >could read beyond the input buffer?
> >
> >To prevent this problem in other parts of the kernel wouldn't it be better 
> >to
> >replace the strlen with strnlen in strlcpy?
> 
> Sorry, I should have included the link to the previous thread: 
> https://lkml.org/lkml/2014/3/7/712
> 
> I only resent (adding netdev to Cc) to get this into David Miller's 
> patch queue.

Ah ok, sorry I don't follow lkml as closely as netdev@.

> As you can see from the previous discussion, we _could_ change the Linux 
> kernel's definition of strlcpy(), but I wouldn't recommend it for the 
> following reasons:
> 
> 1. Both BSD man page and BSD implementation _require_ the source string 
> to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
> kernel would probably be a bad idea and cause even more confusion that 
> what we already have.

Sure, we shouldn't change the documented semantics. If at all it would
be an additional safety net. Your patch would still be needed.

> 2. Even if we changed strlcpy() to use strnlen(), it would still be 
> unsafe if the source string is not 0-terminated and the source buffer is 
> shorter than the destination buffer. That's because the size passed to 
> strlcpy() is conceptually the length of the _destination_ buffer, not 
> the source string.

Ack.

> I'm not against changing strlcpy() per se (changing to strnlen() might 
> be a performance improvement), but we shouldn't use that as an excuse to 
> use the interface incorrectly.

I am totally with you there.

Actually in some cases it could hinder finding such bugs as we're more
unlikely to hit a RED_ZONE which should crash the kernel (I actually
think crashes to find such bugs are good). But I guess the propability
is pretty high to hit another NUL byte before that and if at that point a
RED_ZONE is mapped.

Thanks,

  Hannes

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[Dan Carpenter]

On Tue, Apr 01, 2014 at 01:02:55PM +0200, Hannes Frederic Sowa wrote:
> On Tue, Apr 01, 2014 at 12:46:37PM +0200, Vegard Nossum wrote:
> > On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> > >Looking down the problem, it seems the problem is that the strlen in 
> > >strlcpy
> > >could read beyond the input buffer?
> > >
> > >To prevent this problem in other parts of the kernel wouldn't it be better 
> > >to
> > >replace the strlen with strnlen in strlcpy?
> > 
> > Sorry, I should have included the link to the previous thread: 
> > https://lkml.org/lkml/2014/3/7/712
> > 
> > I only resent (adding netdev to Cc) to get this into David Miller's 
> > patch queue.
> 
> Ah ok, sorry I don't follow lkml as closely as netdev@.
> 
> > As you can see from the previous discussion, we _could_ change the Linux 
> > kernel's definition of strlcpy(), but I wouldn't recommend it for the 
> > following reasons:
> > 
> > 1. Both BSD man page and BSD implementation _require_ the source string 
> > to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
> > kernel would probably be a bad idea and cause even more confusion that 
> > what we already have.
> 
> Sure, we shouldn't change the documented semantics. If at all it would
> be an additional safety net. Your patch would still be needed.
> 

Guys, really?  How would the patch "still be needed"?  I feel like if
someone said we had to rub a chicken head on this code we do it in the
name of security...

I don't understand what you think the point of strlcpy() is, if it's not
to deal with source strings which aren't NUL terminated.

I still maintain that the since the stack is full of NUL characters the
current implimentation of strlcpy() is ok for this isdn_loop function
and the patch is not needed at all without the strnlen() change.

However for other heap allocated variables then I could imagine that
the strlen() might be a problem.  I have two theories why we have never
seen problems with this in running code. 1) The string would have to be
at the end of a struct allocated at the end of a page.  You have to be
very unlucky to hit this requirement.  2) Most people pass valid data.

regards,
dan carpenter

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 1 Apr 2014 15:35:34 +0300

> I don't understand what you think the point of strlcpy() is, if it's not
> to deal with source strings which aren't NUL terminated.

If strlcpy() is meant to handle non-NULL terminated strings, then it's
kernel doc needs to be adjusted.

/**
 * strlcpy - Copy a %NUL terminated string into a sized buffer
 * @dest: Where to copy the string to
 * @src: Where to copy the string from
 * @size: size of destination buffer
 *
 * Compatible with *BSD: the result is always a valid
 * NUL-terminated string that fits in the buffer (unless,
 * of course, the buffer size is zero). It does not pad
 * out the result like strncpy() does.
 */

That says to me that 'src' is expected to be NULL terminated.

Furthermore, I like YOSHIFUJI Hideaki's idea that we should
actually validate the string and return -EINVAL if it is not
given to us NULL terminated.

[PATCH] isdnloop: Validate NUL-terminated strings from user.

[YOSHIFUJI Hideaki]

Return -EINVAL unless all of user-given strings are correctly
NUL-terminated.

Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
 drivers/isdn/isdnloop/isdnloop.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 02125e6..e1f8748 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -1070,6 +1070,12 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;
 	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
 		return -EFAULT;
+
+	for (i = 0; i < 3; i++) {
+		if (!memchr(sdef.num[i], 0, sizeof(sdef.num[i])))
+			return -EINVAL;
+	}
+
 	spin_lock_irqsave(&card->isdnloop_lock, flags);
 	switch (sdef.ptype) {
 	case ISDN_PTYPE_EURO:
-- 
1.7.9.5

Re: [PATCH] isdnloop: Validate NUL-terminated strings from user.

[David Miller]

From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Date: Wed, 02 Apr 2014 12:48:42 +0900

> Return -EINVAL unless all of user-given strings are correctly
> NUL-terminated.
> 
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

Applied and queud up for -stable, thanks!

[patch 1/2] lib/string.c: use the name "C-string" in comments

[Dan Carpenter]

For strncpy() and friends the source string may or may not have an
actual NUL character at the end.  The documentation is confusing in this
because it specifically mentions that you are passing a "NUL-terminated"
string.  Wikipedia says that "C-string" is an alternative name we can
use instead.

http://en.wikipedia.org/wiki/Null-terminated_string

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/lib/string.c b/lib/string.c
index 9b1f906..89ad0f0 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -107,7 +107,7 @@ EXPORT_SYMBOL(strcpy);
 
 #ifndef __HAVE_ARCH_STRNCPY
 /**
- * strncpy - Copy a length-limited, %NUL-terminated string
+ * strncpy - Copy a length-limited, C-string
  * @dest: Where to copy the string to
  * @src: Where to copy the string from
  * @count: The maximum number of bytes to copy
@@ -136,7 +136,7 @@ EXPORT_SYMBOL(strncpy);
 
 #ifndef __HAVE_ARCH_STRLCPY
 /**
- * strlcpy - Copy a %NUL terminated string into a sized buffer
+ * strlcpy - Copy a C-string into a sized buffer
  * @dest: Where to copy the string to
  * @src: Where to copy the string from
  * @size: size of destination buffer
@@ -182,7 +182,7 @@ EXPORT_SYMBOL(strcat);
 
 #ifndef __HAVE_ARCH_STRNCAT
 /**
- * strncat - Append a length-limited, %NUL-terminated string to another
+ * strncat - Append a length-limited, C-string to another
  * @dest: The string to be appended to
  * @src: The string to append to it
  * @count: The maximum numbers of bytes to copy
@@ -211,7 +211,7 @@ EXPORT_SYMBOL(strncat);
 
 #ifndef __HAVE_ARCH_STRLCAT
 /**
- * strlcat - Append a length-limited, %NUL-terminated string to another
+ * strlcat - Append a length-limited, C-string to another
  * @dest: The string to be appended to
  * @src: The string to append to it
  * @count: The size of the destination buffer.

[patch 2/2] lib/string.c: strlcpy() might read too far

[Dan Carpenter]

Imagine you have a user controlled variable at the end of a struct which
is allocated at the end of a page.  The strlen() could read beyond the
mapped memory and cause an oops.

Probably there are two reasons why we have never hit this condition in
real life.  First you would have to be really unlucky for all the
variables to line up so the oops can happen.  Second we don't do a lot
of fuzzing with invalid strings.

The strnlen() call is obviously a little bit slower than strlen() but I
have tested it and I think it's probably ok.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/lib/string.c b/lib/string.c
index 9b1f906..8074962 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -148,10 +148,10 @@ EXPORT_SYMBOL(strncpy);
  */
 size_t strlcpy(char *dest, const char *src, size_t size)
 {
-	size_t ret = strlen(src);
+	size_t ret = strnlen(src, size);
 
 	if (size) {
-		size_t len = (ret >= size) ? size - 1 : ret;
+		size_t len = (ret < size) ? ret : ret - 1;
 		memcpy(dest, src, len);
 		dest[len] = '\0';
 	}

Re: [patch 2/2] lib/string.c: strlcpy() might read too far

[Andrew Morton]

On Wed, 2 Apr 2014 11:47:31 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> Imagine you have a user controlled variable at the end of a struct which
> is allocated at the end of a page.  The strlen() could read beyond the
> mapped memory and cause an oops.

Well, it's hard to conceive of a situation where anything like this
could happen.  Code which copies a string from userspace should
immediately ensure that the kernel copy is null-terminated.  But...  I
guess it's defense in depth.

> Probably there are two reasons why we have never hit this condition in
> real life.  First you would have to be really unlucky for all the
> variables to line up so the oops can happen.  Second we don't do a lot
> of fuzzing with invalid strings.
> 
> The strnlen() call is obviously a little bit slower than strlen() but I
> have tested it and I think it's probably ok.

If we cared about speed we wouldn't take two passes across the input
string ;)

Re: [patch 2/2] lib/string.c: strlcpy() might read too far

[Dan Carpenter]

On Mon, Apr 14, 2014 at 02:46:15PM -0700, Andrew Morton wrote:
> On Wed, 2 Apr 2014 11:47:31 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Imagine you have a user controlled variable at the end of a struct which
> > is allocated at the end of a page.  The strlen() could read beyond the
> > mapped memory and cause an oops.
> 
> Well, it's hard to conceive of a situation where anything like this
> could happen.  Code which copies a string from userspace should
> immediately ensure that the kernel copy is null-terminated.  But...  I
> guess it's defense in depth.


The code which prompted this is in isdnloop_start().  I did a:

-	strcpy(card->s0num[0], sdef.num[0]);
+	strlcpy(card->s0num[0], sdef.num[0], sizeof(card->s0num[0]));

So that we wouldn't corrupt memory.  But the debate was whether that was
enough to solve the problem or it was better to null-terminate
sdef.num[0] before doing the strlcpy().  In the end, we decided to
reject input if it wasn't null terminated.

In this case sdef is declared on the stack so there lots NUL characters
so reading too far in the strlen() is not a problem.

regards,
dan carpenter

Re: [patch 2/2] lib/string.c: strlcpy() might read too far

[Dave Jones]

On Wed, Apr 02, 2014 at 11:47:31AM +0300, Dan Carpenter wrote:
 > Imagine you have a user controlled variable at the end of a struct which
 > is allocated at the end of a page.  The strlen() could read beyond the
 > mapped memory and cause an oops.
 > 
 > Probably there are two reasons why we have never hit this condition in
 > real life.  First you would have to be really unlucky for all the
 > variables to line up so the oops can happen.  Second we don't do a lot
 > of fuzzing with invalid strings.

The latter isn't necessarily true, trinity does pass all kinds of
garbage, including malformed ascii of various lengths. But what it
doesn't do fully is pass pointers to this junk in every struct we have
in the kernel. (Just the ones it knows about, which for now is mostly
things like sockaddr_t).

Do you have an example struct with layout like you describe ?
It probably wouldn't be much work to teach the fuzzer about it.
(The tricky part is getting such a malformed struct past the validation
 various syscalls do).

	Dave

Re: [PATCH] isdnloop: NUL-terminate strings from userspace

[YOSHIFUJI Hideaki]

Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated. We could use strncpy() + explicitly terminate the result,
> but this relies on src and dest having the same size, so the safest thing
> to do seems to explicitly terminate the source string before doing the
> strlcpy().
:
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> index 02125e6..50cd348 100644
> --- a/drivers/isdn/isdnloop/isdnloop.c
> +++ b/drivers/isdn/isdnloop/isdnloop.c
> @@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
>  		return -EBUSY;
>  	if (copy_from_user((char *) &sdef, (char *) sdefp, sizeof(sdef)))
>  		return -EFAULT;
> +
> +	/*
> +	 * Null terminate strings from userspace so we don't have to worry
> +	 * about this later on.
> +	 */
> +	for (i = 0; i < 3; i++)
> +		sdef.num[i][sizeof(sdef.num[0]) - 1] = '\0';
> +

Why don't we return -EINVAL if it is not correctly terminated by NUL?

--yoshfuji