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 <[EMAIL PROTECTED]>
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)
signature.asc
Description: Digital signature
