[Home]

Summary:DAHLIN-00062: [patch] incoherent handling of span timing and lack of input validation
Reporter:Tzafrir Cohen (tzafrir)Labels:
Date Opened:2008-11-23 05:42:09.000-0600Date Closed:2009-02-08 06:07:02.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 01-wcte11xp-timing.diff
( 1) tor2_timing_validation.diff
( 2) torisa_timing_validation.diff
Description:CVE-2008-5744

We connected two units of Astribank 8-port BRI to a system and generated configuration for that with dahdi_genconf . The generated configuration includes:

 span=1,1,...
 span=2,2,...
   ...
 span=15,15,...
 span=16,16,...

Sadly dahdi_cfg did not like that last line:

 line 20: Timing should be a number from 0 to 15, not '16'

Hmmm... who cares about that number anyway? Isn't it a simple priority? Weel, not. dahdi-base.c does not use lc->sync . Thus it at the moment has nothing to do with sync master selection, and only optionally used by channel drivers.

How do channel drivers use it?

In xpp it is handled by the card_X drivers. In card_bri.c it is ignored. In card_pri.c it is used for selecting the syncer among the ports of the same Astribank PRI unit (but any value is valid: the highest wins).

wcte12xp also gives it a clear meaning: either 0 (we provide sync to ther other side) or anything else (we take sync from the other side).

For wct4xxp valid values seem to be 0-3 . Any value above 4 is considered as 0. Hence the configuration generated by dahdi_genconf for a system with two such 4-port cards will be invalid.

So far all drivers have at least validated their input. But now we get to the good part. wcte11xp.c write the value that they recieve from userspace to a register:

 control_set_reg(wc, WC_CLOCK, 0x06 | wc->sync | clockextra);

I don't have the specs here so I'm not sure what is the actual impact here of an invalid value.

But the prize goes to tor2.c . It has the code as wct4xp.c , except the input validation part:

       if (lc->sync) {
               p->tor->syncs[lc->sync - 1] = span->spanno;
               p->tor->psyncs[lc->sync - 1] = p->span + 1;
       }

So first those drivers need fixing. Then we need to figure out exactly what the "timing" parameter means.

Note that this is exploitable by anybody who has write access to zaptel.conf/system.conf and by anybody who has write access to /dev/dahdi/ctl . There's almost no good reason why Asterisk would need to write there. Yet this bug is exploitable by the Asterisk user rather than merely by root.

Added a patch for tor2 in dahdi. It applies to tor2 in Zaptel. It uses a different approach than wct4xxp. Which approach do you prefer?
Comments:By: Shaun Ruffell (sruffell) 2008-11-24 13:21:47.000-0600

The wcte11xp should use the same mechanism as the wcte12xp, either 0 or 1 to indicate sync on receive or not.

Still need to check the logic on the wct4xxp. I uploaded 01-wcte11xp-timing.diff with the change.


By: Shaun Ruffell (sruffell) 2008-11-25 13:09:14.000-0600

As far as I can tell, the worst thing that would happen is that the timing is messed up and the span will be inaccessible.  So, an asterisk user could make the span inaccessible if they have access to those dev files anyway, so I don't think this will block the 2.1.0 release.

Agreed as far as you can tell?
By: Shaun Ruffell (sruffell) 2008-11-25 14:02:26.000-0600

The wct4xxp driver needs a little more thought since timing can possibly be shared with other cards via a cable.
By: Shaun Ruffell (sruffell) 2008-11-25 14:03:43.000-0600

I committed revision 5384 and 5383 for this issue.

http://svn.digium.com/view/dahdi?view=rev&rev=5383
http://svn.digium.com/view/dahdi?view=rev&rev=5384
By: Tzafrir Cohen (tzafrir) 2008-11-25 14:28:22.000-0600

Resolved in dahdi in

http://svn.digium.com/view/dahdi?view=rev&rev=5383 tor2.c
http://svn.digium.com/view/dahdi?view=rev&rev=5384 wcte11xp.c , wct1xxp.c

Resolved in dahdi 2.1.0-rc5 .

(commits by sruffell)


By: Tzafrir Cohen (tzafrir) 2008-11-25 14:57:56.000-0600

Leaving open to mark the different meanings of the "timing" parameter. This is something that needs resolving eventually.
By: Digium Subversion (svnbot) 2008-11-25 14:59:15.000-0600

Repository: zaptel
Revision: 4587

U   branches/1.2/tor2.c
U   branches/1.2/torisa.c
U   branches/1.2/wct1xxp.c
U   branches/1.2/wcte11xp.c

------------------------------------------------------------------------
r4587 | tzafrir | 2008-11-25 14:59:14 -0600 (Tue, 25 Nov 2008) | 2 lines

Fixing issue DAHLIN-62 in zaptel 1.2.

------------------------------------------------------------------------

http://svn.digium.com/view/zaptel?view=rev&revision=4587
By: Digium Subversion (svnbot) 2008-11-25 15:01:00.000-0600

Repository: zaptel
Revision: 4588

U   branches/1.4/kernel/tor2.c
U   branches/1.4/kernel/torisa.c
U   branches/1.4/kernel/wct1xxp.c
U   branches/1.4/kernel/wcte11xp.c

------------------------------------------------------------------------
r4588 | tzafrir | 2008-11-25 15:00:59 -0600 (Tue, 25 Nov 2008) | 4 lines

Fix handling of an invalid timing (sync) parameter in DAHDI_SPANCONFIG.

Fixing issue DAHLIN-62 in zaptel 1.4

------------------------------------------------------------------------

http://svn.digium.com/view/zaptel?view=rev&revision=4588
By: Digium Subversion (svnbot) 2008-12-14 17:00:26.000-0600

Repository: dahdi
Revision: 5530

U   tools/trunk/dahdi_cfg.c

------------------------------------------------------------------------
r5530 | tzafrir | 2008-12-14 17:00:26 -0600 (Sun, 14 Dec 2008) | 5 lines

Allow the span timing to be up to 255.

The test for 15 was ineffective anyway. Some drivers can use higher
values. See issue DAHLIN-62 .

------------------------------------------------------------------------

http://svn.digium.com/view/dahdi?view=rev&revision=5530
By: Jan Lieskovsky (iankko) 2008-12-19 05:14:57.000-0600

Hello guys,

 Eugene Teo noticed, there is problem with the dahdi/tor2.c patch:

In http://bugs.digium.com/file_download.php?file_id=20796&type=bug
we add checking if lc->sync is in range of 0-63 there:

tor2.c:#define MAX_TOR_CARDS 64

+ if ((lc->sync < 0) || (lc->sync >= MAX_TOR_CARDS)) {

but later in the code, we assign the value of 'span->spanno'
to array item with index of  'p->tor->syncs[lc->sync - 1]'.

The relevant part of the code is here:

   215         p->tor->syncpos[p->span] = lc->sync;
   216         /* if a sync src, put it in the proper place */
   217         if (lc->sync) {
   218                 p->tor->syncs[lc->sync - 1] = span->spanno;
   219                 p->tor->psyncs[lc->sync - 1] = p->span + 1;
   220         }

The problem is, p->tor->syncs is defined only as array containing 4 items:
In tor2_spanconfig p is defined to be struct tor2_span *p = span->pvt;
where 'tor' in tor2_span struct is defined as:

struct tor2_span {
       /* Private pointer for span.  We want to know our
          span number and pointer to the tor device */
       struct tor2 *tor;

and 'syncs' item in the tor2 structure is defined as:

int syncs[SPANS_PER_CARD];      /* sync sources */

but according to: tor2.c:#define SPANS_PER_CARD  4
syncs can will be only 4 items array. So in case,
lc->sync will be still in the range of 0-63, but for example
63, we will try to assign:

span->spanno to array item with index of 62:

p->tor->syncs[lc->sync - 1] = span->spanno;

i.e. the array will overflow. Similar problem is in row:
 
219                 p->tor->psyncs[lc->sync - 1] = p->span + 1;

as psyncs in tor2 array is defined also as only containing max 4 items:

 int psyncs[SPANS_PER_CARD];     /* span-relative sync sources */


Credit for discovering this issue goes to Eugene Teo.

Please fix this issue.

Regards, Jan.
--
Jan iankko Lieskovsky / Red Hat Security Response Team
By: Tzafrir Cohen (tzafrir) 2008-12-19 06:26:13.000-0600

Indeed it should be SPANS_PER_CARD rather than MAX_TOR_CARDS (this whole thing applies to a single card).
By: Digium Subversion (svnbot) 2008-12-19 06:39:27.000-0600

Repository: dahdi
Revision: 5590

U   linux/trunk/drivers/dahdi/tor2.c

------------------------------------------------------------------------
r5590 | tzafrir | 2008-12-19 06:39:26 -0600 (Fri, 19 Dec 2008) | 4 lines

Fix the safety check in tor2 to be for SPANS_PER_CARD

Thanks to Eugene Teo, in a from issue DAHLIN-62 .

------------------------------------------------------------------------

http://svn.digium.com/view/dahdi?view=rev&revision=5590
By: Digium Subversion (svnbot) 2009-01-27 20:54:52.000-0600

Repository: dahdi
Revision: 5865

_U  linux/tags/2.1.0.4/
U   linux/tags/2.1.0.4/drivers/dahdi/dahdi-base.c
U   linux/tags/2.1.0.4/drivers/dahdi/dahdi_dynamic.c
U   linux/tags/2.1.0.4/drivers/dahdi/tor2.c
U   linux/tags/2.1.0.4/drivers/dahdi/wct1xxp.c
U   linux/tags/2.1.0.4/drivers/dahdi/wct4xxp/base.c
U   linux/tags/2.1.0.4/drivers/dahdi/wcte11xp.c
U   linux/tags/2.1.0.4/drivers/dahdi/wcte12xp/base.c
U   linux/tags/2.1.0.4/include/dahdi/kernel.h

------------------------------------------------------------------------
r5865 | sruffell | 2009-01-27 20:54:51 -0600 (Tue, 27 Jan 2009) | 28 lines

Merged revisions 5590,5811,5819 via svnmerge from
https://origsvn.digium.com/svn/dahdi/linux/trunk

........
r5590 | tzafrir | 2008-12-19 04:39:31 -0800 (Fri, 19 Dec 2008) | 4 lines

Fix the safety check in tor2 to be for SPANS_PER_CARD

Thanks to Eugene Teo, in a from issue DAHLIN-62 .

........
r5811 | sruffell | 2009-01-25 23:19:47 -0800 (Sun, 25 Jan 2009) | 7 lines

Ensure the channel is in a good state before placing it on the chans arrays.
Also ensure that dahdi_receive holds the chan_lock while iterating over the
chans array to prevent channels from entering or leaving the array while the
interrupt handler is running.

Related to issue DAHLIN-71 .

........
r5819 | sruffell | 2009-01-26 11:44:36 -0800 (Mon, 26 Jan 2009) | 3 lines

Manipulate the REGISTERED flag with atomic bitops now since the bit is set
outside the protection of any locks.

........

------------------------------------------------------------------------

http://svn.digium.com/view/dahdi?view=rev&revision=5865
By: Tzafrir Cohen (tzafrir) 2009-02-08 06:07:02.000-0600

The potential memory writing issue has already been resolved.

For further discussion of the incoherent handling of 'timing', see issue DAHLIN-25 .