Skip to contentSkip to conversation

isdnloop: several buffer overflows

PATCH In review Dan Carpenter 4 participants 5 messages Apr 8, 2014 – Apr 10, 2014

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

Dan Carpenter

author
#0 lore raw

PATCH 1/1 isdnloop: several buffer overflows

There are three buffer overflows addressed in this patch.

1) In isdnloop_fake_err() we add an 'E' to a 60 character string and then copy it into a 60 character buffer. I have made the destination buffer 64 characters and I'm changed the sprintf() to a snprintf().

2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60 character buffer so we have 54 characters. The ->eazlist[] is 11 characters long. I have modified the code to return if the source buffer is too long.

3) In isdnloop_command() the cbuf[] array was 60 characters long but the max length of the string then can be up to 79 characters. I made the cbuf array 80 characters long and changed the sprintf() to snprintf(). I also removed the temporary "dial" buffer and changed it to use "p" directly.

Unfortunately, we pass the "cbuf" string from isdnloop_command() to isdnloop_writecmd() which truncates anything over 60 characters to make it fit in card->omsg[]. (It can accept values up to 255 characters so long as there is a '\n' character every 60 characters). For now I have just fixed the memory corruption bug and left the other problems in this driver alone.

Signed-off-by Dan Carpenter <[email protected]>
drivers/isdn/isdnloop/isdnloop.c +9 -8
index e1f8748..5a4da94 100644
@@ -518,9 +518,9 @@ static isdnloop_stat isdnloop_cmd_table[]  static void
 isdnloop_fake_err(isdnloop_card *card)
 {
-	char buf[60];
+	char buf[64];
 
-	sprintf(buf, "E%s", card->omsg);
+	snprintf(buf, sizeof(buf), "E%s", card->omsg);
 	isdnloop_fake(card, buf, -1);
 	isdnloop_fake(card, "NAK", -1);
 }

@@ -903,6 +903,8 @@ isdnloop_parse_cmd(isdnloop_card *card)
 	case 7:
 		/* 0x;EAZ */
 		p += 3;
+		if (strlen(p) >= sizeof(card->eazlist[0]))
+			break;
 		strcpy(card->eazlist[ch - 1], p);
 		break;
 	case 8:

@@ -1133,7 +1135,7 @@ isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 {
 	ulong a;
 	int i;
-	char cbuf[60];
+	char cbuf[80];
 	isdn_ctrl cmd;
 	isdnloop_cdef cdef;
 

@@ -1198,7 +1200,6 @@ isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 			break;
 		if ((c->arg & 255) < ISDNLOOP_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];
 
 			a = c->arg;

@@ -1210,10 +1211,10 @@ isdnloop_command(isdn_ctrl *c, isdnloop_card *card)
 			} else
 				/* Normal Dial */
 				strcpy(dcode, "CAL");
-			strcpy(dial, p);
-			sprintf(cbuf, "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
-				dcode, dial, c->parm.setup.si1,
-				c->parm.setup.si2, c->parm.setup.eazmsn);
+			snprintf(cbuf, sizeof(cbuf),
+				 "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
+				 dcode, p, c->parm.setup.si1,
+				 c->parm.setup.si2, c->parm.setup.eazmsn);
 			i = isdnloop_writecmd(cbuf, strlen(cbuf), 0, card);
 		}
 		break;
#1 lore raw

From: Dan Carpenter

Quoted 1 line

There are three buffer overflows addressed in this patch.

...

Quoted 4 lines

2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60 character buffer so we have 54 characters. The ->eazlist[] is 11 characters long. I have modified the code to return if the source buffer is too long.

...

drivers/isdn/isdnloop/isdnloop.c isdnloop_parse_cmd(isdnloop_card *card) · line 910

Quoted 9 lines
drivers/isdn/isdnloop/isdnloop.c +2 -0
@@ -903,6 +903,8 @@ isdnloop_parse_cmd(isdnloop_card *card)
 	case 7:
 		/* 0x;EAZ */
 		p += 3;
+		if (strlen(p) >= sizeof(card->eazlist[0]))
+			break;
 		strcpy(card->eazlist[ch - 1], p);
 		break;
 	case 8:

If you've done the strlen() you might as well use memcpy(). There are also functions that will do a bounded strlen(), (eg memchr()).

David

author
#2 lore raw
Quoted 8 lines · David Laight

From: Dan Carpenter

Quoted 1 line

There are three buffer overflows addressed in this patch.

...

Quoted 4 lines

2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60 character buffer so we have 54 characters. The ->eazlist[] is 11 characters long. I have modified the code to return if the source buffer is too long.

...

Quoted 14 lines

drivers/isdn/isdnloop/isdnloop.c isdnloop_parse_cmd(isdnloop_card *card) · line 910

Quoted 9 lines
drivers/isdn/isdnloop/isdnloop.c +2 -0
@@ -903,6 +903,8 @@ isdnloop_parse_cmd(isdnloop_card *card)
 	case 7:
 		/* 0x;EAZ */
 		p += 3;
+		if (strlen(p) >= sizeof(card->eazlist[0]))
+			break;
 		strcpy(card->eazlist[ch - 1], p);
 		break;
 	case 8:

If you've done the strlen() you might as well use memcpy(). There are also functions that will do a bounded strlen(), (eg memchr()).

I re-wrote the patch based on your suggestion but decided that I prefer the original just because the diff is smaller. This is a driver that no one uses and it's full of bugs. Let's not worry about optimizing the slow paths at this point.

regards,
dan carpenter

maintainer
#3 lore raw
In reply to Dan Carpenter

From: Dan Carpenter <[email protected]>
Date: Tue, 8 Apr 2014 12:23:09 +0300

Quoted 17 lines

There are three buffer overflows addressed in this patch.

1) In isdnloop_fake_err() we add an 'E' to a 60 character string and then copy it into a 60 character buffer. I have made the destination buffer 64 characters and I'm changed the sprintf() to a snprintf().

2) In isdnloop_parse_cmd(), p points to a 6 characters into a 60 character buffer so we have 54 characters. The ->eazlist[] is 11 characters long. I have modified the code to return if the source buffer is too long.

3) In isdnloop_command() the cbuf[] array was 60 characters long but the max length of the string then can be up to 79 characters. I made the cbuf array 80 characters long and changed the sprintf() to snprintf(). I also removed the temporary "dial" buffer and changed it to use "p" directly.

Quoted 8 lines

Unfortunately, we pass the "cbuf" string from isdnloop_command() to isdnloop_writecmd() which truncates anything over 60 characters to make it fit in card->omsg[]. (It can accept values up to 255 characters so long as there is a '\n' character every 60 characters). For now I have just fixed the memory corruption bug and left the other problems in this driver alone.

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

Applied and queued up for -stable, thanks.

This thing is marked broken-on-smp so the impact is very close to zero.

#4 lore raw
Quoted 4 lines · David Miller

Applied and queued up for -stable, thanks.

This thing is marked broken-on-smp so the impact is very close to zero.

isdnloop is a loopback test driver for the deprecated old ISDN4Linux subsystem.
I doubt anyone is still interested in it.

Signature · 2 lines
Tilman Schmidt
[email protected]