Open Bug 510629 (cfi) Opened 15 years ago Updated 2 years ago

[meta] Ship Control Flow Integrity (CFI)

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: brendan, Unassigned)

References

(Depends on 6 open bugs, Blocks 1 open bug,
URL
)

Details

(Keywords: meta, parity-chrome, sec-want)

The URL links to a paper that uses points-to and costs 5-10% at runtime. No good for us. But could we do better? We've discussed by email this idea: Copy all vtbls to a part of the virtual address space mapped so that we can test quickly (say, "is bit 25 set?") whether a call is going to one of our vtbls, and insert a check -- or just force bit 25 to be set -- in all vitual method calls. We'd have to mangle our constructors to "correct" the vptr(s) in the new instance, or hack into our C++ compilers. We would have to put the XPConnect stub vtbl in the mapped address space. This is not CFI, because an attacker could still force a method call to go to the wrong method body through the wrong vtbl. But it might be helpful. First thing to do: analyze the callgraph (bug 507711) and see how many virtual method callsites are monomorphic (noscript method / non-scriptable interface, only one implementation -- ripe for deCOMtamination but we may still have a bunch of these kinds of callsites), and if not, for how many virtual method callsites can we statically bound the polymorphism (ignoring XPConnect). For XPConnect we can afford to do a CFI check, I claim, since we've quickstubbed the hot DOM paths and are going to JIT-optimize further. For the monomorphic sites, if any (I bet we have too many still), can we simply deCOMtaminate, that is, devirtualize the callsite? Or even inline if appropriate. For the polymorphic virtual method callsites, perhaps we can afford to add a full CFI check as developed in the paper at the URL. Of course the NX bit is important, but as the paper argues, it's not enough. And we have to write code that becomes executable from the JIT. So it seems worth trying for a systematic CFI enforcement mechanism, especially if we can do some enforcement (or just deCOMtamination) statically. /be
I wonder if we already have incomplete but possibly helpful existing abstraction that could be extended to enforce some kind of CFI: nsCOMPtr. Is there template or other C++ magic we could use with nsCOMPtr::operator-> to check the address of the method being called? /be
Note that we have monomorphic virtual methods that are virtual just so they can be called across module boundaries...
Those are ripe for optimization too! Dynamic libraries, another '90s boondoggle :-P. /be
Depends on: 1302891
Product: Core → Firefox Build System
A note; -fwhole-program-vtables should help with perf.
Okay; I'm going to take this bug over. The intent of this bug is to capture the process of creating a CFI build and then trying to improve its performance enough until it can be shipped.
Assignee: nobody → tom
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=cfg
Summary: Investigate CFI (Control Flow Integrity) for Mozilla code → [meta] Ship Control Flow Integrity (CFI)
Alias: cfi
Depends on: 1457482
Depends on: 1459314
Depends on: 1459624
Depends on: 1459626
Depends on: 1464193
Depends on: 1465549
No longer depends on: 1459314, 1459626
No longer depends on: 1459624
Depends on: 1465863
Depends on: 1468382
Keywords: meta
Priority: -- → P3

Interesting that Chrome seems to have put a major effort into this recently.
There is a lot here, on this page and by following links:
https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-integrity
They use Clang's CFI: https://clang.llvm.org/docs/ControlFlowIntegrity.html

Follow-up: According to the link https://www.chromium.org/developers/testing/control-flow-integrity CFI is now enabled on "the official Chrome on Linux x86-64 (M68 and newer)", has enabled discovery of a large list of bugs (same link), and the direct CPU overhead is negligible, while compiled code size has grown manageably:
"Overhead (only tested on x64)

  • CPU overhead is < 1%
  • Code size overhead is 5%-9%
  • RAM overhead is a small constant (read-only tables inside the binary shared between all chrome processes)"
Assignee: tom → nobody
Type: defect → enhancement
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
Depends on: 1810099
Depends on: 1810096
Depends on: 1809880

I've been trying to get cfi to work, here as some of my observations:

  • clang insists on the -fvisibility= argument (for non-windows builds) despite the visibility being managed in other ways. Bypassing this check requires a minor clang patch.
  • the js-shell works with all cfi-schemes (some minor patches are necessary)
  • {cfi-derived-cast, cfi-nvcall, cfi-mfcall}: work on the full browser with some minor hiccups
  • cfi-icall: Seems to require major changes as all calls to dynamically resolved functions pointers dlym/GetProcAddress must be adapted.
  • cfi-vcall: Currently, the lto-visibility of xpcom class is set to hidden, which causes clang to emit vcall checks for calls to xpcom objects. However, because these classes aren't properly visible to clang, the checks will fails at runtime. Setting the lto-visibility of these classes to public seems like a minor change as one can change the code emitted by xpcom/build/Services.py and xpcom/idl-parser/xpidl/header.py. However, the code base contains plenty of forwards-declarations of xpcom classes (e.g., https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionsParent.h#12). These forward-declarations reset the lto-visibility to hidden, hence (failing) vcall checks are emitted. I rewrote the code base with a small script, wrapping the forward-declarations of xpcom classes such that the lto-visibility remains public. This works, but (1) the number of changed source files is rather large (>700) and (2) there might be a better approach altogether.
Depends on: 1812001
No longer depends on: 1812001
You need to log in before you can comment on or make changes to this bug.