Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#12516 closed enhancement (fixed)

Compile Tor Browser with -fwrapv/-fno-strict-overflow

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, tbb-gitian, tbb-hardened, TorBrowserTeam201512R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should compile Tor Browser with -fwrapv/-fno-strict-overflow to tell the compiler that signed integer overflow is defined behavior.

See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1031653
http://www.airs.com/blog/archives/120

Child Tickets

Change History (10)

comment:1 Changed 5 years ago by erinn

Component: Tor bundles/installationTor Browser
Keywords: tbb-gitian added
Owner: changed from erinn to tbb-team

comment:2 Changed 5 years ago by gk

Keywords: gitian removed

I think we want -fwrapv here see: http://pdos.csail.mit.edu/papers/ub:apsys12.pdf

comment:3 Changed 4 years ago by gk

Keywords: tbb-hardening added

comment:4 Changed 4 years ago by gk

Keywords: tbb-hardened added; tbb-hardening removed

comment:5 Changed 4 years ago by gk

Keywords: TorBrowserTeam201512R added
Severity: Normal
Status: newneeds_review

Let's include that for the next hardened release: bug_12519 (https://gitweb.torproject.org/user/gk/tor-browser.git/commit/?h=bug_12516) has the fix.

comment:6 Changed 4 years ago by mcs

Interesting stuff. Your changes look good and I agree that using -fwrapv is the safest choice, so r=mcs.

I wonder what the performance impact is? Hopefully not noticeable given the comment that is has been turned on for the Linux kernel.

comment:7 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

I guess given ASan on it won't matter much. :) Cherry-picked to tor-browser-38.4.0esr-5.5-1 (commit 507be1ee78b54b23106d6e99b5d07835d34682a0).

comment:8 Changed 4 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

The CXXFLAGS definition contains a typo (-frwapv instead of -fwrapv).

comment:9 Changed 4 years ago by gk

Resolution: fixed
Status: reopenedclosed

Bah, that's embarrassing. :/ Thanks for catching that one. Fixed with commit 2dd8df51e236c471675870aca0857946c0c1d035.

comment:10 in reply to:  9 Changed 4 years ago by mcs

Replying to gk:

Bah, that's embarrassing. :/ Thanks for catching that one.

And I should have caught it during review, but my mind did not register the typo even if my eyes saw it. Sorry!

Note: See TracTickets for help on using tickets.