netdev - Re: [bug report] buffer overflow in isdn capi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <533C3EEE.6020004@linux-pingi.de>
Date:	Wed, 02 Apr 2014 18:46:38 +0200
From:	Karsten Keil <kkeil@...ux-pingi.de>
To:	Dan Carpenter <dan.carpenter@...cle.com>,
	Karsten Keil <isdn@...ux-pingi.de>
CC:	netdev@...r.kernel.org
Subject: Re: [bug report] buffer overflow in isdn capi

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.


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