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 <[EMAIL PROTECTED]>
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
diff -ur tmp/vim-7.0/vim/src/memline.c vim-7.0/vim/src/memline.c
--- tmp/vim-7.0/vim/src/memline.c 2007-04-16 08:34:01.000000000 -0400
+++ vim-7.0/vim/src/memline.c 2007-04-16 08:00:58.000000000 -0400
@@ -836,7 +836,7 @@
buf_T *buf = NULL;
memfile_T *mfp = NULL;
char_u *fname;
- bhdr_T *hp = NULL;
+ bhdr_T *hp = NULL, *hp0 = NULL;
ZERO_BL *b0p;
int b0_ff;
char_u *b0_fenc = NULL;
@@ -968,7 +968,7 @@
/*
* try to read block 0
*/
- if ((hp = mf_get(mfp, (blocknr_T)0, 1)) == NULL)
+ if ((hp0 = mf_get(mfp, (blocknr_T)0, 1)) == NULL)
{
msg_start();
MSG_PUTS_ATTR(_("Unable to read block 0 from "), attr | MSG_HIST);
@@ -979,7 +979,7 @@
msg_end();
goto theend;
}
- b0p = (ZERO_BL *)(hp->bh_data);
+ b0p = (ZERO_BL *)(hp0->bh_data);
if (STRNCMP(b0p->b0_version, "VIM 3.0", 7) == 0)
{
msg_start();
@@ -1073,9 +1073,6 @@
b0_fenc = vim_strnsave(p, (int)(b0p->b0_fname + B0_FNAME_SIZE - p));
}
- mf_put(mfp, hp, FALSE, FALSE); /* release block 0 */
- hp = NULL;
-
/*
* Now that we are sure that the file is going to be recovered, clear the
* contents of the current buffer.
@@ -1327,6 +1324,16 @@
{
if (hp != NULL)
mf_put(mfp, hp, FALSE, FALSE);
+
+ /*
+ * XXX: Due to potential page size changes, this MUST not be done while
+ * we are still parsing the swapfile.
+ */
+
+ if (hp0 != NULL)
+ mf_put(mfp, hp0, FALSE, FALSE); /* release block 0 */
+ hp0 = NULL;
+
mf_close(mfp, FALSE); /* will also vim_free(mfp->mf_fname) */
}
vim_free(buf);
signature.asc
Description: Digital signature
