Loading ...
Sorry, an error occurred while loading the content.
 

[patch] vim memory corruption and potential exploit on swapfile recovery

Expand Messages
  • Zephaniah E. Hull
    I tried posting about this a few days ago, but the message vanished without a bounce. Since then I noticed that it has security implications as well. This took
    Message 1 of 3 , Apr 19, 2007
      I tried posting about this a few days ago, but the message vanished
      without a bounce.

      Since then I noticed that it has security implications as well.

      This took a while to find, and I'll put crash reproduction directions at
      the bottom. But first a description of what's broken, why it's broken,
      and some ways to fix it.


      The bug, simply put, is that if we ever try to recover a swapfile that
      has a page size larger then MIN_SWAP_PAGE_SIZE[0], and for any reason
      mf_recover decides that we've used enough memory and should try to reuse
      some pages[1], we try to read a swapfile page size chunk into a
      MIN_SWAP_PAGE_SIZE buffer[2].

      Since the rules on mf_recover are quite possible to determine from
      source and knowledge of the system[3], and the swapfile defines it's own
      page size[4], this allows anyone who can trick someone into recovering a
      specially crafted swapfile to cause them to read an arbitrary number of
      attacker controlled bytes into a MIN_SWAP_PAGE_SIZE buffer[2]. Which
      looks like a potential security hole to me. (Even if it is bloody hard
      to setup the conditions for it.)


      I can think of three fixes for this, 1 is good if we plan on allowing
      page_size changes, 2 I'm not sure I can think of a justification for, 3
      is non-invasive, easy, and there's a fully tested patch attached that
      implements it, it's also a kludge, but.


      Zephaniah E. Hull.


      1: Replace block_hdr->bh_page_count with bh_size, always check against
      that.

      2: Add a block_hdr->bh_page_size.

      3: In ml_recover don't do a mf_put on the initial header block until we
      are done parsing the swapfile in it's entirety.



      0: MIN_SWAP_PAGE_SIZE is defined to be 1024 bytes. ml_recover in
      memline.c has the following comment and line:
      ---
      /*
      * The page size set in mf_open() might be different from the page size
      * used in the swap file, we must get it from block 0. But to read block
      * 0 we need a page size. Use the minimal size for block 0 here, it will
      * be set to the real value below.
      */
      mfp->mf_page_size = MIN_SWAP_PAGE_SIZE;
      ---

      1:
      mf_release in memfile.c has the following:
      ---
      /*
      * Need to release a block if the number of blocks for this memfile is
      * higher than the maximum or total memory used is over 'maxmemtot'
      */
      need_release = ((mfp->mf_used_count >= mfp->mf_used_count_max)
      || (total_mem_used >> 10) >= (long_u)p_mmt);
      ---
      On the system where this showed up, p_mmt ended up as 62, however a
      fairly large swapfile should be able to trigger this as well on most
      systems.

      2:
      First, mf_release checks to see if the released page has a buffer of the
      right size, but only by checking page_count, since page_count is
      identical, this falsely assumes that the buffer is of the proper size.
      mf_release in memfile.c
      ---
      /*
      * If a bhdr_T is returned, make sure that the page_count of bh_data is
      * right
      */
      if (hp->bh_page_count != page_count)
      { ... }
      return hp;
      ---
      Then when that gets returned, fairly quickly it's passed to mf_read
      in memfile.c which, well:
      ---
      page_size = mfp->mf_page_size;
      offset = (off_t)page_size * hp->bh_bnum;
      size = page_size * hp->bh_page_count;
      ...
      if ((unsigned)vim_read(mfp->mf_fd, hp->bh_data, size) != size)
      ---
      And, boom.


      3: See option.c for the maxmemtot and maxmem variable defaults, it
      wanders through platform specific code, but it's not that hard to get a
      real good guess at what the values will be.

      4: Mentioned in the comment of [0], ml_recover pulls the page size out
      of the swapfile header as a signed 32bit long.

      --
      1024D/E65A7801 Zephaniah E. Hull <warp@...>
      92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
      CCs of replies from mailing lists are requested.

      lba32 requires bios support - which is common, and working bios support
      which is slightly less common on older boxes
      -- Alan Cox
    • Bram Moolenaar
      ... Thanks for catching this. I think the security implications are minimal, since it depends on the amount of memory someone has available. Also, recovering
      Message 2 of 3 , Apr 19, 2007
        Zephaniah Hull wrote:

        > I tried posting about this a few days ago, but the message vanished
        > without a bounce.
        >
        > Since then I noticed that it has security implications as well.
        >
        > This took a while to find, and I'll put crash reproduction directions at
        > the bottom. But first a description of what's broken, why it's broken,
        > and some ways to fix it.
        >
        >
        > The bug, simply put, is that if we ever try to recover a swapfile that
        > has a page size larger then MIN_SWAP_PAGE_SIZE[0], and for any reason
        > mf_recover decides that we've used enough memory and should try to reuse
        > some pages[1], we try to read a swapfile page size chunk into a
        > MIN_SWAP_PAGE_SIZE buffer[2].
        >
        > Since the rules on mf_recover are quite possible to determine from
        > source and knowledge of the system[3], and the swapfile defines it's own
        > page size[4], this allows anyone who can trick someone into recovering a
        > specially crafted swapfile to cause them to read an arbitrary number of
        > attacker controlled bytes into a MIN_SWAP_PAGE_SIZE buffer[2]. Which
        > looks like a potential security hole to me. (Even if it is bloody hard
        > to setup the conditions for it.)
        >
        >
        > I can think of three fixes for this, 1 is good if we plan on allowing
        > page_size changes, 2 I'm not sure I can think of a justification for, 3
        > is non-invasive, easy, and there's a fully tested patch attached that
        > implements it, it's also a kludge, but.

        Thanks for catching this. I think the security implications are
        minimal, since it depends on the amount of memory someone has available.
        Also, recovering from a swap file that you get from someone else is very
        unusual. But someone could make a swapfile that causes Vim to crash.

        > 1: Replace block_hdr->bh_page_count with bh_size, always check against
        > that.

        Effectively means that this block will never be reused, since the size
        doesn't match the page size.

        > 2: Add a block_hdr->bh_page_size.

        Same.

        > 3: In ml_recover don't do a mf_put on the initial header block until we
        > are done parsing the swapfile in it's entirety.

        Works, but wastes a bit of memory.

        Besides the solutions you suggest, it's also possible to reallocate the
        memory when the page size is changed. I think that is a cleaner
        solution, because it's local.

        I think it's also a good idea to check that the page size as specified
        in block 0 is at least the minimal block size.

        Have a look at this patch:

        *** ../../vim-7.0.224/src/memline.c Tue Mar 6 20:27:03 2007
        --- memline.c Thu Apr 19 16:10:39 2007
        ***************
        *** 1015,1032 ****
        --- 1015,1053 ----
        msg_end();
        goto theend;
        }
        +
        /*
        * If we guessed the wrong page size, we have to recalculate the
        * highest block number in the file.
        */
        if (mfp->mf_page_size != (unsigned)char_to_long(b0p->b0_page_size))
        {
        + unsigned previous_page_size = mfp->mf_page_size;
        +
        mf_new_page_size(mfp, (unsigned)char_to_long(b0p->b0_page_size));
        + if (mfp->mf_page_size < previous_page_size)
        + {
        + msg_start();
        + msg_outtrans_attr(mfp->mf_fname, attr | MSG_HIST);
        + MSG_PUTS_ATTR(_(" has been damaged (page size is smaller than minimum value).\n"),
        + attr | MSG_HIST);
        + msg_end();
        + goto theend;
        + }
        if ((size = lseek(mfp->mf_fd, (off_t)0L, SEEK_END)) <= 0)
        mfp->mf_blocknr_max = 0; /* no file or empty file */
        else
        mfp->mf_blocknr_max = (blocknr_T)(size / mfp->mf_page_size);
        mfp->mf_infile_count = mfp->mf_blocknr_max;
        +
        + /* need to reallocate the memory used to store the data */
        + p = alloc(mfp->mf_page_size);
        + if (p == NULL)
        + goto theend;
        + mch_memmove(p, hp->bh_data, previous_page_size);
        + vim_free(hp->bh_data);
        + hp->bh_data = p;
        + b0p = (ZERO_BL *)(hp->bh_data);
        }

        /*
        ***************
        *** 2549,2555 ****
        if (lineadd)
        {
        --(buf->b_ml.ml_stack_top);
        ! /* fix line count for rest of blocks in the stack */
        ml_lineadd(buf, lineadd);
        /* fix stack itself */
        buf->b_ml.ml_stack[buf->b_ml.ml_stack_top].ip_high +=
        --- 2570,2576 ----
        if (lineadd)
        {
        --(buf->b_ml.ml_stack_top);
        ! /* fix line count for rest of blocks in the stack */
        ml_lineadd(buf, lineadd);
        /* fix stack itself */
        buf->b_ml.ml_stack[buf->b_ml.ml_stack_top].ip_high +=
        ***************
        *** 2852,2863 ****
        mf_put(mfp, hp, TRUE, FALSE);

        buf->b_ml.ml_stack_top = stack_idx; /* truncate stack */
        ! /* fix line count for rest of blocks in the stack */
        ! if (buf->b_ml.ml_locked_lineadd)
        {
        ml_lineadd(buf, buf->b_ml.ml_locked_lineadd);
        buf->b_ml.ml_stack[buf->b_ml.ml_stack_top].ip_high +=
        ! buf->b_ml.ml_locked_lineadd;
        }
        ++(buf->b_ml.ml_stack_top);

        --- 2873,2884 ----
        mf_put(mfp, hp, TRUE, FALSE);

        buf->b_ml.ml_stack_top = stack_idx; /* truncate stack */
        ! /* fix line count for rest of blocks in the stack */
        ! if (buf->b_ml.ml_locked_lineadd != 0)
        {
        ml_lineadd(buf, buf->b_ml.ml_locked_lineadd);
        buf->b_ml.ml_stack[buf->b_ml.ml_stack_top].ip_high +=
        ! buf->b_ml.ml_locked_lineadd;
        }
        ++(buf->b_ml.ml_stack_top);

        ***************
        *** 3187,3193 ****
        * The stack is updated to lead to the locked block. The ip_high field in
        * the stack is updated to reflect the last line in the block AFTER the
        * insert or delete, also if the pointer block has not been updated yet. But
        ! * if if ml_locked != NULL ml_locked_lineadd must be added to ip_high.
        *
        * return: NULL for failure, pointer to block header otherwise
        */
        --- 3208,3214 ----
        * The stack is updated to lead to the locked block. The ip_high field in
        * the stack is updated to reflect the last line in the block AFTER the
        * insert or delete, also if the pointer block has not been updated yet. But
        ! * if ml_locked != NULL ml_locked_lineadd must be added to ip_high.
        *
        * return: NULL for failure, pointer to block header otherwise
        */
        ***************
        *** 3244,3254 ****
        buf->b_ml.ml_flags & ML_LOCKED_POS);
        buf->b_ml.ml_locked = NULL;

        ! /*
        ! * if lines have been added or deleted in the locked block, need to
        ! * update the line count in pointer blocks
        ! */
        ! if (buf->b_ml.ml_locked_lineadd)
        ml_lineadd(buf, buf->b_ml.ml_locked_lineadd);
        }

        --- 3265,3275 ----
        buf->b_ml.ml_flags & ML_LOCKED_POS);
        buf->b_ml.ml_locked = NULL;

        ! /*
        ! * If lines have been added or deleted in the locked block, need to
        ! * update the line count in pointer blocks.
        ! */
        ! if (buf->b_ml.ml_locked_lineadd != 0)
        ml_lineadd(buf, buf->b_ml.ml_locked_lineadd);
        }



        --
        If you had to identify, in one word, the reason why the
        human race has not achieved, and never will achieve, its
        full potential, that word would be "meetings."

        /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
        /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
        \\\ download, build and distribute -- http://www.A-A-P.org ///
        \\\ help me help AIDS victims -- http://ICCF-Holland.org ///
      • Zephaniah E. Hull
        ... Agreed on the security implications, but note that on some unix systems it actually depends on the user s ulimit, not the amount available when they run,
        Message 3 of 3 , Apr 19, 2007
          On Thu, Apr 19, 2007 at 04:20:12PM +0200, Bram Moolenaar wrote:
          >
          > Zephaniah Hull wrote:
          >
          > > I tried posting about this a few days ago, but the message vanished
          > > without a bounce.
          > >
          > > Since then I noticed that it has security implications as well.
          > >
          > > This took a while to find, and I'll put crash reproduction directions at
          > > the bottom. But first a description of what's broken, why it's broken,
          > > and some ways to fix it.
          > >
          > >
          > > The bug, simply put, is that if we ever try to recover a swapfile that
          > > has a page size larger then MIN_SWAP_PAGE_SIZE[0], and for any reason
          > > mf_recover decides that we've used enough memory and should try to reuse
          > > some pages[1], we try to read a swapfile page size chunk into a
          > > MIN_SWAP_PAGE_SIZE buffer[2].
          > >
          > > Since the rules on mf_recover are quite possible to determine from
          > > source and knowledge of the system[3], and the swapfile defines it's own
          > > page size[4], this allows anyone who can trick someone into recovering a
          > > specially crafted swapfile to cause them to read an arbitrary number of
          > > attacker controlled bytes into a MIN_SWAP_PAGE_SIZE buffer[2]. Which
          > > looks like a potential security hole to me. (Even if it is bloody hard
          > > to setup the conditions for it.)
          > >
          > >
          > > I can think of three fixes for this, 1 is good if we plan on allowing
          > > page_size changes, 2 I'm not sure I can think of a justification for, 3
          > > is non-invasive, easy, and there's a fully tested patch attached that
          > > implements it, it's also a kludge, but.
          >
          > Thanks for catching this. I think the security implications are
          > minimal, since it depends on the amount of memory someone has available.
          > Also, recovering from a swap file that you get from someone else is very
          > unusual. But someone could make a swapfile that causes Vim to crash.

          Agreed on the security implications, but note that on some unix systems
          it actually depends on the user's ulimit, not the amount available when
          they run, which may be more predictable.

          As far as making a swapfile that causes Vim to crash, a family member
          has trouble with the majority of her swapfile after a crash with this
          one. :)
          >
          > > 1: Replace block_hdr->bh_page_count with bh_size, always check against
          > > that.
          >
          > Effectively means that this block will never be reused, since the size
          > doesn't match the page size.

          Not quite, if the page_count differs, it frees the buffer and
          reallocates it, and reuses the header. Same would apply if we replaced
          it with size which was page_size * page_count.
          >
          > > 2: Add a block_hdr->bh_page_size.
          >
          > Same.
          >
          > > 3: In ml_recover don't do a mf_put on the initial header block until we
          > > are done parsing the swapfile in it's entirety.
          >
          > Works, but wastes a bit of memory.
          >
          > Besides the solutions you suggest, it's also possible to reallocate the
          > memory when the page size is changed. I think that is a cleaner
          > solution, because it's local.
          >
          > I think it's also a good idea to check that the page size as specified
          > in block 0 is at least the minimal block size.
          >
          > Have a look at this patch:

          The parts I can read look like a good fix, and I'll try applying and
          testing it, but any chance of a unified diff? :)

          Zephaniah E. Hull.

          --
          1024D/E65A7801 Zephaniah E. Hull <warp@...>
          92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
          CCs of replies from mailing lists are requested.

          "I would rather spend 10 hours reading someone else's source code than
          10 minutes listening to Musak waiting for technical support which
          isn't."
          (By Dr. Greg Wettstein, Roger Maris Cancer Center)
        Your message has been successfully submitted and would be delivered to recipients shortly.