Skip to contentSkip to conversation

isdn: make sure strings are null terminated

PATCH Superseded Dan Carpenter 6 participants 10 messages Nov 23, 2011 – Nov 29, 2011

Subsystems (3)
Networking Network drivers Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 isdn: make sure strings are null terminated

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/divert/divert_procfs.c +4 -0
index 33ec9e4..0c16687 100644
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd = IIOCDOCFACT) ? 1 : (cmd = IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,
#1 lore raw
Quoted 11 lines · Dan Carpenter

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/divert/divert_procfs.c +0 -0
index 33ec9e4..0c16687 100644

drivers/isdn/divert/divert_procfs.c static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg) · line 246

Quoted 5 lines
drivers/isdn/divert/divert_procfs.c +2 -0
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;

This looks buggy.

If string is not null terminated, how strlen() will stop you from going out of bounds, and trigger some run time checker ?

strnlen() would be more effective...

Quoted 6 lines
drivers/isdn/divert/divert_procfs.c +2 -1
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd = IIOCDOCFACT) ? 1 : (cmd = IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,
--
author
#2 lore raw
Quoted 10 lines · Eric Dumazet

Review

Quoted 2 lines
(quoted diff) +2 -0
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;

This looks buggy.

If string is not null terminated, how strlen() will stop you from going out of bounds, and trigger some run time checker ?

strnlen() would be more effective...

Aw crap. My first version used strnlen() and I redid it to be simpler. I just figured that it doesn't take long to hit a zeroed u8.

I'll resend all three strlen() patches to use strnlen().

regards,
dan carpenter

author
#3 lore raw

PATCH v2 1/1 isdn: make sure strings are null terminated

In reply to Eric Dumazet

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

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

---
v2: use strnlen() instead of strlen().

drivers/isdn/divert/divert_procfs.c +6 -0
index 33ec9e4..9021182 100644
@@ -242,6 +242,12 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strnlen(dioctl.cf_ctrl.msn, sizeof(dioctl.cf_ctrl.msn)) ==
+					sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strnlen(dioctl.cf_ctrl.fwd_nr, sizeof(dioctl.cf_ctrl.fwd_nr)) ==
+					sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd == IIOCDOCFACT) ? 1 : (cmd == IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,
maintainer Applied
#4 lore raw

From: Dan Carpenter <[email protected]>
Date: Thu, 24 Nov 2011 15:41:49 +0300

Quoted 7 lines

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

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

---
v2: use strnlen() instead of strlen().

Applied.

#5 lore raw
In reply to Dan Carpenter

Am 23.11.2011 07:42, schrieb Dan Carpenter:

Quoted 11 lines

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/divert/divert_procfs.c +0 -0
index 33ec9e4..0c16687 100644

drivers/isdn/divert/divert_procfs.c static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg) · line 248

Quoted 7 lines
drivers/isdn/divert/divert_procfs.c +4 -0
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;

forcing the last field to be zero seems more easy. dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;

re,
wh

Quoted 9 lines
drivers/isdn/divert/divert_procfs.c +0 -1
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd = IIOCDOCFACT) ? 1 : (cmd = IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#6 lore raw
Quoted 9 lines · walter harms

Am 23.11.2011 07:42, schrieb Dan Carpenter:

Quoted 6 lines

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

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

drivers/isdn/divert/divert_procfs.c static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg) · line 248

Quoted 12 lines
drivers/isdn/divert/divert_procfs.c +4 -0
index 33ec9e4..0c16687 100644
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;

forcing the last field to be zero seems more easy. dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;

That's a valid option to use, but I'd prefer to return an error code here because that's what we do on the line before. Passing a too long string is clearly invalid.

regards,
dan carpenter

#7 lore raw

Am 24.11.2011 12:34, schrieb Dan Carpenter:

Quoted 27 lines · walter harms
Quoted 9 lines

Am 23.11.2011 07:42, schrieb Dan Carpenter:

Quoted 6 lines

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

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

drivers/isdn/divert/divert_procfs.c static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg) · line 248

Quoted 12 lines
drivers/isdn/divert/divert_procfs.c +4 -0
index 33ec9e4..0c16687 100644
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;

forcing the last field to be zero seems more easy. dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;

Quoted 4 lines

That's a valid option to use, but I'd prefer to return an error code here because that's what we do on the line before. Passing a too long string is clearly invalid.

the line before has the same problem, of cause.

So far i see you do not get a string, you get a structure. An it will hard to validate the element is a useful string. I thing my (sledgehammer) method is ok here because you make sure that all later calls (strcmp,strcpy) will succeed. If someone supplies a bad string the later calls will catch by failing to identify and return a proper code from there (at least i hope so).

re,
wh

#8 lore raw
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +2 -0
+			if (strlen(dioctl.cf_ctrl.msn) >sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;

...

Quoted 10 lines

So far i see you do not get a string, you get a structure. And it will hard to validate the element is a useful string. I think my (sledgehammer) method is ok here because you make sure that all later calls (strcmp,strcpy) will succeed. If someone supplies a bad string the later calls will catch by failing to identify and return a proper code from there (at least i hope so).

re,
wh

Except that the strlen() can run right off the end of the structure - and might eventually fault. You need to use something like strnlen().

David

#9 lore raw
In reply to walter harms
Quoted 35 lines · walter harms

Am 24.11.2011 12:34, schrieb Dan Carpenter:

Quoted 27 lines · walter harms
Quoted 9 lines

Am 23.11.2011 07:42, schrieb Dan Carpenter:

Quoted 6 lines

These strings come from the user. We strcpy() them inside cf_command() so we should check that they are NULL terminated and return an error if not.

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

drivers/isdn/divert/divert_procfs.c static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg) · line 248

Quoted 12 lines
drivers/isdn/divert/divert_procfs.c +4 -0
index 33ec9e4..0c16687 100644
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;

forcing the last field to be zero seems more easy. dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;

Quoted 4 lines

That's a valid option to use, but I'd prefer to return an error code here because that's what we do on the line before. Passing a too long string is clearly invalid.

Quoted 8 lines

the line before has the same problem, of cause.

So far i see you do not get a string, you get a structure. An it will hard to validate the element is a useful string. I thing my (sledgehammer) method is ok here because you make sure that all later calls (strcmp,strcpy) will succeed. If someone supplies a bad string the later calls will catch by failing to identify and return a proper code from there (at least i hope so).

Since the proper userspace programs should use the correct size, these valid fixes (Thanks !) will prevent bad programs to use this for some bad purpose. I think both methods are OK, I tend to the sledgehammer, because it is the lowest CPU consuming method - but on the other side the valid length is small enough.

Thanks
Karsten