[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

[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

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

[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] 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!

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

[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 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] isdnloop: NUL-terminate strings from userspace

[David Miller]

From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Mon, 31 Mar 2014 14:56:07 +0200

> Ping, Dave? Just making sure this doesn't fall through the cracks. I
> don't see the patch applied anywhere yet and without this patch we
> still have a valid security concern IMO.

If it's not sent to netdev, it's not in my queue.

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

[Vegard Nossum]

On 31 March 2014 15:36, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Mar 31, 2014 at 02:56:07PM +0200, Vegard Nossum wrote:
>> Ping, Dave? Just making sure this doesn't fall through the cracks. I
>> don't see the patch applied anywhere yet and without this patch we
>> still have a valid security concern IMO.
>
> Gar.  No...  To recap:
>
>> On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> > Both the in-kernel and BSD strlcpy() require that the source string is
>> > NUL terminated.
>
> The *whole point* of strlcpy() is that the source string doesn't have to
> be NUL terminated.  The BSD man pages are trying to say that strlcpy()
> only works on C-strings as opposed to Vstr or other safer string
> implementions.

I read the BSD man page differently. Also, if you look at the actual
BSD implementation, it also scans the remaining buffer until it hits a
0.

I quote again: "for strlcpy() src must be NUL-terminated". It doesn't
get much clearer than that.

> There is a potential problem in the kernel implementation of strlcpy()
> because it does:
>
> lib/string.c
>    149  size_t strlcpy(char *dest, const char *src, size_t size)
>    150  {
>    151          size_t ret = strlen(src);
>    152
>    153          if (size) {
>    154                  size_t len = (ret >= size) ? size - 1 : ret;
>    155                  memcpy(dest, src, len);
>    156                  dest[len] = '\0';
>    157          }
>    158          return ret;
>    159  }
>
> The strlen() on line 151 could read beyond the end of the source buffer
> and if the memory wasn't mapped, it could Oops.
>
> That concern doesn't apply here because the source string is on stack
> memory and we will hit a NUL character before we hit unmapped memory.

As before, I agree that it's _likely_ we'll hit a 0 before hitting
unmapped memory, but I don't see at all that we have a guarantee of
it.


Vegard

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

[Dan Carpenter]

On Mon, Mar 31, 2014 at 02:56:07PM +0200, Vegard Nossum wrote:
> Ping, Dave? Just making sure this doesn't fall through the cracks. I
> don't see the patch applied anywhere yet and without this patch we
> still have a valid security concern IMO.

Gar.  No...  To recap:

> On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> > Both the in-kernel and BSD strlcpy() require that the source string is
> > NUL terminated.

The *whole point* of strlcpy() is that the source string doesn't have to
be NUL terminated.  The BSD man pages are trying to say that strlcpy()
only works on C-strings as opposed to Vstr or other safer string
implementions.

There is a potential problem in the kernel implementation of strlcpy()
because it does:

lib/string.c
   149  size_t strlcpy(char *dest, const char *src, size_t size)
   150  {
   151          size_t ret = strlen(src);
   152
   153          if (size) {
   154                  size_t len = (ret >= size) ? size - 1 : ret;
   155                  memcpy(dest, src, len);
   156                  dest[len] = '\0';
   157          }
   158          return ret;
   159  }

The strlen() on line 151 could read beyond the end of the source buffer
and if the memory wasn't mapped, it could Oops.

That concern doesn't apply here because the source string is on stack
memory and we will hit a NUL character before we hit unmapped memory.

regards,
dan carpenter

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

[Vegard Nossum]

On 7 March 2014 11:56, Vegard Nossum <vegard.nossum@oracle.com> 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';
> +
>         spin_lock_irqsave(&card->isdnloop_lock, flags);
>         switch (sdef.ptype) {
>         case ISDN_PTYPE_EURO:

Ping, Dave? Just making sure this doesn't fall through the cracks. I
don't see the patch applied anywhere yet and without this patch we
still have a valid security concern IMO.


Vegard

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

[Dan Carpenter]

On Fri, Mar 07, 2014 at 01:44:17PM +0100, Vegard Nossum wrote:
> On 03/07/2014 12:52 PM, Dan Carpenter wrote:
> >On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
> >>On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> >>>On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> >>>>Both the in-kernel and BSD strlcpy() require that the source string is
> >>>>NUL terminated.
> >>>
> >>>No.  You're obviously wrong.  What on earth?
> >>
> >>Well, from lib/string.c:
> >>
> >>size_t strlcpy(char *dest, const char *src, size_t size)
> >>{
> >>         size_t ret = strlen(src);
> >>
> >
> >Ah...  So you mean that we could read far beyond the end of the string
> >and it would be a DoS because there would be 4 gigs of memory before we
> >hit a NUL character.  That won't happen in this case because the user
> >only controls a small buffer.  Normal memory is full of NUL chars.
> 
> Well, that's true, but what happens if you accidentally read from an
> unmapped page or an mmio page?

Interesting idea, but "sdef" is on the stack so that doesn't apply here
even though it might apply to other strlcpy() callers.

It might apply to other places.

regards,
dan carpenter

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

[Vegard Nossum]

On 03/07/2014 12:52 PM, Dan Carpenter wrote:
> On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
>> On 03/07/2014 12:26 PM, Dan Carpenter wrote:
>>> On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
>>>> Both the in-kernel and BSD strlcpy() require that the source string is
>>>> NUL terminated.
>>>
>>> No.  You're obviously wrong.  What on earth?
>>
>> Well, from lib/string.c:
>>
>> size_t strlcpy(char *dest, const char *src, size_t size)
>> {
>>          size_t ret = strlen(src);
>>
>
> Ah...  So you mean that we could read far beyond the end of the string
> and it would be a DoS because there would be 4 gigs of memory before we
> hit a NUL character.  That won't happen in this case because the user
> only controls a small buffer.  Normal memory is full of NUL chars.

Well, that's true, but what happens if you accidentally read from an 
unmapped page or an mmio page?

I agree that it's not likely to happen in practice, but that's true for 
a lot of kernel bugs that we nevertheless want to fix. Are you saying 
that the patch is bad or wrong?

> I don't know the speed impact of changing the strlen() there to
> strnlen().

I don't think we could or should do that, because it changes the 
semantics of the function.  I think C string operations are probably 
confusing enough already without breaking compatibility with existing 
documentation and other implementations.

Also, the size parameter is the size of the destination buffer, not the 
source string. So even if we could make it safe to use strnlen() in this 
case, it still wouldn't be safe in other cases where the source string 
is shorter than the destination.

>
>> The BSD man page:
>>
>> "Also note that strlcpy() and strlcat() only operate on true ``C''
>> strings.  This means that for strlcpy() src must be NUL-terminated
>> and for strlcat() both src and dst must be NUL-terminated."
>
> It's talking about the kind of strings.  If it's a string which includes
> NUL characters the strlcpy() won't work for that.  Or if it is not
> *supposed* to end in a NUL character then it won't work for that.
>
> We are using C strings here.

I think the man page is quite clear: "for strlcpy() src must be 
NUL-terminated". Without the patch, this is not necessarily the case.


Vegard

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

[Dan Carpenter]

On Fri, Mar 07, 2014 at 12:42:12PM +0100, Vegard Nossum wrote:
> On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> >On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> >>Both the in-kernel and BSD strlcpy() require that the source string is
> >>NUL terminated.
> >
> >No.  You're obviously wrong.  What on earth?
> 
> Well, from lib/string.c:
> 
> size_t strlcpy(char *dest, const char *src, size_t size)
> {
>         size_t ret = strlen(src);
>

Ah...  So you mean that we could read far beyond the end of the string
and it would be a DoS because there would be 4 gigs of memory before we
hit a NUL character.  That won't happen in this case because the user
only controls a small buffer.  Normal memory is full of NUL chars.

I don't know the speed impact of changing the strlen() there to
strnlen().

> The BSD man page:
> 
> "Also note that strlcpy() and strlcat() only operate on true ``C''
> strings.  This means that for strlcpy() src must be NUL-terminated
> and for strlcat() both src and dst must be NUL-terminated."

It's talking about the kind of strings.  If it's a string which includes
NUL characters the strlcpy() won't work for that.  Or if it is not
*supposed* to end in a NUL character then it won't work for that.

We are using C strings here.

regards,
dan carpenter

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

[Vegard Nossum]

On 03/07/2014 12:26 PM, Dan Carpenter wrote:
> On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
>> Both the in-kernel and BSD strlcpy() require that the source string is
>> NUL terminated.
>
> No.  You're obviously wrong.  What on earth?

Well, from lib/string.c:

size_t strlcpy(char *dest, const char *src, size_t size)
{
         size_t ret = strlen(src);

The BSD man page:

"Also note that strlcpy() and strlcat() only operate on true ``C'' 
strings.  This means that for strlcpy() src must be NUL-terminated and 
for strlcat() both src and dst must be NUL-terminated."


Vegard

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

[Dan Carpenter]

On Fri, Mar 07, 2014 at 11:56:04AM +0100, Vegard Nossum wrote:
> Both the in-kernel and BSD strlcpy() require that the source string is
> NUL terminated.

No.  You're obviously wrong.  What on earth?

regards,
dan carpenter

[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