Opened 4 months ago

Closed 4 months ago

#21290 closed defect (implemented)

Rename and/or redocument --enable-expensive-hardening

Reported by: nickm Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The hardening from "--enable-expensive-hardening" is not only expensive, it is also fragile.

The compiler options that "--enable-exensive-hardening" uses (asan, ubsan, trapv) work by converting different classes of C bugs into program crashes. If those bugs are remotely triggerable, we replace them with denial-of-service bugs.

Some possible underlying bugs here are actually harmless -- like the integer bug in TROVE-2017-001, or the read-one-extra-byte bug of TROVE-2016-12-002. So long as any bugs like these bugs exist, "expensive-hardening" will make your Tor more vulnerable to remote denial of service.

But some possible underlying bugs are potential trouble -- like if we had an actual stack overflow bug or a heap overflow bug. "expensive-hardening" can replace some of these with aborts too. So long as any bugs like these bugs exist, "expensive-hardening" makes it a little more difficult to do RCE or heartbleed-style leaks against your Tor.

The first kind of bug seems much more common in practice over Tor's history. But the impact of the second kind of bug would be significantly worse.

So using "--enable-expensive-hardening" in production means "Make me much more vulnerable to remote DoS, but (maybe? probably?) less vulnerable to RCE or heartbleed."

We should document this, and maybe rename the option, so that it's clear that CPU cycles aren't the only issue.

Child Tickets

Change History (6)

comment:1 Changed 4 months ago by cypherpunks

This is also a good time to reintroduce http://www.openwall.com/lists/oss-security/2016/02/17/9.

comment:2 Changed 4 months ago by nickm

--enable-fragile-hardening

comment:3 Changed 4 months ago by nickm

(The old option should still _work_, though. Either one should produce a message.)

comment:4 Changed 4 months ago by dgoulet

  • Owner set to dgoulet
  • Status changed from new to accepted

comment:5 Changed 4 months ago by dgoulet

  • Status changed from accepted to needs_review

See branch bug21290_030_01.

comment:6 Changed 4 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

Merged!

Note: See TracTickets for help on using tickets.