Re: [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()

From: Ian Jackson <ian.jackson@eu.citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	WeiLiu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Xen-devel <xen-devel@lists.xen.org>,
	Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH] libelf: Fix div0 issues in elf_{shdr, phdr}_count()
Date: Thu, 8 Dec 2016 15:47:40 +0000	[thread overview]
Message-ID: <22601.32924.967352.367426@mariner.uk.xensource.com> (raw)
In-Reply-To: <1ca334cd-cda0-e870-b213-72a82447fe37@citrix.com>

Andrew Cooper writes ("Re: [PATCH] libelf: Fix div0 issues in elf_{shdr,phdr}_count()"):
> On 08/12/16 15:17, Jan Beulich wrote:
> > Oh, I see - elf_phdr_count() itself relies on the check that's
> > about to be done. But I think the correct thing then would be to
> > not use elf_phdr_count() here, but read the raw field instead.
> > You patch basically adds a weaker check there which is then
> > immediately being followed by the stronger check above.

There must be no "reading of raw fields".  You may use
elf_access_unsigned if you really want to.

> > And looking at it from that angle it's then questionable why
> > elf_{p,s}hdr_count() do any checking at all - this should all be
> > done once in elf_init(). IOW I did the adjustment in the wrong
> > way, and I really should have made elf_shdr_count() match
> > elf_phdr_count() (moving the checks into elf_init()).

The point of this checking is not to avoid overflow, but just to
ensure that the loops which rely on elf_*_count do not iterate "far
too much".

> > And looking at elf_init() again I also realize that I blindly
> > copied the range checks, calculation of which can overflow.

These possibly-faulty range checks are harmless because they all
operate on unsigned values.

Frankly I'm not sure what the point of a01b6d46 was ?

> > I think it would be better to do those checks such that
> > overflow results in an error return.

The design principle behind my fixes to XSA-55 is to write the bulk of
libelf in a subset of C which is always safe.

This means:

 * Always using unsigned integers (so no signed integer overflow).

 * Doing all pointer dereferences into the ELF block using an
   access function which does a bounds check.  (And this also
   means representing all pointers as unsigned offsets, so that we
   avoid calculating basilisk pointer values.)

 * Explicitly limiting the iteration count of every loop where
   necessayr (to avoid DOS attacks from bad images).

 * Not using unsafe operations like division by input-controlled
   values.

Sticking to these rules is a lot easier than writing explicit checks.
This is because these explicit checks are very easy to omit, or get
wrong.  The pointers are all encapsulated in a special type which
prevents uncontrolled dereference.

Admittedly the rule about iteration count is not so easy to remember,
and I had failed to anticipate that someone would introduce division.
But both of those kind of bugs are denial of service, rather than
privilege escalation.

Having stared at the commit message of a01b6d46 and at the
pre-a01b6d46 code, I still don't understand what motivated the
changes.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel