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

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

[Andrew Cooper]

elf_uval() can return zero either because the field itself is zero, or because
the access is out of bounds.

c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
as e_{ph,sh}entsize are not checked for sanity before being used to divide
elf->size.

Spotted by Coverity.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>

I experimented with making elf_access_unsigned() __must_check, but this didn't
cause a compiler error.  I am not quite sure why.
---
 xen/common/libelf/libelf-tools.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index bf661e7..f62d9c3 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
 unsigned elf_shdr_count(struct elf_binary *elf)
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
     uint64_t max;
 
     if ( !count )
         return 0;
-    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
+    if ( !entsize )
+    {
+        elf_mark_broken(elf, "e_shentsize is zero");
+        return 0;
+    }
+    max = elf->size / entsize;
     if ( max > UINT_MAX )
         max = UINT_MAX;
     if ( count > max )
@@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
 unsigned elf_phdr_count(struct elf_binary *elf)
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
     uint64_t max;
 
     if ( !count )
         return 0;
-    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+    if ( !entsize )
+    {
+        elf_mark_broken(elf, "e_phentsize is zero");
+        return 0;
+    }
+    max = elf->size / entsize;
     if ( max > UINT_MAX )
         max = UINT_MAX;
     if ( count > max )
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 08.12.16 at 15:18, <andrew.cooper3@citrix.com> wrote:
> elf_uval() can return zero either because the field itself is zero, or because
> the access is out of bounds.
> 
> c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
> as e_{ph,sh}entsize are not checked for sanity before being used to divide
> elf->size.
> 
> Spotted by Coverity.

And wrongly so, imo.

> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
>  unsigned elf_shdr_count(struct elf_binary *elf)
>  {
>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
>      uint64_t max;
>  
>      if ( !count )
>          return 0;
> -    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
> +    if ( !entsize )
> +    {
> +        elf_mark_broken(elf, "e_shentsize is zero");
> +        return 0;
> +    }

This as well as ...

> @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
>  unsigned elf_phdr_count(struct elf_binary *elf)
>  {
>      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
>      uint64_t max;
>  
>      if ( !count )
>          return 0;
> -    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
> +    if ( !entsize )
> +    {
> +        elf_mark_broken(elf, "e_phentsize is zero");
> +        return 0;
> +    }

... this would end up being dead code, due to the checks the same
patch you refer to introduced in elf_init().

Jan


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

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

[Andrew Cooper]

On 08/12/16 14:41, Jan Beulich wrote:
>>>> On 08.12.16 at 15:18, <andrew.cooper3@citrix.com> wrote:
>> elf_uval() can return zero either because the field itself is zero, or because
>> the access is out of bounds.
>>
>> c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 issues
>> as e_{ph,sh}entsize are not checked for sanity before being used to divide
>> elf->size.
>>
>> Spotted by Coverity.
> And wrongly so, imo.
>
>> --- a/xen/common/libelf/libelf-tools.c
>> +++ b/xen/common/libelf/libelf-tools.c
>> @@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
>>  unsigned elf_shdr_count(struct elf_binary *elf)
>>  {
>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
>>      uint64_t max;
>>  
>>      if ( !count )
>>          return 0;
>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
>> +    if ( !entsize )
>> +    {
>> +        elf_mark_broken(elf, "e_shentsize is zero");
>> +        return 0;
>> +    }
> This as well as ...
>
>> @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
>>  unsigned elf_phdr_count(struct elf_binary *elf)
>>  {
>>      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
>>      uint64_t max;
>>  
>>      if ( !count )
>>          return 0;
>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
>> +    if ( !entsize )
>> +    {
>> +        elf_mark_broken(elf, "e_phentsize is zero");
>> +        return 0;
>> +    }
> ... this would end up being dead code, due to the checks the same
> patch you refer to introduced in elf_init().

Are you sure?  elf_init() currently looks like this:

    /* Sanity check phdr if present. */
    count = elf_phdr_count(elf);
    if ( count )
    {
        if ( elf_uval(elf, elf->ehdr, e_phentsize) <
             elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
        {
            elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
                    elf_uval(elf, elf->ehdr, e_phentsize));
            return -1;
        }

There is no check of e_phentsize before it is used to divide with.

~Andrew

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

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in elf_{shdr,phdr}_count()"):
> On 08.12.16 at 15:18, <andrew.cooper3@citrix.com> wrote:
> > Spotted by Coverity.
> 
> And wrongly so, imo.
...
> > @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
> >  unsigned elf_phdr_count(struct elf_binary *elf)
> >  {
> >      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
> > +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
...
> ... this would end up being dead code, due to the checks the same
> patch you refer to introduced in elf_init().

I think I would prefer code that is obviously correct from local
inspection.  There is no performance implication here.

Maybe what we want is elf_divide() which implments anything/0 as 0.

Ian.

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

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

[Jan Beulich]

>>> On 08.12.16 at 15:46, <andrew.cooper3@citrix.com> wrote:
> On 08/12/16 14:41, Jan Beulich wrote:
>>>>> On 08.12.16 at 15:18, <andrew.cooper3@citrix.com> wrote:
>>> elf_uval() can return zero either because the field itself is zero, or 
> because
>>> the access is out of bounds.
>>>
>>> c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 
> issues
>>> as e_{ph,sh}entsize are not checked for sanity before being used to divide
>>> elf->size.
>>>
>>> Spotted by Coverity.
>> And wrongly so, imo.
>>
>>> --- a/xen/common/libelf/libelf-tools.c
>>> +++ b/xen/common/libelf/libelf-tools.c
>>> @@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t 
> addr)
>>>  unsigned elf_shdr_count(struct elf_binary *elf)
>>>  {
>>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
>>>      uint64_t max;
>>>  
>>>      if ( !count )
>>>          return 0;
>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
>>> +    if ( !entsize )
>>> +    {
>>> +        elf_mark_broken(elf, "e_shentsize is zero");
>>> +        return 0;
>>> +    }
>> This as well as ...
>>
>>> @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
>>>  unsigned elf_phdr_count(struct elf_binary *elf)
>>>  {
>>>      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
>>>      uint64_t max;
>>>  
>>>      if ( !count )
>>>          return 0;
>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
>>> +    if ( !entsize )
>>> +    {
>>> +        elf_mark_broken(elf, "e_phentsize is zero");
>>> +        return 0;
>>> +    }
>> ... this would end up being dead code, due to the checks the same
>> patch you refer to introduced in elf_init().
> 
> Are you sure?  elf_init() currently looks like this:
> 
>     /* Sanity check phdr if present. */
>     count = elf_phdr_count(elf);
>     if ( count )
>     {
>         if ( elf_uval(elf, elf->ehdr, e_phentsize) <
>              elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
>         {
>             elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
>                     elf_uval(elf, elf->ehdr, e_phentsize));
>             return -1;
>         }
> 
> There is no check of e_phentsize before it is used to divide with.

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.

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()).

And looking at elf_init() again I also realize that I blindly
copied the range checks, calculation of which can overflow.
I think it would be better to do those checks such that
overflow results in an error return.

I wonder if it wouldn't be better to revert that change, for
me to produce a better v2.

Jan


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

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

[Andrew Cooper]

On 08/12/16 15:17, Jan Beulich wrote:
>>>> On 08.12.16 at 15:46, <andrew.cooper3@citrix.com> wrote:
>> On 08/12/16 14:41, Jan Beulich wrote:
>>>>>> On 08.12.16 at 15:18, <andrew.cooper3@citrix.com> wrote:
>>>> elf_uval() can return zero either because the field itself is zero, or 
>> because
>>>> the access is out of bounds.
>>>>
>>>> c/s a01b6d4 "libelf: treat phdr and shdr similarly" introduced two div0 
>> issues
>>>> as e_{ph,sh}entsize are not checked for sanity before being used to divide
>>>> elf->size.
>>>>
>>>> Spotted by Coverity.
>>> And wrongly so, imo.
>>>
>>>> --- a/xen/common/libelf/libelf-tools.c
>>>> +++ b/xen/common/libelf/libelf-tools.c
>>>> @@ -130,11 +130,17 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t 
>> addr)
>>>>  unsigned elf_shdr_count(struct elf_binary *elf)
>>>>  {
>>>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_shentsize);
>>>>      uint64_t max;
>>>>  
>>>>      if ( !count )
>>>>          return 0;
>>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
>>>> +    if ( !entsize )
>>>> +    {
>>>> +        elf_mark_broken(elf, "e_shentsize is zero");
>>>> +        return 0;
>>>> +    }
>>> This as well as ...
>>>
>>>> @@ -148,11 +154,17 @@ unsigned elf_shdr_count(struct elf_binary *elf)
>>>>  unsigned elf_phdr_count(struct elf_binary *elf)
>>>>  {
>>>>      unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
>>>> +    unsigned entsize = elf_uval(elf, elf->ehdr, e_phentsize);
>>>>      uint64_t max;
>>>>  
>>>>      if ( !count )
>>>>          return 0;
>>>> -    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
>>>> +    if ( !entsize )
>>>> +    {
>>>> +        elf_mark_broken(elf, "e_phentsize is zero");
>>>> +        return 0;
>>>> +    }
>>> ... this would end up being dead code, due to the checks the same
>>> patch you refer to introduced in elf_init().
>> Are you sure?  elf_init() currently looks like this:
>>
>>     /* Sanity check phdr if present. */
>>     count = elf_phdr_count(elf);
>>     if ( count )
>>     {
>>         if ( elf_uval(elf, elf->ehdr, e_phentsize) <
>>              elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
>>         {
>>             elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
>>                     elf_uval(elf, elf->ehdr, e_phentsize));
>>             return -1;
>>         }
>>
>> There is no check of e_phentsize before it is used to divide with.
> 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.
>
> 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()).
>
> And looking at elf_init() again I also realize that I blindly
> copied the range checks, calculation of which can overflow.
> I think it would be better to do those checks such that
> overflow results in an error return.
>
> I wonder if it wouldn't be better to revert that change, for
> me to produce a better v2.

Feel free.

I have to admit that I find it odd that the values aren't sanitised
once, then copied out into local good state.  The repeated use of
elf_uval() risks a lot of zeroes because of its range check case.

~Andrew

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

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

[Ian Jackson]

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

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

[Jan Beulich]

>>> On 08.12.16 at 16:47, <ian.jackson@eu.citrix.com> wrote:
> 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.

Oh, by "raw" I meant using elf_uval() rather than elf_phdr_count().

>> > 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.

Why does operating on unsigned values make overflow harmless?

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

First of all I'd like to refer you to its description, but see below.
Are you suggesting that all those adjustments appear pointless
to you?

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

But using a hard coded but possibly wrong value instead isn't
really a solution.

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

Hmm, that's interesting. Are you then saying all the changes it
did and described are wrong? Pointless? I could do with some
clarification here, as otherwise it's not clear whether spending
time on producing an improved v2 is actually worth it.

Jan


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

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

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] libelf: Fix div0 issues in elf_{shdr,phdr}_count()"):
> 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.

Jan and I had a conversation on irc.


Part of the motivation for a01b6d4 was the anomaly which is the
difference between the implementations of elf_phdr_size and
elf_shdr_size.

This was introduced in 39bf7b9d0ae5 "libelf: check loops for running
away".  That was part of the XSA-55 patchset.

Looking at the commit message I was concerned that the phdr and shdr
counts might be excessive, and that libelf might get stuck in a loop
with an unreasonably high iteration count.

I appear to have attempted to mitigate this by:

 (a) in the case of shdrs, by using elf_access_ok inside each
    loop, on the principle that an out of control loop count
    would walk off the end of the image and stop;

 (b) in the case of phdrs, making an explicit limit of
    (image size) / sizeof(phdr).  NB that sizeof(phdr) is not the
    actual size of phdrs; for a valid ELF it is a minimum.  This
    is fine because we're computing a backstop, which does not
    have to be entirely accurate.

I now see that (a) is ineffective because the image can specify its
own stride for the iteration (the header size).  If it specifies a
stride of zero, the maximum count is unbounded.

However, both count values are actually 16-bit.  So there is already a
limit.  The extra code in elf_phdr_size is not necessary.

IMO this near-miss demonstrates that a more robust approach is
required to bounding loops in libelf.

A possibility would be to introduce
  bool elf_iter_cont(struct elf_binary *elf, size_t copysz);
and using it in every for(;;) in libelf.  It would keep a counter (set
to zero in libelf) and abandon the processing if the total of all the
copysz values passed was "too large".  (copysz would be bounded below
by 1, so that it is safe to pass a size from the ELF.)

Then

-     for ( i = 0; i < elf_shdr_count(elf); i++ )
+     for ( i = 0; elf_iter_cont(elf, e_shentsize)
+          && i < elf_shdr_count(elf); i++ )

An alternative would be to decorate every loop with a comment
explaining why the loop does reasonably-bounded work.  I'm not sure
whether this, or my more sophisticated suggestion, will find favour.

In any case, the max calculation in elf_phdr_size is unnecessary for
security, and has no purpose related to functionality, and can be
deleted.


Another part of the motivation for a01b6d4 is to produce better
messages for certain malformed ELFs.  There has been no assertion that
any such ELFs (ie ELFs which would fail these new checks) actually
exist or have caused trouble for anyone.

I think that it is a bad idea to add unnecessary checking to libelf.

libelf is a format parser on a security boundary.  Despite attempts to
provide a safe dialect to write in, every piece of code in libelf
risks security problems.

If libelf "accepts" some malformed image, and constructs a mess in the
guest memory space, then that is unfortunate.  But it is not a big
problem.  The guest will probably crash or go wrong, but that is not a
problem for the host.

If someone is faced with fallout from a malformed ELF, they would
hopefully try use an object file inspector like objdump on the loaded
image.  That would reveal the problem.  (And of course ELFs are mostly
generated by a fairly small ecosystem of toolchain programs.)


Also, this has demonstrated that the design principles underlying the
safety of libelf are not properly understood.  IIRC they were
explained in the XSA-55 00/ patch, but that's not sufficient.


So I would prefer to:


Revert all of a01b6d4.


Delete the header count check in elf_phdr_count and replace it with a
compile-time assertion, in elf_phdr_count and in elf_shdr_count, that
sizeof the count field is <= 2.  (This may need a new macro
ELF_HANDLE_FIELD_SIZEOF implemented in terms of
ELF__HANDLE_FIELD_TYPE.)


Add a comment near the top of libelf.h explaining the rules for
programming inside libelf, to include:

  * Arithmetic on signed values is forbidden

  * Use of actual pointers into the ELF is forbidden

  * Division by non-constant values is forbidden

  * All loops must ensure that they have a reasonably low
    iteration count

  * Code (including ELF format sanity checks) which is necessary
    neither for safety nor for correct processing of correct files
    should not be added to libelf.  It is not libelf's role to
    be an ELF validator.


Thanks,
Ian.

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

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

[Jan Beulich]

>>> On 08.12.16 at 18:28, <ian.jackson@eu.citrix.com> wrote:
> Part of the motivation for a01b6d4 was the anomaly which is the
> difference between the implementations of elf_phdr_size and
> elf_shdr_size.
> 
> This was introduced in 39bf7b9d0ae5 "libelf: check loops for running
> away".  That was part of the XSA-55 patchset.
> 
> Looking at the commit message I was concerned that the phdr and shdr
> counts might be excessive, and that libelf might get stuck in a loop
> with an unreasonably high iteration count.

Looking at that commit message, what I'm missing first of all is an
explanation of why long loops are a problem in the first place. Do
we need to be concerned of single threaded tool stacks? I think
such a problem would better be addressed at higher layers,
switching to multi threaded designs.

> I appear to have attempted to mitigate this by:
> 
>  (a) in the case of shdrs, by using elf_access_ok inside each
>     loop, on the principle that an out of control loop count
>     would walk off the end of the image and stop;
> 
>  (b) in the case of phdrs, making an explicit limit of
>     (image size) / sizeof(phdr).  NB that sizeof(phdr) is not the
>     actual size of phdrs; for a valid ELF it is a minimum.  This
>     is fine because we're computing a backstop, which does not
>     have to be entirely accurate.
> 
> I now see that (a) is ineffective because the image can specify its
> own stride for the iteration (the header size).  If it specifies a
> stride of zero, the maximum count is unbounded.

Well, a stride of zero is invalid (as is any smaller than the base ELF
spec's entry structure size).

> However, both count values are actually 16-bit.  So there is already a
> limit.  The extra code in elf_phdr_size is not necessary.

I'll make v2 of the patch then simply drop it.

> Another part of the motivation for a01b6d4 is to produce better
> messages for certain malformed ELFs.  There has been no assertion that
> any such ELFs (ie ELFs which would fail these new checks) actually
> exist or have caused trouble for anyone.
> 
> I think that it is a bad idea to add unnecessary checking to libelf.
> 
> libelf is a format parser on a security boundary.  Despite attempts to
> provide a safe dialect to write in, every piece of code in libelf
> risks security problems.

Any crash (e.g. as a result of a signal) of course is a problem also
for a multi threaded tool stack. But beyond that I'm not sure I
understand why you say "every piece". That's perhaps partly
because ...

> Also, this has demonstrated that the design principles underlying the
> safety of libelf are not properly understood.  IIRC they were
> explained in the XSA-55 00/ patch, but that's not sufficient.

... this explanation has not been recorded anywhere afaics, not
even in xsa.git.

> So I would prefer to:
> 
> Revert all of a01b6d4.

That was already done yesterday.

> Delete the header count check in elf_phdr_count and replace it with a
> compile-time assertion, in elf_phdr_count and in elf_shdr_count, that
> sizeof the count field is <= 2.  (This may need a new macro
> ELF_HANDLE_FIELD_SIZEOF implemented in terms of
> ELF__HANDLE_FIELD_TYPE.)

I'll wait with adding such an assertion until we've clarified the point
near the top of this reply.

> Add a comment near the top of libelf.h explaining the rules for
> programming inside libelf, to include:
> 
>   * Arithmetic on signed values is forbidden
> 
>   * Use of actual pointers into the ELF is forbidden
> 
>   * Division by non-constant values is forbidden
> 
>   * All loops must ensure that they have a reasonably low
>     iteration count
> 
>   * Code (including ELF format sanity checks) which is necessary
>     neither for safety nor for correct processing of correct files
>     should not be added to libelf.  It is not libelf's role to
>     be an ELF validator.

For all these points, I'd like it to also be clarified why those are
being established. I'd appreciate if you could submit a respective
patch based on the 00/ patch you refer to above, which I
understand you still have available in your mailbox.

For this last point - "ELF validator" is certainly the goal. But I
think it is reasonable for a library to at least check the input it
makes use of. If I had meant to fully validate the ELF image,
I would e.g. have had to reject images with zero e_phnum but
non-zero other e_ph* fields. Yet we don't care about those
other fields when there are no program headers (which, as
pointed out on IRC, is useless anyway, as then there's nothing
to load; only e_shnum can sensibly be zero).

Jan

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

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

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in elf_{shdr,phdr}_count()"):
> On 08.12.16 at 18:28, <ian.jackson@eu.citrix.com> wrote:
> > Looking at the commit message I was concerned that the phdr and shdr
> > counts might be excessive, and that libelf might get stuck in a loop
> > with an unreasonably high iteration count.
> 
> Looking at that commit message, what I'm missing first of all is an
> explanation of why long loops are a problem in the first place. Do
> we need to be concerned of single threaded tool stacks? I think
> such a problem would better be addressed at higher layers,
> switching to multi threaded designs.

libxl is (essentially) singlethreaded.  It is very hard to write
correct multithreaded programs and I would be adamantly opposed to
anything that made libxl more thready inside.  Also, even with
threading, a long-running domain setup would make it difficult to kill
the domain, unless we have asynchronous termination of such a thread,
which would be a nightmare to implement correctly.

> > I now see that (a) is ineffective because the image can specify its
> > own stride for the iteration (the header size).  If it specifies a
> > stride of zero, the maximum count is unbounded.
> 
> Well, a stride of zero is invalid (as is any smaller than the base ELF
> spec's entry structure size).

That something is invalid does not mean it cannot be specified.

> > libelf is a format parser on a security boundary.  Despite attempts to
> > provide a safe dialect to write in, every piece of code in libelf
> > risks security problems.
> 
> Any crash (e.g. as a result of a signal) of course is a problem also
> for a multi threaded tool stack. But beyond that I'm not sure I
> understand why you say "every piece". That's perhaps partly
> because ...

All code risks bugs.

The programming environment in which libelf code is written is by far
from guaranteed safe.  Safety relies on programmers who write the code
following the rules.  The most common kinds of mistake are rendered
harmless, but other kinds of mistake are possible.

> > Add a comment near the top of libelf.h explaining the rules for
> > programming inside libelf, to include:
> > 
> >   * Arithmetic on signed values is forbidden
> > 
> >   * Use of actual pointers into the ELF is forbidden
> > 
> >   * Division by non-constant values is forbidden
> > 
> >   * All loops must ensure that they have a reasonably low
> >     iteration count
> > 
> >   * Code (including ELF format sanity checks) which is necessary
> >     neither for safety nor for correct processing of correct files
> >     should not be added to libelf.  It is not libelf's role to
> >     be an ELF validator.
> 
> For all these points, I'd like it to also be clarified why those are
> being established. I'd appreciate if you could submit a respective
> patch based on the 00/ patch you refer to above, which I
> understand you still have available in your mailbox.

I will do this.

> For this last point - "ELF validator" is certainly the goal. But I
> think it is reasonable for a library to at least check the input it
> makes use of. If I had meant to fully validate the ELF image,
> I would e.g. have had to reject images with zero e_phnum but
> non-zero other e_ph* fields. Yet we don't care about those
> other fields when there are no program headers (which, as
> pointed out on IRC, is useless anyway, as then there's nothing
> to load; only e_shnum can sensibly be zero).

(I think you omitted "not", so you meant "is certainly not the goal.)

IMO libelf should do enough tests to function correctly with correct
input, and avoid being vulnerable to incorrect input.  Code to perform
other tests introduces risk (of bugs, including security bugs such as
the one introduced in a01b6d4).

Such code has almost zero benefit because 1. we do not expect
ELF-generating tools to generate invalid ELFs so these checks' failure
paths will very likely never be executed anywhere, and 2. anyone
debugging such a hypothetical bad ELF will have a variety of tools
available which will do a much better job of analysing it.


Ian.

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

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

[Jan Beulich]

>>> On 09.12.16 at 12:54, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH] libelf: Fix div0 issues in 
> elf_{shdr,phdr}_count()"):
>> On 08.12.16 at 18:28, <ian.jackson@eu.citrix.com> wrote:
>> > Looking at the commit message I was concerned that the phdr and shdr
>> > counts might be excessive, and that libelf might get stuck in a loop
>> > with an unreasonably high iteration count.
>> 
>> Looking at that commit message, what I'm missing first of all is an
>> explanation of why long loops are a problem in the first place. Do
>> we need to be concerned of single threaded tool stacks? I think
>> such a problem would better be addressed at higher layers,
>> switching to multi threaded designs.
> 
> libxl is (essentially) singlethreaded.  It is very hard to write
> correct multithreaded programs and I would be adamantly opposed to
> anything that made libxl more thready inside.  Also, even with
> threading, a long-running domain setup would make it difficult to kill
> the domain, unless we have asynchronous termination of such a thread,
> which would be a nightmare to implement correctly.

But libxl is only a library - the question is whether xl (or whatever
containing process) may be sitting in on libelf invocation,
preventing the process(es) controlling another domain(s) from
making forward progress.

As to killing the domain - wouldn't killing the xl process doing the
creation followed by an "xl destroy" be sufficient? (As you know,
I don't know very much about how the tool stack works, but this
would seem a pretty natural pair of operations.)

>> > I now see that (a) is ineffective because the image can specify its
>> > own stride for the iteration (the header size).  If it specifies a
>> > stride of zero, the maximum count is unbounded.
>> 
>> Well, a stride of zero is invalid (as is any smaller than the base ELF
>> spec's entry structure size).
> 
> That something is invalid does not mean it cannot be specified.

And I didn't mean to imply that. What I mean to imply is that if
we guarded against too small (and hence invalid) entry size values
(which is one of the things the reverted patch does), we wouldn't
have this problem.

>> For this last point - "ELF validator" is certainly the goal. But I
>> think it is reasonable for a library to at least check the input it
>> makes use of. If I had meant to fully validate the ELF image,
>> I would e.g. have had to reject images with zero e_phnum but
>> non-zero other e_ph* fields. Yet we don't care about those
>> other fields when there are no program headers (which, as
>> pointed out on IRC, is useless anyway, as then there's nothing
>> to load; only e_shnum can sensibly be zero).
> 
> (I think you omitted "not", so you meant "is certainly not the goal.)

Indeed, thanks for noticing and deducing the right thing.

> IMO libelf should do enough tests to function correctly with correct
> input, and avoid being vulnerable to incorrect input.  Code to perform
> other tests introduces risk (of bugs, including security bugs such as
> the one introduced in a01b6d4).
> 
> Such code has almost zero benefit because 1. we do not expect
> ELF-generating tools to generate invalid ELFs so these checks' failure
> paths will very likely never be executed anywhere, and 2. anyone
> debugging such a hypothetical bad ELF will have a variety of tools
> available which will do a much better job of analysing it.

Well, in that case I'll have to teach myself to keep my hands off of
libelf/ as much as possible. But as you can see from the entry size
aspect further up, some extra checks may help (and might even
allow to weaken your "don't allow division by ..." criteria).

Jan


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

[PATCH 0/8] libelf: safety enhancements

[Ian Jackson]

We recently discovered two near-miss in libelf:

* The intended method for limiting the phdr loop iteration count was
  not effective.  But happily this turned not to be important because
  the count field is only 16 bits.

* A recent commit accidentally introduced a division by zero
  vulnerability.

Subsequent discussion revealed that the design principles underlying
libelf's safety were not widely understood - because they were not
documented.

Initially I tried dealing with the loop safety problem by auditing the
code and adding a suitable comment next to each loop, stating a proof
sketch of the loop's safety.  I found that this quickly became
unworkable, because there are nested loops.  These nested loops did
not have badly unreasonable upper bounds but the complexity of the
analysis was unsuitable for security-critical review.

An upper bound on the work done in loops in libelf is necessary
because libelf may be called by the toolstack in a context where it
would block other work.  Specifically, libelf is called by libxl, and
libxl does all of its work within a single per-ctx lock.  libxl's
callers are not supposed to be required to invoke libxl on multiple
ctxs or with multiple processes simultaneously, and in any case
we don't want to generate and leak stuck toolstack processes.

So, in this series, I propose:

 * A new scheme for limiting the work done by libelf.  We track it
   explicitly, and check it on each iteration of every loop.  (This
   replaces a similar ad-hoc scheme used for copying image data.)

 * Documentation which states the safety design principles for libelf,
   and the coding rules which follow from those design principles.

After this series is done there are a few redundant loop safety
checks, from the previous approach:

 * There are a number of ad-hoc limits on string sizes, certain table
   sizes, etc.

 * There are calls to elf_access_ok which were intended to limit loop
   iteration counts (but are ineffective at doing so since the stride
   is controlled by the input image and might be zero).

I have chosen to retain these.  Removing them seems like an
unnecessary risk.  In particular, searching for and removing
certain elf_access_ok calls seems unwise.

Thanks,
Ian.

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

[PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Ian Jackson]

This will allow us to keep track of the total amount of work we are
doing.  When it becomes excessive, we mark the ELF broken, and stop
processing.

This is a more robust way of preventing DoS problems by bad images
than attempting to prove, for each of the (sometimes rather deeply
nested) loops, that the total work is "reasonable".  We bound the
notional work by 4x the image size (plus 1M).

Also introduce elf_strcmp_safe, which unconditionally does the work,
but increments the count so any outer loop may be aborted if
necessary.

Currently there are no callers, so no functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-loader.c | 14 ++++++++++++++
 xen/include/xen/libelf.h          | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index a72cd8a..00479af 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
     ELF_HANDLE_DECL(elf_shdr) shdr;
     unsigned i, count, section, link;
     uint64_t offset;
+    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
 
     if ( !elf_is_elfbinary(image_input, size) )
     {
@@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
     elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
     elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
 
+    elf->iteration_deaccumulator = 1024*1024 +
+        (size > max_size_for_deacc ? max_size_for_deacc : size)
+        * ELF_MAX_ITERATION_FACTOR;        
+
     /* Sanity check phdr. */
     offset = elf_uval(elf, elf->ehdr, e_phoff) +
         elf_uval(elf, elf->ehdr, e_phentsize) * elf_phdr_count(elf);
@@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
     return value;
 }
 
+bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) {
+    if (maxcopysz > elf->iteration_deaccumulator)
+        elf_mark_broken(elf, "excessive iteration - too much work to parse");
+    if (elf->broken)
+        return false;
+    elf->iteration_deaccumulator -= maxcopysz;
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 1b763f3..294231a 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -56,6 +56,8 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
 #define ELF_MAX_STRING_LENGTH 4096
 #define ELF_MAX_TOTAL_NOTE_COUNT 65536
 
+#define ELF_MAX_ITERATION_FACTOR 4
+
 /* ------------------------------------------------------------------------ */
 
 /* Macros for accessing the input image and output area. */
@@ -201,6 +203,9 @@ struct elf_binary {
     uint64_t bsd_symtab_pstart;
     uint64_t bsd_symtab_pend;
 
+    /* private */
+    uint64_t iteration_deaccumulator;
+    
     /*
      * caller's other acceptable destination.
      * Set by elf_set_xdest.  Do not set these directly.
@@ -264,6 +269,14 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, elf_ptrval ptr,
 
 uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
 
+bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t count);
+  /* It is OK for count to be out by a smallish constant factor.
+   * It is OK for count to be 0, as we clamp it to 1, so we
+   * can use lengths or sizes from the image. */
+
+static inline bool elf_iter_ok(struct elf_binary *elf)
+    { return elf_iter_ok_counted(elf,1); }
+
 const char *elf_strval(struct elf_binary *elf, elf_ptrval start);
   /* may return NULL if the string is out of range etc. */
 
@@ -463,6 +476,14 @@ static inline void *elf_memset_unchecked(void *s, int c, size_t n)
    * memcpy, memset and memmove to undefined MISTAKE things.
    */
 
+static inline int elf_strcmp_safe(struct elf_binary *elf,
+                                  const char *a, const char *b) {
+    elf_iter_ok_counted(elf, strlen(b));
+    return strcmp(a,b);
+}
+  /* Unlike other *_safe functions, elf_strcmp_safe is called on
+   * values already extracted from the image (eg by elf_strval),
+   * and fixed constant strings (typically, the latter is "b"). */
 
 /* Advances past amount bytes of the current destination area. */
 static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount)
-- 
2.1.4


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

[PATCH 2/8] libelf: loop safety: Pass `elf' to elf_xen_parse_features

[Ian Jackson]

Not used yet, so no functional change.  We will need this in a moment.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-dominfo.c | 5 +++--
 xen/include/xen/libelf.h           | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index a52900c..7f4a6a0 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -32,7 +32,8 @@ static const char *const elf_xen_feature_names[] = {
 static const unsigned elf_xen_features =
 sizeof(elf_xen_feature_names) / sizeof(elf_xen_feature_names[0]);
 
-elf_errorstatus elf_xen_parse_features(const char *features,
+elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
+                           const char *features,
                            uint32_t *supported,
                            uint32_t *required)
 {
@@ -202,7 +203,7 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
         break;
 
     case XEN_ELFNOTE_FEATURES:
-        if ( elf_xen_parse_features(str, parms->f_supported,
+        if ( elf_xen_parse_features(elf, str, parms->f_supported,
                                     parms->f_required) )
             return -1;
         break;
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 294231a..6436bd7 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -452,7 +452,8 @@ static inline int elf_xen_feature_get(int nr, uint32_t * addr)
     return !!(addr[nr >> 5] & (1 << (nr & 31)));
 }
 
-int elf_xen_parse_features(const char *features,
+int elf_xen_parse_features(struct elf_binary *elf,
+                           const char *features,
                            uint32_t *supported,
                            uint32_t *required);
 int elf_xen_parse_note(struct elf_binary *elf,
-- 
2.1.4


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

[PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Ian Jackson]

In every `for' or `while' loop, either call elf_iter_ok, or explain
why it's not necessary.  This is part of comprehensive defence against
out of control loops.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-dominfo.c | 22 +++++++++++++---------
 xen/common/libelf/libelf-loader.c  |  8 ++++----
 xen/common/libelf/libelf-tools.c   |  6 +++---
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 7f4a6a0..b139e32 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
     if ( features == NULL )
         return 0;
 
-    for ( pos = 0; features[pos] != '\0'; pos += len )
+    for ( pos = 0;
+          elf_iter_ok_counted(elf, sizeof(feature)) &&
+              features[pos] != '\0';
+          pos += len )
     {
         elf_memset_unchecked(feature, 0, sizeof(feature));
-        for ( len = 0;; len++ )
+        for ( len = 0;; len++ ) /* can't do more than sizeof(feature) */
         {
             if ( len >= sizeof(feature)-1 )
                 break;
@@ -60,7 +63,7 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
             feature[len] = features[pos + len];
         }
 
-        for ( i = 0; i < elf_xen_features; i++ )
+        for ( i = 0; elf_iter_ok(elf) && i < elf_xen_features; i++ )
         {
             if ( !elf_xen_feature_names[i] )
                 continue;
@@ -236,7 +239,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
     parms->elf_note_start = start;
     parms->elf_note_end   = end;
     for ( note = ELF_MAKE_HANDLE(elf_note, parms->elf_note_start);
-          ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
+          elf_iter_ok(elf) && ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
           note = elf_note_next(elf, note) )
     {
 #ifdef __XEN__
@@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
 
     h = parms->guest_info;
 #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
-    while ( STAR(h) )
+    while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
+            STAR(h) )
     {
         elf_memset_unchecked(name, 0, sizeof(name));
         elf_memset_unchecked(value, 0, sizeof(value));
-        for ( len = 0;; len++, h++ )
+        for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted above */
         {
             if ( len >= sizeof(name)-1 )
                 break;
@@ -291,7 +295,7 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
             if ( STAR(h) == '=' )
             {
                 h++;
-                for ( len = 0;; len++, h++ )
+                for ( len = 0;; len++, h++ ) /* covered by iter_ok_counted */
                 {
                     if ( len >= sizeof(value)-1 )
                         break;
@@ -504,7 +508,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
 
     /* Find and parse elf notes. */
     count = elf_phdr_count(elf);
-    for ( i = 0; i < count; i++ )
+    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
@@ -537,7 +541,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
     if ( xen_elfnotes == 0 )
     {
         count = elf_shdr_count(elf);
-        for ( i = 1; i < count; i++ )
+        for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
         {
             shdr = elf_shdr_by_index(elf, i);
             if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 00479af..68c9021 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -85,7 +85,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
 
     /* Find symbol table and symbol string table. */
     count = elf_shdr_count(elf);
-    for ( i = 1; i < count; i++ )
+    for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
@@ -425,7 +425,7 @@ do {                                                                \
      * NB: this _must_ be done one by one, and taking the bitness into account,
      * so that the guest can treat this as an array of type Elf{32/64}_Shdr.
      */
-    for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
+    for ( i = 0; elf_iter_ok(elf) && i < ELF_BSDSYM_SECTIONS; i++ )
     {
         rc = elf_load_image(elf, header_base + header_size + shdr_size * i,
                             ELF_REALPTR2PTRVAL(&header.elf_header.section[i]),
@@ -453,7 +453,7 @@ void elf_parse_binary(struct elf_binary *elf)
     unsigned i, count;
 
     count = elf_phdr_count(elf);
-    for ( i = 0; i < count; i++ )
+    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
@@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
     uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
 
     count = elf_phdr_count(elf);
-    for ( i = 0; i < count; i++ )
+    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index a9edb6a..56dab63 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
     ELF_HANDLE_DECL(elf_shdr) shdr;
     const char *sname;
 
-    for ( i = 1; i < count; i++ )
+    for ( i = 1; elf_iter_ok(elf) && i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
@@ -214,7 +214,7 @@ const char *elf_strval(struct elf_binary *elf, elf_ptrval start)
         if ( !elf_access_unsigned(elf, start, length, 1) )
             /* ok */
             return ELF_UNSAFE_PTR(start);
-        if ( length >= ELF_MAX_STRING_LENGTH )
+        if ( !elf_iter_ok(elf) || length >= ELF_MAX_STRING_LENGTH )
         {
             elf_mark_broken(elf, "excessively long string");
             return NULL;
@@ -262,7 +262,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
     uint64_t info, name;
     const char *sym_name;
 
-    for ( ; ptr < end; ptr += elf_size(elf, sym) )
+    for ( ; elf_iter_ok(elf) && ptr < end; ptr += elf_size(elf, sym) )
     {
         sym = ELF_MAKE_HANDLE(elf_sym, ptr);
         info = elf_uval(elf, sym, st_info);
-- 
2.1.4


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

[PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe

[Ian Jackson]

When we use elf_mem*_unsafe, we need to check that we are not doing
too much work.

Ensure that a call to elf_iter_ok_counted is near every call to
elf_mem*_unsafe.

(At one call site, just have a comment instead.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-dominfo.c | 1 +
 xen/common/libelf/libelf-loader.c  | 2 +-
 xen/common/libelf/libelf-tools.c   | 6 ++++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index b139e32..87a47d9 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -498,6 +498,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
     unsigned total_note_count = 0;
 
     elf_memset_unchecked(parms, 0, sizeof(*parms));
+    elf_iter_ok_counted(elf, sizeof(*parms));
     parms->virt_base = UNSET_ADDR;
     parms->virt_entry = UNSET_ADDR;
     parms->virt_hypercall = UNSET_ADDR;
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 68c9021..d5e51d3 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -46,7 +46,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
         return -1;
     }
 
-    elf_memset_unchecked(elf, 0, sizeof(*elf));
+    elf_memset_unchecked(elf, 0, sizeof(*elf)); /* loop safety: singleton */
     elf->image_base = image_input;
     elf->size = size;
     elf->ehdr = ELF_MAKE_HANDLE(elf_ehdr, (elf_ptrval)image_input);
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 56dab63..ab83150 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -69,7 +69,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
                      elf_ptrval src, size_t size)
 {
     if ( elf_access_ok(elf, dst, size) &&
-         elf_access_ok(elf, src, size) )
+         elf_access_ok(elf, src, size) &&
+         elf_iter_ok_counted(elf, size) )
     {
         /* use memmove because these checks do not prove that the
          * regions don't overlap and overlapping regions grant
@@ -80,7 +81,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
 
 void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size)
 {
-    if ( elf_access_ok(elf, dst, size) )
+    if ( elf_access_ok(elf, dst, size) &&
+         elf_iter_ok_counted(elf, size))
     {
         elf_memset_unchecked(ELF_UNSAFE_PTR(dst), c, size);
     }
-- 
2.1.4


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

[PATCH 5/8] libelf: loop safety: Replace all calls to strcmp

[Ian Jackson]

strcmp can do singificant work, and is found in some inner loops where
we search for the meaning of things we find in the image.  We need to
avoid doing too much work.

So replace all calls to strcmp with elf_strcmp_safe.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-dominfo.c | 37 +++++++++++++++++++------------------
 xen/common/libelf/libelf-private.h |  7 ++++---
 xen/common/libelf/libelf-tools.c   |  4 ++--
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 87a47d9..b037d10 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -70,7 +70,8 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
             if ( feature[0] == '!' )
             {
                 /* required */
-                if ( !strcmp(feature + 1, elf_xen_feature_names[i]) )
+                if ( !elf_strcmp_safe(elf, feature + 1,
+                                      elf_xen_feature_names[i]) )
                 {
                     elf_xen_feature_set(i, supported);
                     if ( required )
@@ -81,7 +82,7 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
             else
             {
                 /* supported */
-                if ( !strcmp(feature, elf_xen_feature_names[i]) )
+                if ( !elf_strcmp_safe(elf, feature, elf_xen_feature_names[i]) )
                 {
                     elf_xen_feature_set(i, supported);
                     break;
@@ -173,13 +174,13 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
         safe_strcpy(parms->xen_ver, str);
         break;
     case XEN_ELFNOTE_PAE_MODE:
-        if ( !strcmp(str, "yes") )
+        if ( !elf_strcmp_safe(elf, str, "yes") )
             parms->pae = XEN_PAE_EXTCR3;
         if ( strstr(str, "bimodal") )
             parms->pae = XEN_PAE_BIMODAL;
         break;
     case XEN_ELFNOTE_BSD_SYMTAB:
-        if ( !strcmp(str, "yes") )
+        if ( !elf_strcmp_safe(elf, str, "yes") )
             parms->bsd_symtab = 1;
         break;
 
@@ -255,7 +256,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
         note_name = elf_note_name(elf, note);
         if ( note_name == NULL )
             continue;
-        if ( strcmp(note_name, "Xen") )
+        if ( elf_strcmp_safe(elf, note_name, "Xen") )
             continue;
         if ( elf_xen_parse_note(elf, parms, note) )
             return ELF_NOTE_INVALID;
@@ -315,38 +316,38 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
         elf_msg(elf, "ELF: %s=\"%s\"\n", name, value);
 
         /* strings */
-        if ( !strcmp(name, "LOADER") )
+        if ( !elf_strcmp_safe(elf, name, "LOADER") )
             safe_strcpy(parms->loader, value);
-        if ( !strcmp(name, "GUEST_OS") )
+        if ( !elf_strcmp_safe(elf, name, "GUEST_OS") )
             safe_strcpy(parms->guest_os, value);
-        if ( !strcmp(name, "GUEST_VER") )
+        if ( !elf_strcmp_safe(elf, name, "GUEST_VER") )
             safe_strcpy(parms->guest_ver, value);
-        if ( !strcmp(name, "XEN_VER") )
+        if ( !elf_strcmp_safe(elf, name, "XEN_VER") )
             safe_strcpy(parms->xen_ver, value);
-        if ( !strcmp(name, "PAE") )
+        if ( !elf_strcmp_safe(elf, name, "PAE") )
         {
-            if ( !strcmp(value, "yes[extended-cr3]") )
+            if ( !elf_strcmp_safe(elf, value, "yes[extended-cr3]") )
                 parms->pae = XEN_PAE_EXTCR3;
             else if ( !strncmp(value, "yes", 3) )
                 parms->pae = XEN_PAE_YES;
         }
-        if ( !strcmp(name, "BSD_SYMTAB") )
+        if ( !elf_strcmp_safe(elf, name, "BSD_SYMTAB") )
             parms->bsd_symtab = 1;
 
         /* longs */
-        if ( !strcmp(name, "VIRT_BASE") )
+        if ( !elf_strcmp_safe(elf, name, "VIRT_BASE") )
             parms->virt_base = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "VIRT_ENTRY") )
+        if ( !elf_strcmp_safe(elf, name, "VIRT_ENTRY") )
             parms->virt_entry = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "ELF_PADDR_OFFSET") )
+        if ( !elf_strcmp_safe(elf, name, "ELF_PADDR_OFFSET") )
             parms->elf_paddr_offset = strtoull(value, NULL, 0);
-        if ( !strcmp(name, "HYPERCALL_PAGE") )
+        if ( !elf_strcmp_safe(elf, name, "HYPERCALL_PAGE") )
             parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) +
                 parms->virt_base;
 
         /* other */
-        if ( !strcmp(name, "FEATURES") )
-            if ( elf_xen_parse_features(value, parms->f_supported,
+        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
+            if ( elf_xen_parse_features(elf, value, parms->f_supported,
                                         parms->f_required) )
                 return -1;
     }
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 388c3da..082c572 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -98,9 +98,10 @@ do { strncpy((d),(s),sizeof((d))-1);            \
 #define memset  MISTAKE_unspecified_memset
 #define memmove MISTAKE_unspecified_memmove
 #define strcpy  MISTAKE_unspecified_strcpy
-  /* This prevents libelf from using these undecorated versions
-   * of memcpy, memset, memmove and strcpy.  Every call site
-   * must either use elf_mem*_unchecked, or elf_mem*_safe. */
+#define strcmp  MISTAKE_unspecified_strcmp
+ /* This prevents libelf from using these undecorated versions
+   * of memcpy, memset, memmove, strcpy and strcmp.  Every call site
+   * must either use elf_mem*_unchecked, or elf_*_safe. */
 
 #endif /* __LIBELF_PRIVATE_H__ */
 
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index ab83150..7fa5963 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -162,7 +162,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
             /* input has an insane section header count field */
             break;
         sname = elf_section_name(elf, shdr);
-        if ( sname && !strcmp(sname, name) )
+        if ( sname && !elf_strcmp_safe(elf, sname, name) )
             return shdr;
     }
     return ELF_INVALID_HANDLE(elf_shdr);
@@ -274,7 +274,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
         sym_name = elf_strval(elf, elf->sym_strtab + name);
         if ( sym_name == NULL ) /* out of range, oops */
             return ELF_INVALID_HANDLE(elf_sym);
-        if ( strcmp(sym_name, symbol) )
+        if ( elf_strcmp_safe(elf, sym_name, symbol) )
             continue;
         return sym;
     }
-- 
2.1.4


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

[PATCH 6/8] libelf: loop safety cleanup: Remove obsolete check in elf_shdr_count

[Ian Jackson]

All the loops which might go out of control, due to excessive shdrs,
have been decorated with elf_iter_ok.  So there is no need for this
explicit (and rather crude) check.

(Anyway, the count was a 16-bit field, so the check was redundant.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-tools.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 7fa5963..b799b56 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -131,17 +131,7 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
 
 unsigned elf_shdr_count(struct elf_binary *elf)
 {
-    unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
-    uint64_t max = elf->size / sizeof(Elf32_Shdr);
-
-    if ( max > UINT_MAX )
-        max = UINT_MAX;
-    if ( count > max )
-    {
-        elf_mark_broken(elf, "far too many section headers");
-        count = max;
-    }
-    return count;
+    return elf_uval(elf, elf->ehdr, e_shnum);
 }
 
 unsigned elf_phdr_count(struct elf_binary *elf)
-- 
2.1.4


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

[PATCH 7/8] libelf: loop safety cleanup: Remove superseded image size copy check

[Ian Jackson]

Now, elf_load_image eventually calls elf_memcpy_safe, which calls
elf_iter_ok_counted.

So there is a work limit of 4x the image size.  This is larger than
the previous limit of 2x the image size, but it includes a lot of
other processing too.  And the purpose is to reject bad images without
a significant risk of rejecting sane ones.  A 4x limit is tight
enough.

So this ad-hoc remain_allow_copy check has been entirely superseded
and can be removed.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/libelf/libelf-loader.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index d5e51d3..5e4671b 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -482,12 +482,6 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
     uint64_t paddr, offset, filesz, memsz;
     unsigned i, count;
     elf_ptrval dest;
-    /*
-     * Let bizarre ELFs write the output image up to twice; this
-     * calculation is just to ensure our copying loop is no worse than
-     * O(domain_size).
-     */
-    uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
 
     count = elf_phdr_count(elf);
     for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
@@ -504,19 +498,6 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
         memsz = elf_uval(elf, phdr, p_memsz);
         dest = elf_get_ptr(elf, paddr);
 
-        /*
-         * We need to check that the input image doesn't have us copy
-         * the whole image zillions of times, as that could lead to
-         * O(n^2) time behaviour and possible DoS by a malicous ELF.
-         */
-        if ( remain_allow_copy < memsz )
-        {
-            elf_mark_broken(elf, "program segments total to more"
-                            " than the input image size");
-            break;
-        }
-        remain_allow_copy -= memsz;
-
         elf_msg(elf,
                 "ELF: phdr %u at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
                 i, dest, (elf_ptrval)(dest + filesz));
-- 
2.1.4


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

[PATCH 8/8] libelf: safety: Document safety principles in header file

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/include/xen/libelf.h | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 6436bd7..8b75242 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -60,6 +60,96 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
 
 /* ------------------------------------------------------------------------ */
 
+/*
+ * DESIGN PRINCIPLES FOR THE SAFETY OF LIBELF
+ *
+ * libelf is a complex piece of code on a security boundary: when
+ * built as part of the tools, it parses guest kernels and loads them
+ * into guest memory.  Bugs in libelf can become privilege escalation
+ * or denial of service bugs in the toolstack.
+ *
+ * We try to reduce the risk of such bugs by writing the actual format
+ * parsing in a mostly-safe subset of C.  To avoid nonlocal exits or
+ * the need for explicit error-checking code, we make all references
+ * into the input image, or into guest memory, via an inherently safe
+ * wrapper system.
+ *
+ * This means that it is safe to simply honour the instructions from
+ * the image, even if they are nonsense.  If the image implies wild
+ * pointer accesses, these will be harmlessly defused; a note will be
+ * made that things are broken; and processing can safely continue
+ * despite some of the operations having not been done.  Eventually
+ * the error will be reported.
+ *
+ *
+ * To preserve these safety properties, there are some rules that
+ * programmers editing libelf need to follow:
+ *
+ *  - Any loop needs to be accompanied by calls to elf_iter_ok (or
+ *    elf_iter_ok_counted).
+ *
+ *        Rationale: the image must not be able to cause libelf to do
+ *        unbounded work (ie, get stuck in a loop).
+ *
+ *  - The input image and output area must be accessed only via the
+ *    safe pointer access system.  Real pointers into the input or
+ *    output may not be even *calculated*.
+ *
+ *        Rationale: calculating wild pointers is undefined behaviour;
+ *        if the compiler sees that you might be calculating wild
+ *        pointers, it may remove important checks!
+ *
+ *  - Stack local buffer variables containing information derived from
+ *    the image (including structs, or byte buffers) must be
+ *    completely zeroed, using elf_memset_unchecked (and an
+ *    accompanying elf_iter_ok_counted) on entry to the function (or
+ *    somewhere very obviously near there).
+ *
+ *        Rationale: This avoids uninitialised stack data being used
+ *        as input to any of the loader.
+ *
+ *  - All integer variables should be unsigned.
+ *
+ *        Rationale: this avoids signed integer overflow (which has
+ *        undefined behaviour in C, and if spotted by the compiler can
+ *        cause it to generate bad code).
+ *
+ *  - Arithmetic operations other than + - * should be avoided; in
+ *    particular, division (/ or %) by non-constant values should be
+ *    avoided.  If it cannot be avoided, the divisor must be checked
+ *    for zero.
+ *
+ *        Rationale: We must avoid division-by-zero (or other overflow
+ *        traps).
+ *
+ *  - If it is desirable to breach these rules, there should be a
+ *    comment explaining why this is OK.
+ *
+ * Even so, this is a fairly high-risk environment, so:
+ *
+ *  - Do not add code which is not necessary for libelf to function
+ *    with correct input, or to avoid being vulnerable to incorrect
+ *    input.  Do not add additional functionally-unnecessary checks
+ *    for diagnosing problems with the image, or validating sanity of
+ *    the input ELF.
+ *
+ *        Rationale: Redundant checks have almost zero benefit because
+ *        1. we do not expect ELF-generating tools to generate invalid
+ *        ELFs so these checks' failure paths will very likely never
+ *        be executed anywhere, and 2. anyone debugging such a
+ *        hypothetical bad ELF will have a variety of tools available
+ *        which will do a much better job of analysing it.
+ *
+ *  - However, it is OK to have checks code which provide duplicate
+ *    defence against certain hostile images, if it is not otherwise
+ *    obvious how libelf would be defended against such images.
+ *
+ *        Rationale: Redundant checks where the situation would
+ *        otherwise not be quite clear mean that the safety of the
+ *        code is easy to see throughout; so that any unsafe code
+ *        would be more obvious.
+ */
+
 /* Macros for accessing the input image and output area. */
 
 /*
@@ -475,6 +565,8 @@ static inline void *elf_memset_unchecked(void *s, int c, size_t n)
    * pointers.  These are just like the real functions.
    * We provide these so that in libelf-private.h we can #define
    * memcpy, memset and memmove to undefined MISTAKE things.
+   *
+   * Remember to call elf_iter_ok_counted nearby.
    */
 
 static inline int elf_strcmp_safe(struct elf_binary *elf,
-- 
2.1.4


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> This will allow us to keep track of the total amount of work we are
> doing.  When it becomes excessive, we mark the ELF broken, and stop
> processing.
> 
> This is a more robust way of preventing DoS problems by bad images
> than attempting to prove, for each of the (sometimes rather deeply
> nested) loops, that the total work is "reasonable".  We bound the
> notional work by 4x the image size (plus 1M).

The 4x has been selected arbitrarily, I suppose?

> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char 
> *image_input, size_t
>      ELF_HANDLE_DECL(elf_shdr) shdr;
>      unsigned i, count, section, link;
>      uint64_t offset;
> +    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;

This will need adjustment for 32-bit tool stack builds - did you
perhaps mean UINT64_C(1), considering the type of the variable?

Also please add blanks around the / .

> @@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
>      return value;
>  }
>  
> +bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) {
> +    if (maxcopysz > elf->iteration_deaccumulator)
> +        elf_mark_broken(elf, "excessive iteration - too much work to parse");
> +    if (elf->broken)
> +        return false;
> +    elf->iteration_deaccumulator -= maxcopysz;
> +    return true;
> +}

Hypervisor coding style here please (missing lots of blanks, and the
opening brace needs to go on its own line). I think there are more
such style problems further down.

And then - would it perhaps make sense to have most of this
function's body in #ifndef __XEN__, as there's nothing to guard
against when loading Dom0?

Jan


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

Re: [PATCH 2/8] libelf: loop safety: Pass `elf' to elf_xen_parse_features

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> Not used yet, so no functional change.  We will need this in a moment.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -43,10 +43,13 @@ elf_errorstatus elf_xen_parse_features(struct elf_binary *elf,
>      if ( features == NULL )
>          return 0;
>  
> -    for ( pos = 0; features[pos] != '\0'; pos += len )
> +    for ( pos = 0;
> +          elf_iter_ok_counted(elf, sizeof(feature)) &&
> +              features[pos] != '\0';

The two sides of the && want to align with one another.

> @@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
>  
>      h = parms->guest_info;
>  #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
> -    while ( STAR(h) )
> +    while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
> +            STAR(h) )

So you're using libelf determined sizes here rather than image
properties. Perhaps that's not a big deal, but wouldn't it be more
reasonable to simply account the whole section's size just once?

> @@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
>      uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
>  
>      count = elf_phdr_count(elf);
> -    for ( i = 0; i < count; i++ )
> +    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )

At the example of this, but likely applicable elsewhere - what use is
this check at this point in the series? Without peeking at later patches
it is not really clear how adding this makes any difference.

Overall I'm not entirely opposed to the approach, but I find these
extra checks rather unpleasant to have.

Jan


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> +static inline bool elf_iter_ok(struct elf_binary *elf)
> +    { return elf_iter_ok_counted(elf,1); }

Having seen first uses of this - why is this 1 instead of 0? Once it is,
calling elf_iter_ok_counted() here would be rather pointless, and
checking just elf_broken() here would allow the parameter to become
const.

Jan


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

Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> @@ -80,7 +81,8 @@ void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
>  
>  void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size)
>  {
> -    if ( elf_access_ok(elf, dst, size) )
> +    if ( elf_access_ok(elf, dst, size) &&
> +         elf_iter_ok_counted(elf, size))

With the style issue here fixed,
Acked-by: Jan Beulich <jbeulich@suse.com>
despite not being really happy about the added clutter.

Jan


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

Re: [PATCH 5/8] libelf: loop safety: Replace all calls to strcmp

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
>          /* other */
> -        if ( !strcmp(name, "FEATURES") )
> -            if ( elf_xen_parse_features(value, parms->f_supported,
> +        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
> +            if ( elf_xen_parse_features(elf, value, parms->f_supported,
>                                          parms->f_required) )
>                  return -1;

There must be some patch split problem here - this patch does not
alter the parameters of elf_xen_parse_features(), so the argument
list can't change here.

And one of the then (at least) two patches touching this code could
then take the opportunity and fold the two if()s into one.

Jan


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > This is a more robust way of preventing DoS problems by bad images
> > than attempting to prove, for each of the (sometimes rather deeply
> > nested) loops, that the total work is "reasonable".  We bound the
> > notional work by 4x the image size (plus 1M).
> 
> The 4x has been selected arbitrarily, I suppose?

Yes.

> >      uint64_t offset;
> > +    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
> 
> This will need adjustment for 32-bit tool stack builds - did you
> perhaps mean UINT64_C(1), considering the type of the variable?

Oops, yes.  Although it has to be (uint64_t)1 since the tools do not
have UINT64_C.

> Also please add blanks around the / .

Done.

> Hypervisor coding style here please (missing lots of blanks, and the
> opening brace needs to go on its own line). I think there are more
> such style problems further down.

Sorry, I will (try to) fix the style in all the patches.

> And then - would it perhaps make sense to have most of this
> function's body in #ifndef __XEN__, as there's nothing to guard
> against when loading Dom0?

This is a good idea.  If it's OK with you I'll leave the variable
initialisation etc., and simply stub out body of elf_iter_ok_counted.
If you want a more comprehensive approach I can add more #ifdefs.

Thanks,
Ian.

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

Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Ian Jackson]

Thanks for your careful review.

Jan Beulich writes ("Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > +    for ( pos = 0;
> > +          elf_iter_ok_counted(elf, sizeof(feature)) &&
> > +              features[pos] != '\0';
> 
> The two sides of the && want to align with one another.

Sure, if you like it that way.

> > @@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
> >  
> >      h = parms->guest_info;
> >  #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
> > -    while ( STAR(h) )
> > +    while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
> > +            STAR(h) )
> 
> So you're using libelf determined sizes here rather than image
> properties.

I'm not sure what you mean.  Each iteration of this loop contains some
calls to elf_memset_unchecked.  The rules say:

 *  - Stack local buffer variables containing information derived from
 *    the image (including structs, or byte buffers) must be
 *    completely zeroed, using elf_memset_unchecked (and an
 *    accompanying elf_iter_ok_counted) on entry to the function (or
 *    somewhere very obviously near there).

And by elf_*_unchecked:

 * Remember to call elf_iter_ok_counted nearby.

So the calls to elf_memset_unchecked, to zero name and value, imply
that there must be a call to elf_iter_ok_counted.  The count parameter
should be the actual work done.

There are two loops inside this outer loop.  They are textually but
not logically nested.  By the rules each of these loops needs a call
to elf_iter_ok, but: since they iterate over characters for name and
value respectively, they clearly do no more work than the memsets.

> Perhaps that's not a big deal, but wouldn't it be more
> reasonable to simply account the whole section's size just once?

You mean to call elf_iter_ok_counted on the size of the note section ?

But that would be wrong, because in principle almost each character in
the note section could cause a memset of all of name and a memset of
all of value.

Doing the iteration checks at a distance, and based on input image
properties rather than actual code flow, requires the kind of subtle
and complicated analysis I found too fragile.

> > @@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
> >      uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
> >  
> >      count = elf_phdr_count(elf);
> > -    for ( i = 0; i < count; i++ )
> > +    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
> 
> At the example of this, but likely applicable elsewhere - what use is
> this check at this point in the series? Without peeking at later patches
> it is not really clear how adding this makes any difference.
> 
> Overall I'm not entirely opposed to the approach, but I find these
> extra checks rather unpleasant to have.

I think you may need to read the big comment in patch 8.  I introduce
that at the end of the series because it doesn't become true before
then.

If you like I can move it to the front of the series, and introduce it
with a "may not be true" caveat which is later removed.

Ian.

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

Re: [PATCH 6/8] libelf: loop safety cleanup: Remove obsolete check in elf_shdr_count

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> All the loops which might go out of control, due to excessive shdrs,
> have been decorated with elf_iter_ok.  So there is no need for this
> explicit (and rather crude) check.
> 
> (Anyway, the count was a 16-bit field, so the check was redundant.)
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 5/8] libelf: loop safety: Replace all calls to strcmp

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 5/8] libelf: loop safety: Replace all calls to strcmp"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> >          /* other */
> > -        if ( !strcmp(name, "FEATURES") )
> > -            if ( elf_xen_parse_features(value, parms->f_supported,
> > +        if ( !elf_strcmp_safe(elf, name, "FEATURES") )
> > +            if ( elf_xen_parse_features(elf, value, parms->f_supported,
> >                                          parms->f_required) )
> >                  return -1;
> 
> There must be some patch split problem here - this patch does not
> alter the parameters of elf_xen_parse_features(), so the argument
> list can't change here.

Yes.  That part of this hunk should be in
   libelf: loop safety: Pass `elf' to elf_xen_parse_features
I thought I had done a compile test of that individual patch but
I must have messed that up somehow.  I will move that change.

> And one of the then (at least) two patches touching this code could
> then take the opportunity and fold the two if()s into one.

Looking at the surrounding code, I actually prefer it the way it is.
It makes it more like the other similar test/handle pairs.

Thanks,
Ian.

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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
>      ELF_HANDLE_DECL(elf_shdr) shdr;
>      unsigned i, count, section, link;
>      uint64_t offset;
> +    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
>  
>      if ( !elf_is_elfbinary(image_input, size) )
>      {
> @@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
>      elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
>      elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
>  
> +    elf->iteration_deaccumulator = 1024*1024 +
> +        (size > max_size_for_deacc ? max_size_for_deacc : size)
> +        * ELF_MAX_ITERATION_FACTOR;        

One more question here: Is this useful at all? You're allowing
for approximately 2**63 accounted operations - how big does
an image need to be to actually break this limit? XSA-25 already
limited image sizes to 1Gb (but I do understand that the
guarding here is also against e.g. redundant loading of the
same bits through multiple program header table entries).

And how long will it take you to reach that limit (and to cause
elf->broken to be set)? With 1ns per accounted operation,
that'll be on the order of 270 years. Am I missing something
here?

Jan


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

Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > -    if ( elf_access_ok(elf, dst, size) )
> > +    if ( elf_access_ok(elf, dst, size) &&
> > +         elf_iter_ok_counted(elf, size))
> 
> With the style issue here fixed,
> Acked-by: Jan Beulich <jbeulich@suse.com>
> despite not being really happy about the added clutter.

I have added the missing space, and also moved the && to the next
line (as seems to be done in much of the hypervisor).

I hope that meets with your approval.

Ian.

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

Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Jan Beulich]

>>> On 12.12.16 at 16:38, <ian.jackson@eu.citrix.com> wrote:
>> > @@ -273,11 +276,12 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
>> >  
>> >      h = parms->guest_info;
>> >  #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
>> > -    while ( STAR(h) )
>> > +    while ( elf_iter_ok_counted(elf, sizeof(name) + sizeof(value)) &&
>> > +            STAR(h) )
>> 
>> So you're using libelf determined sizes here rather than image
>> properties.
> 
> I'm not sure what you mean.  Each iteration of this loop contains some
> calls to elf_memset_unchecked.  The rules say:
> 
>  *  - Stack local buffer variables containing information derived from
>  *    the image (including structs, or byte buffers) must be
>  *    completely zeroed, using elf_memset_unchecked (and an
>  *    accompanying elf_iter_ok_counted) on entry to the function (or
>  *    somewhere very obviously near there).
> 
> And by elf_*_unchecked:
> 
>  * Remember to call elf_iter_ok_counted nearby.
> 
> So the calls to elf_memset_unchecked, to zero name and value, imply
> that there must be a call to elf_iter_ok_counted.  The count parameter
> should be the actual work done.

Hmm, if the rules say that, I'll then have to question the rules:
Shouldn't accounting be based on what the workload the image
causes us, instead of our own overhead?

>> Perhaps that's not a big deal, but wouldn't it be more
>> reasonable to simply account the whole section's size just once?
> 
> You mean to call elf_iter_ok_counted on the size of the note section ?
> 
> But that would be wrong, because in principle almost each character in
> the note section could cause a memset of all of name and a memset of
> all of value.

Well, as per above, different ways you and I think this should work
then, as it seems. I can see where you're coming from, but I'm not
convinced this is the right model when taking a client (image)
perspective.

>> > @@ -490,7 +490,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
>> >      uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
>> >  
>> >      count = elf_phdr_count(elf);
>> > -    for ( i = 0; i < count; i++ )
>> > +    for ( i = 0; elf_iter_ok(elf) && i < count; i++ )
>> 
>> At the example of this, but likely applicable elsewhere - what use is
>> this check at this point in the series? Without peeking at later patches
>> it is not really clear how adding this makes any difference.
>> 
>> Overall I'm not entirely opposed to the approach, but I find these
>> extra checks rather unpleasant to have.
> 
> I think you may need to read the big comment in patch 8.  I introduce
> that at the end of the series because it doesn't become true before
> then.

Yeah, I still need to make it there (and maybe I should have looked
at that one first).

Jan


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

Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe

[Jan Beulich]

>>> On 12.12.16 at 16:54, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 4/8] libelf: loop safety: Call 
> elf_iter_ok_counted at every *mem*_unsafe"):
>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
>> > -    if ( elf_access_ok(elf, dst, size) )
>> > +    if ( elf_access_ok(elf, dst, size) &&
>> > +         elf_iter_ok_counted(elf, size))
>> 
>> With the style issue here fixed,
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> despite not being really happy about the added clutter.
> 
> I have added the missing space, and also moved the && to the next
> line (as seems to be done in much of the hypervisor).
> 
> I hope that meets with your approval.

Well, I don't mind either placement of the && (personally I like it
better at the front of a line, but the common model in fact is to
have it at the end).

Jan


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > +    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
...
> > +    elf->iteration_deaccumulator = 1024*1024 +
> > +        (size > max_size_for_deacc ? max_size_for_deacc : size)
> > +        * ELF_MAX_ITERATION_FACTOR;        
> 
> One more question here: Is this useful at all? You're allowing
> for approximately 2**63 accounted operations - how big does
> an image need to be to actually break this limit? XSA-25 already
> limited image sizes to 1Gb (but I do understand that the
> guarding here is also against e.g. redundant loading of the
> same bits through multiple program header table entries).
> 
> And how long will it take you to reach that limit (and to cause
> elf->broken to be set)? With 1ns per accounted operation,
> that'll be on the order of 270 years. Am I missing something
> here?

I'm not sure of your point.

Mostly I allow for 4x the size of the image, plus a fixed constant of
1M operations.

The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow.  It
seems unreasonable to simply allow that overflow to occur.  But if it
is causing confusion we could do that.  The result would be a low
value for iteration_deaccumulator.

In practice the 1Gby image size limit will prevent this limit from
being reached, but it is distant from this check.

Ian.

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

Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 3/8] libelf: loop safety: Call elf_iter_ok[_counted] in every loop"):
> On 12.12.16 at 16:38, <ian.jackson@eu.citrix.com> wrote:
> > So the calls to elf_memset_unchecked, to zero name and value, imply
> > that there must be a call to elf_iter_ok_counted.  The count parameter
> > should be the actual work done.
> 
> Hmm, if the rules say that, I'll then have to question the rules:
> Shouldn't accounting be based on what the workload the image
> causes us, instead of our own overhead?

The purpose of the accounting is to prevent the image from causing us
to do lots of work.  The work calculation should be based on the
actual algorithm, not on some hypothetical other algorithm that might
be more efficient.

Otherwise if our algorithm is inefficient in some surprising way, when
faced with certain unusual images, that would be a DOS vulnerability.

I think it is easier to write these checks, in terms of the actual
work done, than attempt to construct a proof that the algorithm always
only does a reasonable amount of work.

Ian.

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

Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 4/8] libelf: loop safety: Call elf_iter_ok_counted at every *mem*_unsafe"):
> On 12.12.16 at 16:54, <ian.jackson@eu.citrix.com> wrote:
> > I have added the missing space, and also moved the && to the next
> > line (as seems to be done in much of the hypervisor).
> > 
> > I hope that meets with your approval.
> 
> Well, I don't mind either placement of the && (personally I like it
> better at the front of a line, but the common model in fact is to
> have it at the end).

Heh.  At least we agree on something here then ...

Ian.

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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce 
> elf_iter_ok and elf_strcmp_safe"):
>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
>> > +    const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
> ...
>> > +    elf->iteration_deaccumulator = 1024*1024 +
>> > +        (size > max_size_for_deacc ? max_size_for_deacc : size)
>> > +        * ELF_MAX_ITERATION_FACTOR;        
>> 
>> One more question here: Is this useful at all? You're allowing
>> for approximately 2**63 accounted operations - how big does
>> an image need to be to actually break this limit? XSA-25 already
>> limited image sizes to 1Gb (but I do understand that the
>> guarding here is also against e.g. redundant loading of the
>> same bits through multiple program header table entries).
>> 
>> And how long will it take you to reach that limit (and to cause
>> elf->broken to be set)? With 1ns per accounted operation,
>> that'll be on the order of 270 years. Am I missing something
>> here?
> 
> I'm not sure of your point.
> 
> Mostly I allow for 4x the size of the image, plus a fixed constant of
> 1M operations.

Well, I have to confess that I've read the above as max() when
in fact it is min().

> The max_size_for_deacc part is necessary because otherwise the
> calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow.  It
> seems unreasonable to simply allow that overflow to occur.  But if it
> is causing confusion we could do that.  The result would be a low
> value for iteration_deaccumulator.

Considering that overflow here will actually result in a comparably
smaller upper limit, I think this may help overall readability. But with
the above I won't insist on this in any way.

Jan


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

Re: [PATCH 7/8] libelf: loop safety cleanup: Remove superseded image size copy check

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> Now, elf_load_image eventually calls elf_memcpy_safe, which calls
> elf_iter_ok_counted.
> 
> So there is a work limit of 4x the image size.  This is larger than
> the previous limit of 2x the image size, but it includes a lot of
> other processing too.  And the purpose is to reject bad images without
> a significant risk of rejecting sane ones.  A 4x limit is tight
> enough.
> 
> So this ad-hoc remain_allow_copy check has been entirely superseded
> and can be removed.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> Well, I have to confess that I've read the above as max() when
> in fact it is min().

Sadly we can't use min() and max() here it seems.

> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote:
> > The max_size_for_deacc part is necessary because otherwise the
> > calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow.  It
> > seems unreasonable to simply allow that overflow to occur.  But if it
> > is causing confusion we could do that.  The result would be a low
> > value for iteration_deaccumulator.
> 
> Considering that overflow here will actually result in a comparably
> smaller upper limit, I think this may help overall readability. But with
> the above I won't insist on this in any way.

I have replaced the limit with a comment.  Now I have:

    elf->iteration_deaccumulator =
        1024*1024 + size * ELF_MAX_ITERATION_FACTOR;        
        /* overflow (from very big size, probably rejected earlier)
         * would just lead to small limit, which is safe */

Ian.

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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce 
> elf_iter_ok and elf_strcmp_safe"):
>> Well, I have to confess that I've read the above as max() when
>> in fact it is min().
> 
> Sadly we can't use min() and max() here it seems.

Sure, I understand that.

>> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote:
>> > The max_size_for_deacc part is necessary because otherwise the
>> > calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow.  It
>> > seems unreasonable to simply allow that overflow to occur.  But if it
>> > is causing confusion we could do that.  The result would be a low
>> > value for iteration_deaccumulator.
>> 
>> Considering that overflow here will actually result in a comparably
>> smaller upper limit, I think this may help overall readability. But with
>> the above I won't insist on this in any way.
> 
> I have replaced the limit with a comment.  Now I have:
> 
>     elf->iteration_deaccumulator =
>         1024*1024 + size * ELF_MAX_ITERATION_FACTOR;        
>         /* overflow (from very big size, probably rejected earlier)
>          * would just lead to small limit, which is safe */

Thanks. May I ask that you then also use proper hypervisor
style for that comment?

Jan


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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote:
> > I have replaced the limit with a comment.  Now I have:
> > 
> >     elf->iteration_deaccumulator =
> >         1024*1024 + size * ELF_MAX_ITERATION_FACTOR;        
> >         /* overflow (from very big size, probably rejected earlier)
> >          * would just lead to small limit, which is safe */
> 
> Thanks. May I ask that you then also use proper hypervisor
> style for that comment?

You mean like this ?

    elf->iteration_deaccumulator =
        1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
        /*
         * overflow (from very big size, probably rejected earlier)
         * would just lead to small limit, which is safe
         */

Sure, whatever.  Done.

Ian.

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

Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe

[Jan Beulich]

>>> On 13.12.16 at 17:04, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce 
> elf_iter_ok and elf_strcmp_safe"):
>> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote:
>> > I have replaced the limit with a comment.  Now I have:
>> > 
>> >     elf->iteration_deaccumulator =
>> >         1024*1024 + size * ELF_MAX_ITERATION_FACTOR;        
>> >         /* overflow (from very big size, probably rejected earlier)
>> >          * would just lead to small limit, which is safe */
>> 
>> Thanks. May I ask that you then also use proper hypervisor
>> style for that comment?
> 
> You mean like this ?
> 
>     elf->iteration_deaccumulator =
>         1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
>         /*
>          * overflow (from very big size, probably rejected earlier)
>          * would just lead to small limit, which is safe
>          */
> 
> Sure, whatever.  Done.

Almost: Comments are also supposed to start with an upper
case letter. But I'm fine either way.

Jan


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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[Jan Beulich]

>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> +/*
> + * DESIGN PRINCIPLES FOR THE SAFETY OF LIBELF
> + *
> + * libelf is a complex piece of code on a security boundary: when
> + * built as part of the tools, it parses guest kernels and loads them
> + * into guest memory.  Bugs in libelf can become privilege escalation
> + * or denial of service bugs in the toolstack.
> + *
> + * We try to reduce the risk of such bugs by writing the actual format
> + * parsing in a mostly-safe subset of C.  To avoid nonlocal exits or
> + * the need for explicit error-checking code, we make all references
> + * into the input image, or into guest memory, via an inherently safe
> + * wrapper system.
> + *
> + * This means that it is safe to simply honour the instructions from
> + * the image, even if they are nonsense.  If the image implies wild
> + * pointer accesses, these will be harmlessly defused; a note will be
> + * made that things are broken; and processing can safely continue
> + * despite some of the operations having not been done.  Eventually
> + * the error will be reported.
> + *
> + *
> + * To preserve these safety properties, there are some rules that
> + * programmers editing libelf need to follow:
> + *
> + *  - Any loop needs to be accompanied by calls to elf_iter_ok (or
> + *    elf_iter_ok_counted).
> + *
> + *        Rationale: the image must not be able to cause libelf to do
> + *        unbounded work (ie, get stuck in a loop).

As expressed before, I'm not convinced library code should be
concerned about caller restrictions.

> + *  - The input image and output area must be accessed only via the
> + *    safe pointer access system.  Real pointers into the input or
> + *    output may not be even *calculated*.
> + *
> + *        Rationale: calculating wild pointers is undefined behaviour;
> + *        if the compiler sees that you might be calculating wild
> + *        pointers, it may remove important checks!
> + *
> + *  - Stack local buffer variables containing information derived from
> + *    the image (including structs, or byte buffers) must be
> + *    completely zeroed, using elf_memset_unchecked (and an
> + *    accompanying elf_iter_ok_counted) on entry to the function (or
> + *    somewhere very obviously near there).
> + *
> + *        Rationale: This avoids uninitialised stack data being used
> + *        as input to any of the loader.

What distinguishes a "buffer variable" from a plain variable? I.e.
where is the boundary here as to which ones need zeroing? Also,
I don't think zeroing is needed when a variable gets fully written
over (like in a structure assignment). Do you perhaps want to
limit the above to "directly derived from the image"?

> + *  - All integer variables should be unsigned.
> + *
> + *        Rationale: this avoids signed integer overflow (which has
> + *        undefined behaviour in C, and if spotted by the compiler can
> + *        cause it to generate bad code).

There are certainly cases where one explicitly needs negative
values, and hence signed integer types. Therefore I don't think
we can outright exclude this. Perhaps limit by "... variables with
non-negative value range ..."?

> + *  - Arithmetic operations other than + - * should be avoided; in
> + *    particular, division (/ or %) by non-constant values should be
> + *    avoided.  If it cannot be avoided, the divisor must be checked
> + *    for zero.
> + *
> + *        Rationale: We must avoid division-by-zero (or other overflow
> + *        traps).
> + *
> + *  - If it is desirable to breach these rules, there should be a
> + *    comment explaining why this is OK.
> + *
> + * Even so, this is a fairly high-risk environment, so:
> + *
> + *  - Do not add code which is not necessary for libelf to function
> + *    with correct input, or to avoid being vulnerable to incorrect
> + *    input.  Do not add additional functionally-unnecessary checks
> + *    for diagnosing problems with the image, or validating sanity of
> + *    the input ELF.
> + *
> + *        Rationale: Redundant checks have almost zero benefit because
> + *        1. we do not expect ELF-generating tools to generate invalid
> + *        ELFs so these checks' failure paths will very likely never
> + *        be executed anywhere, and 2. anyone debugging such a
> + *        hypothetical bad ELF will have a variety of tools available
> + *        which will do a much better job of analysing it.

While I can understand your reasoning, I continue to think that
libelf in a ELF loader, and hence should check certain things. The
best example I currently know of are e_[ps]hentsize, which the
code uses without checking the lower valid bound.

Overall I'm certainly not meaning to veto any of this, but I think it
would be better for some other REST maintainer (agreeing with
your way of thinking) to ack this.

Jan

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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[George Dunlap]

> On Dec 16, 2016, at 12:43 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
>> + *  - Any loop needs to be accompanied by calls to elf_iter_ok (or
>> + *    elf_iter_ok_counted).
>> + *
>> + *        Rationale: the image must not be able to cause libelf to do
>> + *        unbounded work (ie, get stuck in a loop).
> 
> As expressed before, I'm not convinced library code should be
> concerned about caller restrictions.

People designing toolstacks that call this function are likely to be thinking about domains and things, not, “What happens if I get a rogue elf image that causes this function to run forever?”  I think if we can prevent libelf-source DoS bugs in all toolstacks that rely on libxl, then it makes sense to do so.

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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 8/8] libelf: safety: Document safety principles in header file"):
> > On Dec 16, 2016, at 12:43 AM, Jan Beulich <JBeulich@suse.com> wrote:
> > As expressed before, I'm not convinced library code should be
> > concerned about caller restrictions.

I'm not sure what you mean by "caller restrictions".  This is a rule
that applies inside libelf.  Can you explain ?

Assuming you mean what George seems to think you meant:

> People designing toolstacks that call this function are likely to be thinking about domains and things, not, “What happens if I get a rogue elf image that causes this function to run forever?”  I think if we can prevent libelf-source DoS bugs in all toolstacks that rely on libxl, then it makes sense to do so.

I think in fact the only caller of libelf is libxc.  I doubt we have
out-of-tree callers, but I could be wrong of course.

But that doesn't change the underlying point, which I agree with:
callers of any library should be given information on how to use it
safely.

Our libelf does not have a 100% opaque interface for its callers.  For
example, the state struct is transparent, and the multiple entry
points are not entirely clearly set out.

If it did have a more formally defined and opaque interface, then much
of what is currently in libelf.h (including this comment) would be in
libelf-private.h instead.

Thanks,
Ian.

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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 8/8] libelf: safety: Document safety principles in header file"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > + *  - Stack local buffer variables containing information derived from
> > + *    the image (including structs, or byte buffers) must be
> > + *    completely zeroed, using elf_memset_unchecked (and an
> > + *    accompanying elf_iter_ok_counted) on entry to the function (or
> > + *    somewhere very obviously near there).
> > + *
> > + *        Rationale: This avoids uninitialised stack data being used
> > + *        as input to any of the loader.
> 
> What distinguishes a "buffer variable" from a plain variable? I.e.
> where is the boundary here as to which ones need zeroing?

The hazard is an undiscovered uninitialised variable.  "Undiscovered"
meaning static analysis tools won't find it (eg Coverity will find
many if not most normal uninitialised variables).

This is the biggest risk for variables which can be partially
initialised: ie, structs and arrays.  It is more difficult to reason
about variables which can be partially initialised.

> Also,
> I don't think zeroing is needed when a variable gets fully written
> over (like in a structure assignment). Do you perhaps want to
> limit the above to "directly derived from the image"?

Structure assignment does not necessarily write over the whole
variable.  It may leave the padding un-overwritten.

> > + *  - All integer variables should be unsigned.
> > + *
> > + *        Rationale: this avoids signed integer overflow (which has
> > + *        undefined behaviour in C, and if spotted by the compiler can
> > + *        cause it to generate bad code).
> 
> There are certainly cases where one explicitly needs negative
> values, and hence signed integer types. Therefore I don't think
> we can outright exclude this. Perhaps limit by "... variables with
> non-negative value range ..."?

There are AFAICT the following signed variables in libelf: some error
codes; the c parameter to elf_memset_safe; the `nr' parameters to
elf_xen_feature_set/get.

I think perhaps the exceptions should be mentioned.  The error codes
are unfortunate but they are safe and too hard to change, and should
get a comment.  The c and nr parameters should be made unsigned.  The
open-coded ints for some function return values should be changed to
elf_errorstatus.

I will update the series to improve this.

Anyway, it is not necessary to deal with actually negative
quantities.  If it should become necessary in the future then:

    + *  - If it is desirable to breach these rules, there should be a
    + *    comment explaining why this is OK.

> > + *  - Do not add code which is not necessary for libelf to function
> > + *    with correct input, or to avoid being vulnerable to incorrect
> > + *    input.  Do not add additional functionally-unnecessary checks
> > + *    for diagnosing problems with the image, or validating sanity of
> > + *    the input ELF.
...
> While I can understand your reasoning, I continue to think that
> libelf in a ELF loader, and hence should check certain things. The
> best example I currently know of are e_[ps]hentsize, which the
> code uses without checking the lower valid bound.

You don't give a counterargument to my reasoning:

> > + *        Rationale: Redundant checks have almost zero benefit because
> > + *        1. we do not expect ELF-generating tools to generate invalid
> > + *        ELFs so these checks' failure paths will very likely never
> > + *        be executed anywhere, and 2. anyone debugging such a
> > + *        hypothetical bad ELF will have a variety of tools available
> > + *        which will do a much better job of analysing it.

I don't find "libelf in a ELF loader" a convincing counterargument.

In practical terms, not checking e_[ps]hentsize means that
hypothetical broken images with too-small e_[ps]hentsize will result
in either a confusing error reported later by some other bit of libelf
(for example, due to an out-of-bounds access setting the `broken'
flag), or a badly-constructed guest.  This is IMO a quite tolerable
consequence.  Following a confusing error message the afflicted user
is very likely to do something sensible, like feeding their image to a
disassembler, which would immediately diagnose the problem.  Even
if the result is a badly-constructed guest, the symptoms are very
likely to induce an appropriate debugging response, and not likely to
cause great inconvenience (compared to a specific diagnosis).

Especially, since (and AFIACT you are not disputing this) we do not
expect that such a check's failure path would ever be executed
anywhere.  The expected benefit of the extra check is negligible.

The cost of the extra check is the risk of making mistakes, resulting
in security vulnerabilities.  Security vulnerabilties have a high
cost.  So even if the probability of making such a mistake is fairly
low, the expected cost of the extra checks is definitely
non-negligible.

> Overall I'm certainly not meaning to veto any of this, but I think it
> would be better for some other REST maintainer (agreeing with
> your way of thinking) to ack this.

Thanks anyway for your careful review and attention to detail.

George, since you seem to be reading this thread: what do you think
about this disagreement about the right direction to go ?  I think
that the fact that the other approach resulted in us committing a
vulnerability to staging is a strong indication that we need better
defence against (inevitable) human error.

Thanks,
Ian.

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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[Jan Beulich]

>>> On 16.12.16 at 12:33, <ian.jackson@eu.citrix.com> wrote:
> George Dunlap writes ("Re: [PATCH 8/8] libelf: safety: Document safety 
> principles in header file"):
>> > On Dec 16, 2016, at 12:43 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> > As expressed before, I'm not convinced library code should be
>> > concerned about caller restrictions.
> 
> I'm not sure what you mean by "caller restrictions".  This is a rule
> that applies inside libelf.  Can you explain ?

This goes back to the single threaded discussion we've had. As a
library it shouldn't be concerned how long it takes for it to complete
its job, as long as it follows what the EFL image says (and that
image is either valid, or errors get surfaced for any invalid elements).

Jan


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

Re: [PATCH 8/8] libelf: safety: Document safety principles in header file

[Jan Beulich]

>>> On 16.12.16 at 12:43, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 8/8] libelf: safety: Document safety 
> principles in header file"):
>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
>> > + *  - Stack local buffer variables containing information derived from
>> > + *    the image (including structs, or byte buffers) must be
>> > + *    completely zeroed, using elf_memset_unchecked (and an
>> > + *    accompanying elf_iter_ok_counted) on entry to the function (or
>> > + *    somewhere very obviously near there).
>> > + *
>> > + *        Rationale: This avoids uninitialised stack data being used
>> > + *        as input to any of the loader.
>> 
>> What distinguishes a "buffer variable" from a plain variable? I.e.
>> where is the boundary here as to which ones need zeroing?
> 
> The hazard is an undiscovered uninitialised variable.  "Undiscovered"
> meaning static analysis tools won't find it (eg Coverity will find
> many if not most normal uninitialised variables).
> 
> This is the biggest risk for variables which can be partially
> initialised: ie, structs and arrays.  It is more difficult to reason
> about variables which can be partially initialised.

I guess you meant "can't"? In any event the wording may want
some clarification then.

>> Also,
>> I don't think zeroing is needed when a variable gets fully written
>> over (like in a structure assignment). Do you perhaps want to
>> limit the above to "directly derived from the image"?
> 
> Structure assignment does not necessarily write over the whole
> variable.  It may leave the padding un-overwritten.

Oh, true.

>> > + *  - Do not add code which is not necessary for libelf to function
>> > + *    with correct input, or to avoid being vulnerable to incorrect
>> > + *    input.  Do not add additional functionally-unnecessary checks
>> > + *    for diagnosing problems with the image, or validating sanity of
>> > + *    the input ELF.
> ...
>> While I can understand your reasoning, I continue to think that
>> libelf in a ELF loader, and hence should check certain things. The
>> best example I currently know of are e_[ps]hentsize, which the
>> code uses without checking the lower valid bound.
> 
> You don't give a counterargument to my reasoning:
> 
>> > + *        Rationale: Redundant checks have almost zero benefit because
>> > + *        1. we do not expect ELF-generating tools to generate invalid
>> > + *        ELFs so these checks' failure paths will very likely never
>> > + *        be executed anywhere, and 2. anyone debugging such a
>> > + *        hypothetical bad ELF will have a variety of tools available
>> > + *        which will do a much better job of analysing it.

Well, because as said - I can understand it, given the perspective
you take.

Jan


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