llvm dev - Jun 2017 - [CFI] Manually linking classes that have no inheritance link

-krasin@

On Fri, Jun 16, 2017 at 11:05 AM, Kostya Serebryany <kcc at google.com>
wrote:
>
>
> On Thu, Jun 15, 2017 at 10:39 PM, Enes Göktaş <enes.goktas at
gmail.com>
> wrote:
>
>> Hi Kostya,
>>
>> Please find attached the minimized motivation test.
>> I hope it is minimized enough. If not please let me know so I can try
to
>> make it more minimal.
>> Were you expecting something like this?
>>
>> Also I think the tests that I should provide along with the patch
should
>> be in a special format right?
>>
>
> Yes. Take a look at other tests in llvm/projects/compiler-rt/test/cfi
>
> (I did not study your patch or tests in detail yet, and probably won't
> have time until mid Jul. But others may)
>
> My major concern with any such patch is that it complicates the
> implementation.
> For many parts of compiler extra complexity is acceptable, but CFI is a
> security mitigation feature and as such should be minimal.
>
> --kcc
>
>
>> I think I should be looking at http://llvm.org/docs/Developer
>> Policy.html#test-cases, and http://llvm.org/docs/TestingGuide.html for
>> more information for adding tests to the patch. Any other handy links
by
>> any chance?
>>
>> --
>> Enes
>>
>> On Thu, Jun 15, 2017 at 1:51 PM, Kostya Serebryany <kcc at
google.com>
>> wrote:
>>
>>> Hi Enes,
>>>
>>> I usually find it nearly impossible to discuss complex issues likes
this
>>> w/o having a minimized motivation test.
>>> Could you please provide such a test with one of the patches?
>>> (And in general, please try to provide tests with any patch)
>>>
>>> Thanks!
>>>
>>> --kcc
>>>
>>>
>>> On Thu, Jun 15, 2017 at 5:08 AM, Enes Göktaş <enes.goktas at
gmail.com>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>> I would like to propose extending the Control-Flow Integrity
(CFI)
>>>> mechanism in LLVM/Clang with a feature that allows users to
explicitly link
>>>> classes that have no inheritance link. Usually, if one class is
used at
>>>> locations in code where this class is not expected, this will
create a CFI
>>>> error at runtime, assuming the application is built with CFI
enabled.
>>>> However, in cases where the user has a complex code structure
or design
>>>> that should allow this behavior, there is currently no solution
but
>>>> disabling the CFI checks. Disabling the CFI checks is not a
preferable
>>>> option when one wants to protect against memory corruption
exploitation.
>>>>
>>>> This feature prevents the CFI errors by expanding the valid
vtable sets
>>>> at virtual callsites with vtables of classes specified in a
sanitizer
>>>> blacklist file by the user. This allows keeping the CFI checks
enabled.
>>>>
>>>> When applying CFI to Firefox, I had to use this feature to
solve the
>>>> CFI errors caused by XPCOM in Firefox. XPCOM is a fundamental
technique in
>>>> Firefox and its design is so complex and intricate that
changing XPCOM to
>>>> solve the CFI errors would be very difficult. XPCOM allows
components to be
>>>> written in multiple languages and allows them being used from
different
>>>> languages. For example, components implemented in
JavaScript(JS) can be
>>>> used from C++ through their corresponding classes defined in
C++. However,
>>>> it is worth mentioning that these classes are not implemented
in C++ but in
>>>> JS. Behind the scenes, during runtime a generic proxy class is
used for all
>>>> JS-component classes within the C++ code. This proxy class
leads the
>>>> execution to the JS code.
>>>> When CFI is applied, the CFI checks at virtual callsites that
have the
>>>> type of a JS-component class, fail, because at runtime the
vtable of the
>>>> generic proxy class is used at these virtual callsites, while
there is no
>>>> inheritance link between the JS-component and the generic proxy
class.
>>>>
>>>> With the following patches I was able to specify the links
between
>>>> these classes such that during compilation the vtable of the
generic proxy
>>>> class was added to the vtable sets at virtual callsites with
the type of
>>>> the JS-component classes:
>>>> - https://reviews.llvm.org/D34233
>>>> - https://reviews.llvm.org/D34231
>>>>
>>>> Without these patches, XPCOM would have to be significantly
changed and
>>>> probably written from scratch. Simply making the JS-component
classes a
>>>> descendant of the generic proxy class, or vice versa, is not an
option,
>>>> because this would break the design. Making the generic proxy
class a
>>>> descendant of the JS-component classes would result in a bad
design in
>>>> which the proxy class inherits from tens of classes. Also the
vtable of the
>>>> proxy class should overlay the structure of the JS-component
vtables in a
>>>> very specific way. Making one a descendant of the other will
break the
>>>> overlaying property.
>>>>
>>>> Kind regards,
>>>> Enes
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170616/8e53c6c0/attachment.html>

Well I find the changes necessary to support this exceptionally simple
and obviously safe. All additional risk is on the whitelist user side,
and it does not even compare with blanket whitelisting of libc++ ;)

Having said that, source annotations should be preferred to whitelists
when possible. This could be a class __attribute__ declaring that the
class is layout-compatible with some other class.

On Fri, Jun 16, 2017 at 11:06 AM, Kostya Serebryany <kcc at google.com>
wrote:
> -krasin@
>
> On Fri, Jun 16, 2017 at 11:05 AM, Kostya Serebryany <kcc at
google.com> wrote:
>>
>>
>>
>> On Thu, Jun 15, 2017 at 10:39 PM, Enes Göktaş <enes.goktas at
gmail.com>
>> wrote:
>>>
>>> Hi Kostya,
>>>
>>> Please find attached the minimized motivation test.
>>> I hope it is minimized enough. If not please let me know so I can
try to
>>> make it more minimal.
>>> Were you expecting something like this?
>>>
>>> Also I think the tests that I should provide along with the patch
should
>>> be in a special format right?
>>
>>
>> Yes. Take a look at other tests in llvm/projects/compiler-rt/test/cfi
>>
>> (I did not study your patch or tests in detail yet, and probably
won't
>> have time until mid Jul. But others may)
>>
>> My major concern with any such patch is that it complicates the
>> implementation.
>> For many parts of compiler extra complexity is acceptable, but CFI is a
>> security mitigation feature and as such should be minimal.
>>
>> --kcc
>>
>>>
>>> I think I should be looking at
>>> http://llvm.org/docs/DeveloperPolicy.html#test-cases, and
>>> http://llvm.org/docs/TestingGuide.html for more information for
adding tests
>>> to the patch. Any other handy links by any chance?
>>>
>>> --
>>> Enes
>>>
>>> On Thu, Jun 15, 2017 at 1:51 PM, Kostya Serebryany <kcc at
google.com>
>>> wrote:
>>>>
>>>> Hi Enes,
>>>>
>>>> I usually find it nearly impossible to discuss complex issues
likes this
>>>> w/o having a minimized motivation test.
>>>> Could you please provide such a test with one of the patches?
>>>> (And in general, please try to provide tests with any patch)
>>>>
>>>> Thanks!
>>>>
>>>> --kcc
>>>>
>>>>
>>>> On Thu, Jun 15, 2017 at 5:08 AM, Enes Göktaş <enes.goktas at
gmail.com>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I would like to propose extending the Control-Flow
Integrity (CFI)
>>>>> mechanism in LLVM/Clang with a feature that allows users to
explicitly link
>>>>> classes that have no inheritance link. Usually, if one
class is used at
>>>>> locations in code where this class is not expected, this
will create a CFI
>>>>> error at runtime, assuming the application is built with
CFI enabled.
>>>>> However, in cases where the user has a complex code
structure or design that
>>>>> should allow this behavior, there is currently no solution
but disabling the
>>>>> CFI checks. Disabling the CFI checks is not a preferable
option when one
>>>>> wants to protect against memory corruption exploitation.
>>>>>
>>>>> This feature prevents the CFI errors by expanding the valid
vtable sets
>>>>> at virtual callsites with vtables of classes specified in a
sanitizer
>>>>> blacklist file by the user. This allows keeping the CFI
checks enabled.
>>>>>
>>>>> When applying CFI to Firefox, I had to use this feature to
solve the
>>>>> CFI errors caused by XPCOM in Firefox. XPCOM is a
fundamental technique in
>>>>> Firefox and its design is so complex and intricate that
changing XPCOM to
>>>>> solve the CFI errors would be very difficult. XPCOM allows
components to be
>>>>> written in multiple languages and allows them being used
from different
>>>>> languages. For example, components implemented in
JavaScript(JS) can be used
>>>>> from C++ through their corresponding classes defined in
C++. However, it is
>>>>> worth mentioning that these classes are not implemented in
C++ but in JS.
>>>>> Behind the scenes, during runtime a generic proxy class is
used for all
>>>>> JS-component classes within the C++ code. This proxy class
leads the
>>>>> execution to the JS code.
>>>>> When CFI is applied, the CFI checks at virtual callsites
that have the
>>>>> type of a JS-component class, fail, because at runtime the
vtable of the
>>>>> generic proxy class is used at these virtual callsites,
while there is no
>>>>> inheritance link between the JS-component and the generic
proxy class.
>>>>>
>>>>> With the following patches I was able to specify the links
between
>>>>> these classes such that during compilation the vtable of
the generic proxy
>>>>> class was added to the vtable sets at virtual callsites
with the type of the
>>>>> JS-component classes:
>>>>> - https://reviews.llvm.org/D34233
>>>>> - https://reviews.llvm.org/D34231
>>>>>
>>>>> Without these patches, XPCOM would have to be significantly
changed and
>>>>> probably written from scratch. Simply making the
JS-component classes a
>>>>> descendant of the generic proxy class, or vice versa, is
not an option,
>>>>> because this would break the design. Making the generic
proxy class a
>>>>> descendant of the JS-component classes would result in a
bad design in which
>>>>> the proxy class inherits from tens of classes. Also the
vtable of the proxy
>>>>> class should overlay the structure of the JS-component
vtables in a very
>>>>> specific way. Making one a descendant of the other will
break the overlaying
>>>>> property.
>>>>>
>>>>> Kind regards,
>>>>> Enes
>>>>
>>>>
>>>
>>
>

As you noted the class links are actually of the whitelisting kind and not
of the blacklisting kind. Doing this with an attribute is pretty
interesting and sounds like a better fit to me.
I think this could look something like:

__attribute__((compatible_vtable_layout("JSComponentMath",
"JSComponentImage")))
class ProxyClass{
public:
  ...
}

Would this be more admissible?


On Fri, Jun 16, 2017 at 8:55 PM, Evgenii Stepanov <eugenis at google.com>
wrote:
>
> Well I find the changes necessary to support this exceptionally simple
> and obviously safe. All additional risk is on the whitelist user side,
> and it does not even compare with blanket whitelisting of libc++ ;)
>
> Having said that, source annotations should be preferred to whitelists
> when possible. This could be a class __attribute__ declaring that the
> class is layout-compatible with some other class.
>
> On Fri, Jun 16, 2017 at 11:06 AM, Kostya Serebryany <kcc at
google.com>
wrote:
> > -krasin@
> >
> > On Fri, Jun 16, 2017 at 11:05 AM, Kostya Serebryany <kcc at
google.com>
wrote:
> >>
> >>
> >>
> >> On Thu, Jun 15, 2017 at 10:39 PM, Enes Göktaş <enes.goktas at
gmail.com>
> >> wrote:
> >>>
> >>> Hi Kostya,
> >>>
> >>> Please find attached the minimized motivation test.
> >>> I hope it is minimized enough. If not please let me know so I
can try
to
> >>> make it more minimal.
> >>> Were you expecting something like this?
> >>>
> >>> Also I think the tests that I should provide along with the
patch
should
> >>> be in a special format right?
> >>
> >>
> >> Yes. Take a look at other tests in
llvm/projects/compiler-rt/test/cfi
> >>
> >> (I did not study your patch or tests in detail yet, and probably
won't
> >> have time until mid Jul. But others may)
> >>
> >> My major concern with any such patch is that it complicates the
> >> implementation.
> >> For many parts of compiler extra complexity is acceptable, but CFI
is a
> >> security mitigation feature and as such should be minimal.
> >>
> >> --kcc
> >>
> >>>
> >>> I think I should be looking at
> >>> http://llvm.org/docs/DeveloperPolicy.html#test-cases, and
> >>> http://llvm.org/docs/TestingGuide.html for more information
for
adding tests
> >>> to the patch. Any other handy links by any chance?
> >>>
> >>> --
> >>> Enes
> >>>
> >>> On Thu, Jun 15, 2017 at 1:51 PM, Kostya Serebryany <kcc at
google.com>
> >>> wrote:
> >>>>
> >>>> Hi Enes,
> >>>>
> >>>> I usually find it nearly impossible to discuss complex
issues likes
this
> >>>> w/o having a minimized motivation test.
> >>>> Could you please provide such a test with one of the
patches?
> >>>> (And in general, please try to provide tests with any
patch)
> >>>>
> >>>> Thanks!
> >>>>
> >>>> --kcc
> >>>>
> >>>>
> >>>> On Thu, Jun 15, 2017 at 5:08 AM, Enes Göktaş
<enes.goktas at gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> I would like to propose extending the Control-Flow
Integrity (CFI)
> >>>>> mechanism in LLVM/Clang with a feature that allows
users to
explicitly link
> >>>>> classes that have no inheritance link. Usually, if one
class is
used at
> >>>>> locations in code where this class is not expected,
this will
create a CFI
> >>>>> error at runtime, assuming the application is built
with CFI
enabled.
> >>>>> However, in cases where the user has a complex code
structure or
design that
> >>>>> should allow this behavior, there is currently no
solution but
disabling the
> >>>>> CFI checks. Disabling the CFI checks is not a
preferable option
when one
> >>>>> wants to protect against memory corruption
exploitation.
> >>>>>
> >>>>> This feature prevents the CFI errors by expanding the
valid vtable
sets
> >>>>> at virtual callsites with vtables of classes specified
in a
sanitizer
> >>>>> blacklist file by the user. This allows keeping the
CFI checks
enabled.
> >>>>>
> >>>>> When applying CFI to Firefox, I had to use this
feature to solve the
> >>>>> CFI errors caused by XPCOM in Firefox. XPCOM is a
fundamental
technique in
> >>>>> Firefox and its design is so complex and intricate
that changing
XPCOM to
> >>>>> solve the CFI errors would be very difficult. XPCOM
allows
components to be
> >>>>> written in multiple languages and allows them being
used from
different
> >>>>> languages. For example, components implemented in
JavaScript(JS)
can be used
> >>>>> from C++ through their corresponding classes defined
in C++.
However, it is
> >>>>> worth mentioning that these classes are not
implemented in C++ but
in JS.
> >>>>> Behind the scenes, during runtime a generic proxy
class is used for
all
> >>>>> JS-component classes within the C++ code. This proxy
class leads the
> >>>>> execution to the JS code.
> >>>>> When CFI is applied, the CFI checks at virtual
callsites that have
the
> >>>>> type of a JS-component class, fail, because at runtime
the vtable
of the
> >>>>> generic proxy class is used at these virtual
callsites, while there
is no
> >>>>> inheritance link between the JS-component and the
generic proxy
class.
> >>>>>
> >>>>> With the following patches I was able to specify the
links between
> >>>>> these classes such that during compilation the vtable
of the
generic proxy
> >>>>> class was added to the vtable sets at virtual
callsites with the
type of the
> >>>>> JS-component classes:
> >>>>> - https://reviews.llvm.org/D34233
> >>>>> - https://reviews.llvm.org/D34231
> >>>>>
> >>>>> Without these patches, XPCOM would have to be
significantly changed
and
> >>>>> probably written from scratch. Simply making the
JS-component
classes a
> >>>>> descendant of the generic proxy class, or vice versa,
is not an
option,
> >>>>> because this would break the design. Making the
generic proxy class
a
> >>>>> descendant of the JS-component classes would result in
a bad design
in which
> >>>>> the proxy class inherits from tens of classes. Also
the vtable of
the proxy
> >>>>> class should overlay the structure of the JS-component
vtables in a
very
> >>>>> specific way. Making one a descendant of the other
will break the
overlaying
> >>>>> property.
> >>>>>
> >>>>> Kind regards,
> >>>>> Enes
> >>>>
> >>>>
> >>>
> >>
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170619/90c35721/attachment-0001.html>