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 -- [EMAIL PROTECTED] -- 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 ///
