[bug report] buffer overflow in isdn capi

[bug report] buffer overflow in isdn capi

[Dan Carpenter]

The command_2_index() function is buggy and leads to a buffer overflow.
Does anyone know how to fix this?

drivers/isdn/capi/capiutil.c
   403  static unsigned command_2_index(unsigned c, unsigned sc)
   404  {
   405          if (c & 0x80)
   406                  c = 0x9 + (c & 0x0f);
   407          else if (c <= 0x0f);
   408          else if (c == 0x41)
   409                  c = 0x9 + 0x1;
   410          else if (c == 0xff)
   411                  c = 0x00;
   412          return (sc & 3) * (0x9 + 0x9) + c;
   413  }

Imagine that we input c = 0x7f and sc = 0x3.  Then 3 * 18 + 127 = 181
and we return 181.

The other thing that stands out to me is that the last condition
"(c == 0xff)" is never true because then the first condition
"(c & 0x80)" would have been true already.

Here is how the function is used:

drivers/isdn/capi/capiutil.c
   564  /**
   565   * capi_message2cmsg() - disassemble CAPI 2.0 message into _cmsg structure
   566   * @cmsg:       _cmsg structure
   567   * @msg:        buffer for assembled message
   568   *
   569   * Return value: 0 for success
   570   */
   571  
   572  unsigned capi_message2cmsg(_cmsg *cmsg, u8 *msg)
   573  {
   574          memset(cmsg, 0, sizeof(_cmsg));
   575          cmsg->m = msg;
   576          cmsg->l = 8;
   577          cmsg->p = 0;
   578          byteTRcpy(cmsg->m + 4, &cmsg->Command);
   579          byteTRcpy(cmsg->m + 5, &cmsg->Subcommand);
   580          cmsg->par = cpars[command_2_index(cmsg->Command, cmsg->Subcommand)];
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cpars = is a 79 element array.
cmsg->Command and cmsg->Subcommand come from skb->data so we can't trust
them.
181 is past the end of the 79 element array.

   581  
   582          message_2_pars(cmsg);
   583  
   584          wordTRcpy(msg + 0, &cmsg->l);
   585          wordTRcpy(cmsg->m + 2, &cmsg->ApplId);
   586          wordTRcpy(cmsg->m + 6, &cmsg->Messagenumber);
   587  
   588          return 0;
   589  }

regards,
dan carpenter

Re: [bug report] buffer overflow in isdn capi

[Joe Perches]

On Tue, 2014-04-01 at 18:48 +0300, Dan Carpenter wrote:
> The command_2_index() function is buggy and leads to a buffer overflow.
> Does anyone know how to fix this?
> 
> drivers/isdn/capi/capiutil.c
>    403  static unsigned command_2_index(unsigned c, unsigned sc)

These arguments should be u8 as well.

>    404  {
>    405          if (c & 0x80)
>    406                  c = 0x9 + (c & 0x0f);
>    407          else if (c <= 0x0f);
>    408          else if (c == 0x41)
>    409                  c = 0x9 + 0x1;
>    410          else if (c == 0xff)
>    411                  c = 0x00;
>    412          return (sc & 3) * (0x9 + 0x9) + c;
>    413  }
> 
> Imagine that we input c = 0x7f and sc = 0x3.  Then 3 * 18 + 127 = 181
> and we return 181.
> 
> The other thing that stands out to me is that the last condition
> "(c == 0xff)" is never true because then the first condition
> "(c & 0x80)" would have been true already.

Re: [bug report] buffer overflow in isdn capi

[Karsten Keil]

Hi Dan,

thanks for spotting this, really a bad thing.
Fortunately it is only a local issue, the messages from the ISDN bus
are translated in valid command/subcommand pairs in the controller
firmware, so even if you send random data on the D-channel
it should not result in a buffer overflow.
But of course a local user could send wrong messages to the kernel
via the /dev/capi20 device which then may let to the described out
of bound access.

Am 01.04.2014 17:48, schrieb Dan Carpenter:
> The command_2_index() function is buggy and leads to a buffer overflow.
> Does anyone know how to fix this?
> 

I think I have an idea, I will post a proposal soon.

> drivers/isdn/capi/capiutil.c
>    403  static unsigned command_2_index(unsigned c, unsigned sc)
>    404  {
>    405          if (c & 0x80)
>    406                  c = 0x9 + (c & 0x0f);
>    407          else if (c <= 0x0f);
>    408          else if (c == 0x41)
>    409                  c = 0x9 + 0x1;
>    410          else if (c == 0xff)
>    411                  c = 0x00;
>    412          return (sc & 3) * (0x9 + 0x9) + c;
>    413  }
> 
> Imagine that we input c = 0x7f and sc = 0x3.  Then 3 * 18 + 127 = 181
> and we return 181.

Yes need to check the array size at least, but this is not enough.

> 
> The other thing that stands out to me is that the last condition
> "(c == 0xff)" is never true because then the first condition
> "(c & 0x80)" would have been true already.

Yes this should be the first check or a nested if in the current first
check. command FF is the MANUFACTURER command and normally not
implemented at all.

> 
> Here is how the function is used:
> 
> drivers/isdn/capi/capiutil.c
>    564  /**
>    565   * capi_message2cmsg() - disassemble CAPI 2.0 message into _cmsg structure
>    566   * @cmsg:       _cmsg structure
>    567   * @msg:        buffer for assembled message
>    568   *
>    569   * Return value: 0 for success
>    570   */
>    571  
>    572  unsigned capi_message2cmsg(_cmsg *cmsg, u8 *msg)
>    573  {
>    574          memset(cmsg, 0, sizeof(_cmsg));
>    575          cmsg->m = msg;
>    576          cmsg->l = 8;
>    577          cmsg->p = 0;
>    578          byteTRcpy(cmsg->m + 4, &cmsg->Command);
>    579          byteTRcpy(cmsg->m + 5, &cmsg->Subcommand);
>    580          cmsg->par = cpars[command_2_index(cmsg->Command, cmsg->Subcommand)];
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> cpars = is a 79 element array.
> cmsg->Command and cmsg->Subcommand come from skb->data so we can't trust
> them.
> 181 is past the end of the 79 element array.

correct and this is not the only issue here. If you pass a value which
is not a valid command, but will result in a index inside the array
boundaries, it will result in  cmsg->par = NULL, which is also not
handled properly in the parser functions.

Re: [bug report] buffer overflow in isdn capi

[Tilman Schmidt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello all,

on 01.04.2014 17:48, Dan Carpenter wrote:
> The command_2_index() function is buggy and leads to a buffer
> overflow. Does anyone know how to fix this?

AFAICS this is still unfixed.
As an easy fix I propose:

- --- a/drivers/isdn/capi/capiutil.c
+++ b/drivers/isdn/capi/capiutil.c
@@ -203,14 +203,7 @@ static unsigned char *cpars[] =
 /*-------------------------------------------------------*/
 static unsigned command_2_index(unsigned c, unsigned sc)
 {
- -       if (c & 0x80)
- -               c = 0x9 + (c & 0x0f);
- -       else if (c <= 0x0f);
- -       else if (c == 0x41)
- -               c = 0x9 + 0x1;
- -       else if (c == 0xff)
- -               c = 0x00;
- -       return (sc & 3) * (0x9 + 0x9) + c;
+       return (sc & 3) * (0x9 + 0x9) + ((c & 0xf0) ? 0x9 : 0) + (c &
0x0f);
 }

 /*-------------------------------------------------------*/

This produces identical results to the current function for
all legal values of c (0x00..0x08, 0x41, 0x80..0x88, 0xff)
and guarantees that the result is <= 0x4e for all possible
inputs, whether legal or not. It also makes it clearer what
that function actually does.

Unless somebody points out a serious flaw with this I'll test it
and submit a formal patch.

Karsten's other concern about unhandled NULL pointers in the
mnames[] and cpars[] arrays should be addressed separately IMHO.

Regards,
Tilman

- -- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlOM/yIACgkQQ3+did9BuFuwMwCfVZq5Lx0P+ddhe/5WlxuO5zzp
VV4AoIn50I1wa4r4DtWfHFErMysgjsxr
=k/+h
-----END PGP SIGNATURE-----