1046600 - Investigate if XPConnect can be made compatible with VTV; or if VTV supports whitelisting

Status: NEW

(Core :: XPConnect, defect)

Description

[Georg Koppen]

Virtual Table Verification (VTV) is a new feature in GCC 4.9.0 to help making C++ virtual method dispatch more secure. Compiling Firefox and running it afterwards (doing a plain |firefox --help|) gives a crash with the following backtrace:

#0  0x00007ffff7ff0380 in __vtv_verify_fail(void**, void const*)@plt ()
   from /home/gk/asan/gcc-4.9.0debug/usr/local/lib64/libvtv.so.0
#1  0x00007ffff7ff1f2c in __VLTVerifyVtablePointer (
    set_handle_ptr=0x7ffff58c2c50 <_VTV<xpcIJSGetFactory>::__vtable_map>, 
    vtable_ptr=0x7ffff52fa890 <vtable for nsXPTCStubBase+16>)
    at ../../../libvtv/vtv_rts.cc:1351
#2  0x00007fffeea019a6 in mozJSComponentLoader::ModuleEntry::GetFactory (
    module=..., entry=...)
    at /home/gk/asan/mozilla-central/js/xpconnect/loader/mozJSComponentLoader.cpp:1440
#3  0x00007fffee101e4d in nsFactoryEntry::GetFactory (this=0x7fffe5d77340)
    at /home/gk/asan/mozilla-central/xpcom/components/nsComponentManager.cpp:1786
#4  0x00007fffee100362 in nsComponentManagerImpl::CreateInstanceByContractID (
    this=0x7ffff6e9a360, 
    aContractID=0x7fffe2dfe760 "@mozilla.org/browser/webide-clh;1", aDelegate=0x0, 
    aIID=..., aResult=0x7fffffffcb20)
    at /home/gk/asan/mozilla-central/xpcom/components/nsComponentManager.cpp:1080
#5  0x00007fffee100e46 in nsComponentManagerImpl::GetServiceByContractID (
    this=0x7ffff6e9a360, 
    aContractID=0x7fffe2dfe760 "@mozilla.org/browser/webide-clh;1", aIID=..., 
    result=0x7fffffffcc58)
    at /home/gk/asan/mozilla-central/xpcom/components/nsComponentManager.cpp:1440
#6  0x00007fffee1438e0 in CallGetService (
    aContractID=0x7fffe2dfe760 "@mozilla.org/browser/webide-clh;1", aIID=..., 
    aResult=0x7fffffffcc58)
    at /home/gk/asan/mozilla-central/xpcom/glue/nsComponentManagerUtils.cpp:69

For detailed steps to reproduce this and the findings associated with it see:

https://trac.torproject.org/projects/tor/ticket/12427#comment:5

Comment 1

[Georg Koppen]

Information about VTV can be found on https://gcc.gnu.org/wiki/vtv

Comment 2

[Nathan Froyd [:froydnj]]

From the VTV feature proposal doc:

"For any given object, the set of legal vtables to which it can point can be derived based on inheritance relationship information that the compiler can collect at compile time.  Therefore we will have the compiler collect this information by building a set of mappings from the base classes to derived classes, and recording all the vtable pointers for all these classes.  At run time these mappings will be compiled into data structures that indicate, for every potential polymorphic base class (where “polymorphic” means a class that needs a virtual table), the complete set of valid vtable pointers that an object declared to be of that base class might legally contain.  At each virtual call site we will insert a call to a verification function before we dereference the vtable pointer for the virtual call.  The verification function will take an indicator of the statically declared type of the object and the actual vtable pointer in the object.  It will use the data structure previously mentioned to verify that the vtable pointer in the object is a valid vtable pointer for the declared type."

I don't think this is going to fly at all with the vtable hacking we do to make C++-to-JS function calls work.

Comment 3

[Tom Tromey :tromey]

One way to deal with the xptcall problem is to provide our own definition of __vtv_verify_fail.
This is allowed according to https://docs.google.com/document/d/1vc-npuLRxe3Ihzi3is9m8XNmFaDbGi1ewJDF3alCRAA/pub

A comment in the source says:

  __vtv_verify_fail is defined in
   such a way that it can be replaced by a programmer, if desired.  It
   is the function that __VLTVerifyVtablePointer calls if it can't
   find the pointer in the data set.  Allowing the programmer to
   overwrite this function means that he/she can do some alternate
   verification, including NOT failing in certain specific cases, if
   desired.  This may be the case if the programmer has to deal wtih
   unverified third party software, for example.  __vtv_really_fail is
   available for the programmer to call from his version of
   __vtv_verify_fail, if he decides the failure is real.

The interface looks like:

/* Send information about what we were trying to do when verification
   failed to the error log, then call vtv_fail.  This function can be
   overwritten/replaced by the user, to implement a secondary
   verification function instead.  DATA_SET_PTR is the vtable map
   variable used for the failed verification, and VTBL_PTR is the
   vtable pointer that was not found in the set.  */

void
__vtv_verify_fail (void **data_set_ptr, const void *vtbl_ptr)


So, I think the idea would be to check vtbl_ptr against the nsXPTCStubBase vtable pointer,
and allow the use if they are equal.

Comment 4

[Tom Tromey :tromey]

Jakub (a GCC developer) also pointed out that there's -fsanitize=vptr, which includes more checks.
However it isn't clear to me that this can handle the sort of white-listing we would need.

Comment 5

[Tom Tromey :tromey]

I tried this today and couldn't get it to work.
It seems libvtv isn't calling the local version of the function.
Anyway FWIW here's what I did:

diff --git a/xpcom/reflect/xptcall/xptcall.cpp b/xpcom/reflect/xptcall/xptcall.cpp
index 1ea0ac34dbc6..5f24f9ded036 100644
--- a/xpcom/reflect/xptcall/xptcall.cpp
+++ b/xpcom/reflect/xptcall/xptcall.cpp
@@ -80,3 +80,23 @@ NS_SizeOfIncludingThisXPTCallStub(const nsISomeInterface* aStub,
     // so just measure the size of the object itself.
     return aMallocSizeOf(aStub);
 }
+
+NS_EXPORT void
+__vtv_verify_fail(void **data_set_ptr, const void *vtbl_ptr)
+{
+    static void* stubVtable;
+
+    if (stubVtable == nullptr) {
+        nsXPTCStubBase* stub = new nsXPTCStubBase(nullptr, nullptr);
+        void** objPtr = static_cast<void**>(static_cast<void*>(stub));
+        stubVtable = *objPtr;
+        delete stub;
+    }
+
+    if (vtbl_ptr == stubVtable) {
+        return;
+    }
+
+    fprintf(stderr, "*** Unable to verify vtable pointer (%p) in set (%p) *** \n",
+            vtbl_ptr, data_set_ptr);
+}

Comment 6

[Nathan Froyd [:froydnj]]

The function should be marked `extern "C" NS_EXPORT` so that the linkage is correct.  Does that change help at all?

Comment 7

[Tom Tromey :tromey]

(In reply to Nathan Froyd [:froydnj] (doing reviews while in Austin) from comment #6)
> The function should be marked `extern "C" NS_EXPORT` so that the linkage is
> correct.  Does that change help at all?

We talked in person but for the record, I tried that first, then found out to my surprise
that the function in question is in c++ and not extern "C":

pokyo. nm libvtv.so|grep vtv_verify_fail
00000000000060e0 T _Z17__vtv_verify_failPPvPKv


Nathan suggested then that perhaps the function needs to be in the main executable and not in libxul.so.

Comment 8

[Tom Tromey :tromey]

This seems to do better:

diff --git a/browser/app/nsBrowserApp.cpp b/browser/app/nsBrowserApp.cpp
index 022fbaed4586..fcaf4b3467c9 100644
--- a/browser/app/nsBrowserApp.cpp
+++ b/browser/app/nsBrowserApp.cpp
@@ -14,6 +14,7 @@
 #include <sys/resource.h>
 #include <unistd.h>
 #endif
+#include <dlfcn.h>
 
 #include <stdio.h>
 #include <stdarg.h>
@@ -252,6 +253,23 @@ InitXPCOMGlue()
   return NS_OK;
 }
 
+NS_EXPORT void
+__vtv_verify_fail(void **data_set_ptr, const void *vtbl_ptr)
+{
+  static void* savedVtable;
+
+  if (savedVtable == nullptr) {
+    savedVtable = dlsym(RTLD_DEFAULT, "_ZTV14nsXPTCStubBase");
+  }
+
+  if (vtbl_ptr == savedVtable) {
+    return;
+  }
+
+  fprintf(stderr, "*** Unable to verify vtable pointer (%p) in set (%p) *** \n",
+          vtbl_ptr, data_set_ptr);
+}
+
 int main(int argc, char* argv[], char* envp[])
 {
   mozilla::TimeStamp start = mozilla::TimeStamp::Now();

Comment 9

[Tom Tromey :tromey]

I'm still seeing failures printed, haven't investigated yet.
It's possible that the dlsym hack just isn't working, but at least this
shows errors without crashing.  So far all the failures have the same vtable:

*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233ae4278) *** 
*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233adc010) *** 
*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233adf8a8) *** 
*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233adf8c0) *** 
*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233ae4278) *** 
*** Unable to verify vtable pointer (0x7ff2334c1a10) in set (0x7ff233adc010) ***

Comment 10

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #9)

> It's possible that the dlsym hack just isn't working, but at least this
> shows errors without crashing.

That is in fact the problem: dlsym is always returning NULL here.

Comment 11

[Tom Tromey :tromey]

Just to be clear, I'm not really working on this, I was just poking at it for fun around Austin.
There I discussed the problem with a few people and I figured it would be good to write up some
conclusions.

White-listing would certainly work for VTV.  For this to happen you could use the patch from
comment #8, but you would need an additional patch to export the vtable symbol from libxul.
Currently it is not:

pokyo. nm obj-x86_64-pc-linux-gnu/dist/bin/libxul.so | grep _ZTV14nsXPTCStubBase
0000000007519a40 d _ZTV14nsXPTCStubBase

This seemed to work ok but I don't know if it is ok on all platforms:

diff --git a/xpcom/reflect/xptcall/xptcprivate.h b/xpcom/reflect/xptcall/xptcprivate.h
index 4429ee47dd5b..62109440c2b9 100644
--- a/xpcom/reflect/xptcall/xptcprivate.h
+++ b/xpcom/reflect/xptcall/xptcprivate.h
@@ -39,7 +39,7 @@ public:
 
 #define SENTINEL_ENTRY(n) NS_IMETHOD Sentinel##n() override;
 
-class nsXPTCStubBase final : public nsIXPTCStubBase
+class MOZ_EXPORT nsXPTCStubBase final : public nsIXPTCStubBase
 {
 public:
     NS_DECL_ISUPPORTS_INHERITED


A more robust way of dealing with this issue would be to get rid of the nsXPTCStubBase hack entirely.
The idea here would be to change the build so that, for any IDL interface that can be implemented in JS,
emit a concrete subclass that implements the methods by calling NS_InvokeByIndex.  This idea is complicated
by the way that stubs are currently generated, e.g.:

https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp#131-216

So, this would be a fair amount of work; but the benefit would be that it would work with all vtable-checking
mechanisms, because it would not be playing tricks on the compiler.

Comment 12

[Tom Tromey :tromey]

Nathan and I talked about this for quite a while in SF and there are two new developments.

First, we found out that the linkage name problem can be dealt with by using the
GCC "asm labels" feature:
  https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/Asm-Labels.html#Asm-Labels
This seems to work for virtual methods for both gcc and clang.
This makes the plan from comment #11 a bit more practical to implement.

Second, now that rust can implement xpcom interfaces, there will be a need to extend
vtable checking to rust.  Maybe this can be done using a similar idea, though it's
worth noting that rust also has vtables in trait objects, and if these are used in
firefox then adding some kind of CFI support to the rust compiler would be desirable.

Comment 13

[Tom Tromey :tromey]

Bug 1302891 is related.

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Tom Tromey :tromey from comment #13)
> Bug 1302891 is related.

Specifically Bug 1478061 for cfi-vcall, although there isn't a sub-bug directly for "Get clang to not be mad at our weird vtable stuff".  I'm not sure if that will take the form of a local patch we carry at Mozilla or something we can upstream.