Opened 20 months ago

Last modified 13 months ago

#23357 needs_revision enhancement

Build with non-Cross-DSO CFI

Reported by: shawn.webb Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: security, defence-in-depth, 033-triage-20180320, 033-removed-20180320
Cc: Actual Points:
Parent ID: Points: 1.0
Reviewer: Sponsor:

Description

Control Flow Integrity is an exploit mitigation. clang/llvm has a CFI implementation that can be used on hardened operating systems like HardenedBSD.

When lld is the compiler, non-Cross-DSO CFI from clang/llvm can be used. I've tested this on HardenedBSD with a very basic configuration.

Attached is a candidate patch for enabling non-Cross-DSO CFI for the core Tor application.

Child Tickets

Attachments (2)

tor-cfi-r01.patch (1.8 KB) - added by shawn.webb 20 months ago.
tor-cfi-r02.patch (7.7 KB) - added by shawn.webb 20 months ago.

Download all attachments as: .zip

Change History (27)

Changed 20 months ago by shawn.webb

Attachment: tor-cfi-r01.patch added

comment:1 Changed 20 months ago by teor

Keywords: security defence-in-depth added
Milestone: Tor: 0.3.2.x-final
Points: 1.0
Status: newneeds_revision

Thanks for this patch!

I think we might want this enabled for our debug and development builds. So we might want to add it to --enable-fragile-hardening.

Here is a quick review:

Is there a way of enabling this feature if it's supported by the compiler, but disabling it otherwise?

Why do we only enable this feature for tor itself?
I think it would also be useful to have it enabled for:

  • fuzzing
  • unit tests

Should this line say +=?

src_or_tor_CFLAGS = -flto -fvisibility=hidden -fsanitize=cfi

comment:2 Changed 20 months ago by shawn.webb

CFLAGS isn't set by that point, so autoconf will complain with an error that += was used instead of =.

I would probably leave this as a separate flag, given this needs both explicit compiler and linker support. CFI is still under development by the clang/llvm folks, too.

As an FYI, I've committed this patch to the HardenedBSD ports tree in case anyone wants to play with it on HardenedBSD 12-CURRENT/amd64. I'm about to deploy it on my public relay, which also serves as a transparent proxy for my entire home LAN.

comment:3 Changed 20 months ago by teor

I still think we should enable this while fuzzing and while unit testing.
It would be embarrassing to release a remote code execution that we could have detected using these options while fuzzing, and even more so for unit tests.

comment:4 Changed 20 months ago by shawn.webb

Agreed. Should we keep --enable-cfi, though? What do you think would be the best approach?

comment:5 Changed 20 months ago by teor

We could make --enable-cfi enable CFI across the entire Tor codebase and tests and everything else we compile at the same time.

I can't see why we'd want to selectively disable it, unless there's code that does some really weird things to the stack.

comment:6 Changed 20 months ago by shawn.webb

I wouldn't enable CFI across the entire codebase right now. This patch is specifically for non-Cross-DSO CFI.

Cross-DSO CFI requires a whole heck of a lot more work, including explicit support from the RTLD. Once I get Cross-DSO CFI support working in HardenedBSD (an ongoing effort), I'll likely submit a follow-up patch here.

I don't know of any operating system today that supports Cross-DSO CFI.

comment:7 Changed 20 months ago by teor

Tor doesn't use DSOs for its fuzzing and unit testing code, only for dependent libraries.
I've just checked, and there are no DSOs (dylib, so) in my build tree, nor any in my local tor install.

So can we please enable this across all the code compiled by tor's configure?

comment:8 Changed 20 months ago by shawn.webb

"DSO" is a generic term. For Windows, DSO means ".dll". For *nix, it means ".so". For OSX, it means ".dylib".

DSO stands for "Dynamic Shared Object." Just a fancy way to genericize the term for a .dll, .so, .dylib, etc.

Tor does use DSOs. :)

comment:9 Changed 20 months ago by shawn.webb

$ ldd src/or/tor
src/or/tor:

libz.so.6 => /lib/libz.so.6 (0x50c47ffd000)
libm.so.5 => /lib/libm.so.5 (0x50c06cb9000)
libevent-2.1.so.6 => /usr/local/lib/libevent-2.1.so.6 (0x50c48018000)
libssl.so.43 => /usr/lib/libssl.so.43 (0x50c48267000)
libcrypto.so.41 => /lib/libcrypto.so.41 (0x50c482c7000)
liblzma.so.5 => /usr/lib/liblzma.so.5 (0x50c484b1000)
libthr.so.3 => /lib/libthr.so.3 (0x50c06c87000)
libexecinfo.so.1 => /usr/lib/libexecinfo.so.1 (0x50c484dc000)
libc.so.7 => /lib/libc.so.7 (0x50c06ce9000)
libelf.so.2 => /lib/libelf.so.2 (0x50c484e1000)
libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x50c484fb000)

comment:10 in reply to:  1 Changed 20 months ago by teor

I'm going to drop the discussion about DSOs, because it doesn't answer my question, or move us towards getting this patch revised and merged.

Here is my original question:

Replying to teor:

Why do we only enable this feature for tor itself?
I think it would also be useful to have it enabled for:

  • fuzzing
  • unit tests

Let me try to clarify what I meant:

When we configure with a recent clang/llvm and --enable-cfi, then compile .o files and link them together into the tor binary, does non-Cross-DSO CFI work for that binary?

If so, how do we get non-Cross-DSO CFI working for the other binaries that the tor makefiles generate? In particular, how can we get them working for the test and fuzz_* binaries?

Is it simpler just to enable non-Cross-DSO CFI for all the binaries that the tor makefiles generate, so we don't miss any?

comment:11 Changed 20 months ago by shawn.webb

When we configure with a recent clang/llvm and --enable-cfi, then compile .o files and link them together into the tor binary, does non-Cross-DSO CFI work for that binary?

Yup. One problem with trying to use CFI with both static and dynamic libraries is that you need to use llvm-ar, llvm-nm, and llvm-objdump as your ar, nm, and objdump applications. This is because compiling with CFI will cause clang to output intermediate object files as LLVM bitcode files instead of ELF object files. The ar, nm, and objdump applications that come on most (all?) operating systems only support ELF. Essentially, the whole compiler toolchain must be switched over to the entire llvm tool suite.

So, what I can do, is expand the patch to apply the CFLAGS and LDFLAGS to more of the applications (rather than just tor). This way, we skip applying CFI to the library code (even though the libraries in the codebase get statically linked).

comment:12 in reply to:  11 Changed 20 months ago by teor

Replying to shawn.webb:

CFLAGS isn't set by that point, so autoconf will complain with an error that += was used instead of =.

Is CFLAGS ever set after this?
Because if it is, it will overwrite the CFLAGS you just set.

Replying to shawn.webb:

So, what I can do, is expand the patch to apply the CFLAGS and LDFLAGS to more of the applications (rather than just tor). This way, we skip applying CFI to the library code (even though the libraries in the codebase get statically linked).

Please make this change and re-submit a patch or git branch.

Changed 20 months ago by shawn.webb

Attachment: tor-cfi-r02.patch added

comment:13 Changed 20 months ago by shawn.webb

I've just attached a new revision of the patch which enabled non-Cross-DSO CFI to more bits, including the unit test applications. Please let me know if I've forgotten something.

comment:14 Changed 20 months ago by shawn.webb

As an update: I've been running a public Tor relay with non-Cross-DSO CFI enabled for the Tor daemon for nearly a week now on HardenedBSD 12-CURRENT/amd64 without issues. No meaningful or measurable performance decrease.

comment:15 Changed 20 months ago by cypherpunks

Is there something that prevents building Tor statically for your build?

comment:16 Changed 19 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

Defer all needs_revision non-spec enhancements to 0.3.3.

comment:17 Changed 19 months ago by shawn.webb

We at HardenedBSD have now dropped this patch in our ports tree in favor of CFI applied to the entire codebase. We're actively working on replacing the elftoolchain utilities (objdump, nm, ar, and ranlib) with the llvm equivalents. We're almost there and expect to be there by the end of this week.

I've done basic testing with Tor 0.3.1.7 compiled with CFI applied to the entire codebase with success. My testing consisted of running Tor with the stock config and ensuring it can connect to the Tor network and establish a circuit.

Once HardenedBSD 12-CURRENT officially makes the switch to the full llvm toolchain, I will do an expanded test with my Tor-ified network.

If upstream Tor decides to drop this patch, I would actually be in favor of it. Due to the majority of the code being in statically-linked internal libraries, this patch doesn't amount to much.

Regardless, I will keep this bug report open and report back with my results regarding CFI.

comment:18 Changed 19 months ago by teor

We'd happily take a patch for a configure option that activates CFI on everything that's compiled using Tor's configure.
(Assuming an appropriate version of llvm.)

comment:19 Changed 19 months ago by shawn.webb

That's unneeded, actually. Since configure honors external CFLAGS and LDFLAGS, users can simply set the required CFLAGS and LDFLAGS. That's what we're now doing in the HardenedBSD ports tree for Tor.

To see the CFLAGS and LDFLAGS that get applied, take look here: https://github.com/HardenedBSD/hardenedbsd-ports/blob/master/Mk/Uses/cfi.mk

In the HardenedBSD ports tree, we make sure that the proper llvm toolchain bits are there: https://github.com/HardenedBSD/hardenedbsd-ports/blob/master/Mk/bsd.hardening.mk#L234-L239

comment:20 Changed 19 months ago by teor

I'm asking for a convenience option to configure which activates the right set of flags.
Then if it works well, we can enable it as part of --enable-fragile-hardening, if the compiler supports those options.

comment:21 Changed 19 months ago by shawn.webb

I'll take a look. Given the recent findings above, this is now lower priority for me.

comment:22 Changed 15 months ago by nickm

any progress here?

comment:23 Changed 13 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:24 Changed 13 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:25 Changed 13 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

Note: See TracTickets for help on using tickets.