Open
Bug 1046600
Opened 10 years ago
Updated 2 years ago
Investigate if XPConnect can be made compatible with VTV; or if VTV supports whitelisting
Categories
(Core :: XPConnect, defect)
Tracking
()
NEW
People
(Reporter: gk, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: sec-want)
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
Reporter | ||
Comment 1•10 years ago
|
||
Information about VTV can be found on https://gcc.gnu.org/wiki/vtv
Comment 2•10 years ago
|
||
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.
Updated•7 years ago
|
Summary: Crash in mozJSComponentLoader::ModuleEntry::GetFactory when compiled with GCC 4.9.0 and VTV → Investigate if XPConnect can be made compatible with VTV; or if VTV supports whitelisting
Whiteboard: [sec-want]
Comment 3•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
The function should be marked `extern "C" NS_EXPORT` so that the linkage is correct. Does that change help at all?
Flags: needinfo?(ttromey)
Comment 7•6 years ago
|
||
(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.
Flags: needinfo?(ttromey)
Comment 8•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Bug 1302891 is related.
Comment 14•6 years ago
|
||
(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.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•