[patch] isdn: icn: buffer overflow in icn_command()

[patch] isdn: icn: buffer overflow in icn_command()

[Dan Carpenter]

The cbuf[] buffer is 60 characters but we're putting a potential 79
characters and a NUL into it.  I've made it 80 characters and changed
the sprintf() to snprintf().

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

diff --git a/drivers/isdn/icn/icn.c b/drivers/isdn/icn/icn.c
index 53d487f..88c0603 100644
--- a/drivers/isdn/icn/icn.c
+++ b/drivers/isdn/icn/icn.c
@@ -1155,7 +1155,7 @@ icn_command(isdn_ctrl *c, icn_card *card)
 	ulong a;
 	ulong flags;
 	int i;
-	char cbuf[60];
+	char cbuf[80];
 	isdn_ctrl cmd;
 	icn_cdef cdef;
 	char __user *arg;
@@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			break;
 		if ((c->arg & 255) < ICN_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];
 
 			a = c->arg;
@@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);
 		}
 		break;

Re: [patch] isdn: icn: buffer overflow in icn_command()

[Joe Perches]

On Mon, 2014-04-14 at 11:07 +0300, Dan Carpenter wrote:
> The cbuf[] buffer is 60 characters but we're putting a potential 79
> characters and a NUL into it.  I've made it 80 characters and changed
> the sprintf() to snprintf().
[]
> @@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
>  			break;
>  		if ((c->arg & 255) < ICN_BCH) {
>  			char *p;
> -			char dial[50];
>  			char dcode[4];

The change log does not mentioned removal of dial.

As this subsystem likely has 0 active users, perhaps
making the minimal correctness change might be better.

Also, if making other changes, perhaps it'd be better
to similarly replace dcode with a pointer as well.

> @@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);

Maybe save the snprintf result length and use it
in icn_writecmd too.

Re: [patch] isdn: icn: buffer overflow in icn_command()

[Dan Carpenter]

On Mon, Apr 14, 2014 at 08:52:28AM -0700, Joe Perches wrote:
> On Mon, 2014-04-14 at 11:07 +0300, Dan Carpenter wrote:
> > The cbuf[] buffer is 60 characters but we're putting a potential 79
> > characters and a NUL into it.  I've made it 80 characters and changed
> > the sprintf() to snprintf().
> []
> > @@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
> >  			break;
> >  		if ((c->arg & 255) < ICN_BCH) {
> >  			char *p;
> > -			char dial[50];
> >  			char dcode[4];
> 
> The change log does not mentioned removal of dial.
> 

Oops.  Sorry about that.

> As this subsystem likely has 0 active users, perhaps
> making the minimal correctness change might be better.
> 
> Also, if making other changes, perhaps it'd be better
> to similarly replace dcode with a pointer as well.

I thought about that when I was writing the patch, but I wanted to make
minimal changes.  The dial change is necessary because static checkers
would assume we are using all 50 characters of the dial[] buffer instead
of just the first 32.

> 
> > @@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);
> 
> Maybe save the snprintf result length and use it
> in icn_writecmd too.
> 

snprintf() returns the number of bytes which would have been printed if
there were enough space and not the number of bytes in the string.
Using the value from snprintf() would not introduce a bug because I have
carefully counted the number of bytes in the output string, but it would
hopefully annoy human auditors of this code.  ;)  You are thinking of
scnprintf().

I'm going to apply your minimal changes suggestion here.

regards,
dan carpenter

[patch v2] isdn: icn: buffer overflow in icn_command()

[Dan Carpenter]

This buffer over was detected using static analysis:

	drivers/isdn/icn/icn.c:1325 icn_command()
	error: format string overflow. buf_size: 60 length: 98

The calculation for the length of the string is off because it assumes
that the dial[] buffer holds a 50 character string, but actually it is
at most 31 characters and NUL.  I have removed the dial[] buffer because
it isn't needed.

The maximum length of the string is actually 79 characters and a NUL.  I
have made the cbuf[] array large enough to hold it and changed the
sprintf() to an snprintf() as a further safety enhancement.

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

diff --git a/drivers/isdn/icn/icn.c b/drivers/isdn/icn/icn.c
index 53d487f..88c0603 100644
--- a/drivers/isdn/icn/icn.c
+++ b/drivers/isdn/icn/icn.c
@@ -1155,7 +1155,7 @@ icn_command(isdn_ctrl *c, icn_card *card)
 	ulong a;
 	ulong flags;
 	int i;
-	char cbuf[60];
+	char cbuf[80];
 	isdn_ctrl cmd;
 	icn_cdef cdef;
 	char __user *arg;
@@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
 			break;
 		if ((c->arg & 255) < ICN_BCH) {
 			char *p;
-			char dial[50];
 			char dcode[4];
 
 			a = c->arg;
@@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);
 		}
 		break;

Re: [patch] isdn: icn: buffer overflow in icn_command()

[Joe Perches]

On Wed, 2014-04-16 at 14:16 +0300, Dan Carpenter wrote:

> snprintf() returns the number of bytes which would have been printed if
> there were enough space and not the number of bytes in the string.
> Using the value from snprintf() would not introduce a bug because I have
> carefully counted the number of bytes in the output string, but it would
> hopefully annoy human auditors of this code.  ;)  You are thinking of
> scnprintf().

Not really, I was assuming you'd use max() too
but you're right, scnprintf is more sensible.

> I'm going to apply your minimal changes suggestion here.

swell, thanks

Re: [patch v2] isdn: icn: buffer overflow in icn_command()

[walter harms]

Am 16.04.2014 13:25, schrieb Dan Carpenter:
> This buffer over was detected using static analysis:
> 
> 	drivers/isdn/icn/icn.c:1325 icn_command()
> 	error: format string overflow. buf_size: 60 length: 98
> 
> The calculation for the length of the string is off because it assumes
> that the dial[] buffer holds a 50 character string, but actually it is
> at most 31 characters and NUL.  I have removed the dial[] buffer because
> it isn't needed.
> 
> The maximum length of the string is actually 79 characters and a NUL.  I
> have made the cbuf[] array large enough to hold it and changed the
> sprintf() to an snprintf() as a further safety enhancement.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: update changelog.
> 
> diff --git a/drivers/isdn/icn/icn.c b/drivers/isdn/icn/icn.c
> index 53d487f..88c0603 100644
> --- a/drivers/isdn/icn/icn.c
> +++ b/drivers/isdn/icn/icn.c
> @@ -1155,7 +1155,7 @@ icn_command(isdn_ctrl *c, icn_card *card)
>  	ulong a;
>  	ulong flags;
>  	int i;
> -	char cbuf[60];
> +	char cbuf[80];
>  	isdn_ctrl cmd;
>  	icn_cdef cdef;
>  	char __user *arg;
> @@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
>  			break;
>  		if ((c->arg & 255) < ICN_BCH) {
>  			char *p;
> -			char dial[50];
>  			char dcode[4];
>  
>  			a = c->arg;
> @@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_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 = icn_writecmd(cbuf, strlen(cbuf), 0, card);
>  		}
>  		break;
> 

if someone is still working on this ...
maybe a vararg version of icn_writecmd() would be a nice helper.

re,
 wh

Re: [patch v2] isdn: icn: buffer overflow in icn_command()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 16 Apr 2014 14:25:16 +0300

> This buffer over was detected using static analysis:
> 
> 	drivers/isdn/icn/icn.c:1325 icn_command()
> 	error: format string overflow. buf_size: 60 length: 98
> 
> The calculation for the length of the string is off because it assumes
> that the dial[] buffer holds a 50 character string, but actually it is
> at most 31 characters and NUL.  I have removed the dial[] buffer because
> it isn't needed.
> 
> The maximum length of the string is actually 79 characters and a NUL.  I
> have made the cbuf[] array large enough to hold it and changed the
> sprintf() to an snprintf() as a further safety enhancement.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.