DAHDI-Linux
  1. DAHDI-Linux
  2. DAHLIN-62

[patch] incoherent handling of span timing and lack of input validation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Target Release Version/s: None
    • Component/s: General
    • Labels:
      None
    • Mantis ID:
      13954
    • Regression:
      No

      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?

      1. 01-wcte11xp-timing.diff
        0.5 kB
        Shaun Ruffell
      2. tor2_timing_validation.diff
        0.6 kB
        Tzafrir Cohen
      3. torisa_timing_validation.diff
        0.5 kB
        Tzafrir Cohen

        Activity

        Hide
        Jan Lieskovsky added a comment -

        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

        Show
        Jan Lieskovsky added a comment - 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
        Hide
        Tzafrir Cohen added a comment -

        Indeed it should be SPANS_PER_CARD rather than MAX_TOR_CARDS (this whole thing applies to a single card).

        Show
        Tzafrir Cohen added a comment - Indeed it should be SPANS_PER_CARD rather than MAX_TOR_CARDS (this whole thing applies to a single card).
        Hide
        Digium Subversion added a comment -

        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

        Show
        Digium Subversion added a comment - 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
        Hide
        Digium Subversion added a comment -

        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

        Show
        Digium Subversion added a comment - 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
        Hide
        Tzafrir Cohen added a comment -

        The potential memory writing issue has already been resolved.

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

        Show
        Tzafrir Cohen added a comment - The potential memory writing issue has already been resolved. For further discussion of the incoherent handling of 'timing', see issue DAHLIN-25 .

          People

          • Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development