isdnloop: NUL-terminate strings from userspace

PATCH In review Vegard Nossum 8 participants 14 messages Apr 1, 2014 – Apr 15, 2014

Subsystems (5)

Networking Network drivers Stable branch Library code +1 more subsystems

lore

Share & tools

lore

Vegard Nossum

author

2014-04-01 10:08 UTC #0 lore raw

PATCH 1/1 isdnloop: NUL-terminate strings from userspace

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 <[email protected]>

Cc David S. Miller <[email protected]>

Cc [email protected]

Signed-off-by Vegard Nossum <[email protected]>

---
 drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
 1 file changed, 8 insertions(+)

drivers/isdn/isdnloop/isdnloop.c +8 -0

index 02125e6..50cd348 100644
@@ -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:

Signature · 1 line

1.7.10.4

Hannes Frederic Sowa

2014-04-01 10:30 UTC #1 lore raw

Quoted 21 lines · 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 <[email protected]>
Cc David S. Miller <[email protected]>
Cc [email protected]
Signed-off-by Vegard Nossum <[email protected]>
---
 drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
 1 file changed, 8 insertions(+)
drivers/isdn/isdnloop/isdnloop.c +0 -0
index 02125e6..50cd348 100644
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;

drivers/isdn/isdnloop/isdnloop.c isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp) · line 1080

Quoted 10 lines

drivers/isdn/isdnloop/isdnloop.c +8 -0

 	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

Vegard Nossum

author

2014-04-01 10:46 UTC #2 lore raw

Quoted 36 lines · Hannes Frederic Sowa

Quoted 21 lines · 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 <[email protected]>
Cc David S. Miller <[email protected]>
Cc [email protected]
Signed-off-by Vegard Nossum <[email protected]>
---
  drivers/isdn/isdnloop/isdnloop.c |    8 ++++++++
  1 file changed, 8 insertions(+)
drivers/isdn/isdnloop/isdnloop.c +0 -0
index 02125e6..50cd348 100644
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
  		return -EBUSY;
drivers/isdn/isdnloop/isdnloop.c isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp) · line 1080
Quoted 10 lines
drivers/isdn/isdnloop/isdnloop.c +8 -0
  	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?

Quoted 2 lines

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

Hannes Frederic Sowa

2014-04-01 11:02 UTC #3 lore raw

Quoted 9 lines · Vegard Nossum

Quoted 7 lines · Hannes Frederic Sowa
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?

Quoted 5 lines

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@.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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

Dan Carpenter

2014-04-01 12:35 UTC #4 lore raw

Quoted 22 lines · Hannes Frederic Sowa

Quoted 9 lines · Vegard Nossum
Quoted 7 lines · Hannes Frederic Sowa
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?
Quoted 5 lines
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@.
Quoted 4 lines
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:

Quoted 8 lines

Quoted 4 lines
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

David Miller

maintainer

2014-04-01 20:18 UTC #5 lore raw

From: Dan Carpenter <[email protected]>
Date: Tue, 1 Apr 2014 15:35:34 +0300

Quoted 2 lines

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.

YOSHIFUJI Hideaki

2014-04-02 03:54 UTC #6 lore raw

PATCH 1/1 isdnloop: Validate NUL-terminated strings from user.

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

Signed-off-by YOSHIFUJI Hideaki <[email protected]>

---
 drivers/isdn/isdnloop/isdnloop.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/isdn/isdnloop/isdnloop.c +6 -0

index 02125e6..e1f8748 100644
@@ -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:

Signature · 1 line

1.7.9.5

David Miller

maintainer

2014-04-03 15:25 UTC #7 lore raw

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

From: YOSHIFUJI Hideaki <[email protected]>
Date: Wed, 02 Apr 2014 12:48:42 +0900

Quoted 4 lines

Return -EINVAL unless all of user-given strings are correctly NUL-terminated.
Signed-off-by YOSHIFUJI Hideaki <[email protected]>

Applied and queud up for -stable, thanks!

Dan Carpenter

2014-04-02 08:47 UTC #8 lore raw

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

In reply to David Miller

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 <[email protected]>

lib/string.c +4 -4

index 9b1f906..89ad0f0 100644
@@ -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.

Dan Carpenter

2014-04-02 08:48 UTC #9 lore raw

PATCH 2/2 lib/string.c: strlcpy() might read too far

In reply to David Miller

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 <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

lib/string.c +2 -2

index 9b1f906..8074962 100644
@@ -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';
 	}

Andrew Morton

maintainer

2014-04-14 21:46 UTC #10 lore raw

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

Quoted 3 lines · On Wed, 2 Apr 2014 11:47:31 +0300 Dan Carpenter <[email protected]>…

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.

Quoted 7 lines

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 ;)

Dan Carpenter

2014-04-15 10:38 UTC #11 lore raw

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

Quoted 10 lines · Andrew Morton

Quoted 3 lines · On Wed, 2 Apr 2014 11:47:31 +0300 Dan Carpenter <[email protected]>…
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

Dave Jones

2014-04-14 22:21 UTC #12 lore raw

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

In reply to Dan Carpenter

Quoted 8 lines · 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 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

YOSHIFUJI Hideaki

2014-04-01 11:23 UTC #13 lore raw

In reply to Vegard Nossum

Quoted 5 lines · 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().

Quoted 6 lines

drivers/isdn/isdnloop/isdnloop.c +0 -0

index 02125e6..50cd348 100644
@@ -1070,6 +1070,14 @@ isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp)
 		return -EBUSY;

drivers/isdn/isdnloop/isdnloop.c isdnloop_start(isdnloop_card *card, isdnloop_sdef *sdefp) · line 1080

Quoted 10 lines

drivers/isdn/isdnloop/isdnloop.c +8 -0

 	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