isdn: icn: buffer overflow in icn_command()

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>

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

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.


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

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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