[asterisk-bugs] [DAHDI-linux 0013954]: [patch] incoherent handling of span timing and lack of input validation

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=13954 
====================================================================== 
Reported By:                tzafrir
Assigned To:                tzafrir
====================================================================== 
Project:                    DAHDI-linux
Issue ID:                   13954
Category:                   General
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     confirmed
====================================================================== 
Date Submitted:             2008-11-23 05:42 CST
Last Modified:              2008-12-19 05:14 CST
====================================================================== 
Summary:                    [patch] incoherent handling of span timing and lack
of input validation
Description: 
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?
====================================================================== 

---------------------------------------------------------------------- 
 (0096700) iankko (reporter) - 2008-12-19 05:14
 http://bugs.digium.com/view.php?id=13954#c96700 
---------------------------------------------------------------------- 
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 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2008-12-19 05:14 iankko         Note Added: 0096700                          
======================================================================