Verify security hardening features are turned on #1064

defuse · 2016-07-01T00:36:59Z

No description provided.

daira · 2016-07-05T22:35:54Z

defuse · 2016-07-05T22:54:01Z

I just did a rebuild with --disable-hardening, and the test catches the basic stuff but it still said FORTIFY_SOURCE was on. I think that's because it didn't completely rebuild everything, so the symbols it looks for to check that were still in there. I'm trying again with a full rebuild (after git clean -dfx).

daira · 2016-07-06T10:06:40Z

qa/zcash/check-security-hardening.sh

-    if "${REPOROOT}/qa/zcash/checksec.sh" --fortify-file "$1" | grep -q "FORTIFY_SOURCE support available.*Yes" &&
-       "${REPOROOT}/qa/zcash/checksec.sh" --fortify-file "$1" | grep -q "Binary compiled with FORTIFY_SOURCE support.*Yes"; then
+    if { "${REPOROOT}/qa/zcash/checksec.sh" --fortify-file "$1" | grep -q "FORTIFY_SOURCE support available.*Yes"; } &&
+       { "${REPOROOT}/qa/zcash/checksec.sh" --fortify-file "$1" | grep -q "Binary compiled with FORTIFY_SOURCE support.*Yes"; } then


I think this line should strictly speaking end in }; then. } then is accepted and apparently works, but I can find no documentation guaranteeing that it will work.

daira · 2016-07-11T11:51:35Z

ACK.

daira · 2016-07-11T12:01:25Z

Still concerned about #915 (comment) though.

ebfull · 2016-07-18T19:01:39Z

Is this still WIP or can it be reviewed?

defuse · 2016-07-21T19:45:09Z

I pulled in upstream's stuff, here's a TODO list:

Use -O1 instead of -O2.
Re-enable --enable-hardening security features #915 (comment)
~~Re-enable --enable-hardening security features #915 (comment)~~ (I decided not to do this because it would add unreadable/unmaintainable code for not much benefit)
Re-enable --enable-hardening security features #915 (comment)

defuse · 2016-07-22T01:54:37Z

force pushed

ebfull · 2016-07-22T02:00:30Z

qa/zcash/check-security-hardening.sh

+test_fortify_source "${REPOROOT}/src/zcash-gtest"
+test_fortify_source "${REPOROOT}/src/bitcoin-tx"
+test_fortify_source "${REPOROOT}/src/test/test_bitcoin"
+test_fortify_source "${REPOROOT}/src/zcash/GenerateParams"


I'm a bit worried if we add another binary we'll forget to put it here, but I don't know of a better practical alternative.

ebfull · 2016-07-22T02:01:25Z

What's the story with the CXXFLAGS we give libzcash?

defuse · 2016-07-22T02:05:04Z

@ebfull haven't got to that yet

defuse · 2016-07-22T02:41:20Z

Something is really wrong with our build system, here's a command from the output showing a build in libzcash:

/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/native/bin/ccache g++ -m64 -DHAVE_CONFIG_H -I. -I../src/config  -DMULTICORE -fopenmp -fPIC -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS    -pipe -O2 -O0 -g -Wstack-protector -fstack-protector-all -fPIE -fvisibility=hidden -DSTATIC -I. -I./obj -pthread -I/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/include -I./leveldb/include -I./leveldb/helpers/memenv -I/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/include -I/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/include -I./secp256k1/include  -I/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/include/libsnark -I/home/firexware/Data-1/Documents/FireXware/la/zcash/depends/x86_64-unknown-linux-gnu/include/  -DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -std=c++11 -pipe -O2 -Wno-deprecated-declarations -Wno-placement-new -Wno-terminate -Werror -O1 -g  -Wstack-protector -fstack-protector-all -fPIE -MT zcash/libzcash_a-Note.o -MD -MP -MF zcash/.deps/libzcash_a-Note.Tpo -c -o zcash/libzcash_a-Note.o `test -f 'zcash/Note.cpp' || echo './'`zcash/Note.cpp

Notice there's -O1 and -O2 multiple times.

defuse · 2016-07-22T02:42:19Z

src/Makefile.am

@@ -415,7 +415,7 @@ libzcash_a_SOURCES = \
  zcash/prf.cpp \
  zcash/util.cpp

-libzcash_a_CPPFLAGS = -DMULTICORE -fopenmp -fPIC -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS $(HARDENED_CPPFLAGS) -pipe -O2 -O0 -g -Wstack-protector -fstack-protector-all -fPIE -fvisibility=hidden -DSTATIC $(BITCOIN_INCLUDES)
+libzcash_a_CPPFLAGS = -DMULTICORE -fopenmp -fPIC -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS $(HARDENED_CPPFLAGS) $(HARDENED_CXXFLAGS) $(HARDENED_LDFLAGS) -pipe -O2 -O0 -g -Wstack-protector -fstack-protector-all -fPIE -fvisibility=hidden -DSTATIC $(BITCOIN_INCLUDES)


Not related to this change: How did we end up with -O2 -O0 right beside each other?!?

Also I don't even know if CXXFLAGS will be effective when given to CPPFLAGS... if I add a libzcash_a_CXXFLAGS = ... will it magically put that in the appropriate build commands? What about LDFLAGS?

How did we end up with -O2 -O0 right beside each other?!?

Not sure, we should get rid of it.

will it magically put that in the appropriate build commands

Yes.

What about LDFLAGS?

Yes. And we should.

How did we end up with -O2 -O0 right beside each other?!?
Not sure, we should get rid of it.

Hypothesis: Someone cut and pasted flags from a command executed by Make that appeared in their build output.

I've seen -O2 -O0 -g before in the commands issued by Make due to different variable expansions, but I've never seen those flags defined next to eachother (which is nonsensical). It may be that someone cut and pasted a command?

(BTW: g++ documents that it always uses the last occurring -O value.)

defuse · 2016-07-22T02:44:31Z

force pushed again, doing some cleanup

… and #7424.

defuse · 2016-07-22T02:50:03Z

Ok, this is ready for review.

Rant: But even though I've read more than the first half of a whole book about it, I don't understand the configure/autoconf/magic-variable-name-interpretation crap so I don't have much confidence that the security stuff is really enabled for everything. #1064 (comment) demonstrates there is at least something wrong with our build (prior to the work in this PR).

solardiz · 2016-07-22T04:31:16Z

On Thu, Jul 21, 2016 at 12:45:14PM -0700, Taylor Hornby wrote:

Use -O1 instead of -O2.

I probably missed earlier discussion somewhere, but why do you prefer -O1? I think going with -O1 may actually hurt security, not to mention performance. How about "-O2 -fno-strict-aliasing -fwrapv"? That's for production builds; for development, we should also test with pure -O2 or even -O3, and ensure we receive no strict aliasing violation warnings, etc. (which are not printed in the -fno-strict-aliasing mode).

daira · 2016-07-22T12:25:17Z

I prefer -O1 because I don't trust GCC's implementation of optimizations used at higher levels to be correct. I can't say I trust GCC at all, but we're forced to, and the optimizations at -O2 and above start getting very complicated. However, +1 for -fwrapv, and +0.5 for -fno-strict-aliasing.

ebfull · 2016-07-27T04:53:32Z

ACK but my complaints on #1064 (comment) may be relevant.

ebfull · 2016-08-01T15:57:40Z

This can be merged I believe! Let's merge it soon so we can see the performance impact!

bitcartel · 2016-08-01T23:23:37Z

There might be an issue. I just performed a build and I can see both -O1 and -O2 flags are being used:

-DBOOST_SPIRIT_THREADSAFE -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -std=c++11 -pipe -O2 -Wno-deprecated-declarations -Wno-placement-new -Wno-terminate -Werror -O1 -g -Wstack-protector

bitcartel · 2016-08-03T06:18:24Z

@defuse I think we just need a one line change. Here's the diff: https://gist.github.com/bitcartel/436ae915ce7873e374b55e158f04fa47

daira · 2016-08-03T08:27:02Z

That change replaces -O2 -O0 with -O1 in the libzcash flags, but the flags in @bitcartel's comment above did not contain -O0, so I think they must be using a different variable.

Why do we have these options specified in so many different places?

bitcartel · 2016-08-03T17:53:40Z

Optimization flags affect performance measurements, but are they a roadblock for this PR to satisfy "Verify security hardening features are turned on"?

I think we can and should merge but will wait for feedback before doing so. In the meantime, I've opened #1168 as a reminder to review and fix optimization flags for the next release, z9.

bitcartel · 2016-08-03T23:41:49Z

ACK after dicussion with @nathan-at-least.

bitcartel · 2016-08-03T23:57:02Z

@zkbot r+

zkbot · 2016-08-03T23:57:02Z

📌 Commit 73537eb has been approved by bitcartel

zkbot · 2016-08-03T23:57:08Z

⌛ Testing commit 73537eb with merge abd4c16...

…ecurity-hardening, r=bitcartel Verify security hardening features are turned on

zkbot · 2016-08-04T00:27:17Z

☀️ Test successful - zcash

Continuation of zcash#1064 and related to zcash#1168.

defuse mentioned this pull request Jul 1, 2016

Re-enable --enable-hardening security features #915

Closed

daira reviewed Jul 6, 2016
View reviewed changes

Make the --enable-hardening flag explicit.

ae79cec

defuse force-pushed the t915-enable-security-hardening branch from ef0f257 to a8cdc16 Compare July 22, 2016 01:54

ebfull reviewed Jul 22, 2016
View reviewed changes

defuse reviewed Jul 22, 2016
View reviewed changes

defuse force-pushed the t915-enable-security-hardening branch from 2d04fae to be0d1e1 Compare July 22, 2016 02:44

defuse added 6 commits July 21, 2016 20:45

Enable -O1 for better FORTIFY_SOURCE protections.

4e0b4af

Add checksec.sh from http://www.trapkit.de/tools/checksec.html

effd7ca

Add tests for security hardening features

1327d19

Pull in upstream's make check-security, based on upstream PR zcash#6854…

56734f4

… and #7424.

Make security options in configure.ac fail if unavailable.

b00b98a

Put hardened stuff in libzcash CPPFLAGS.

73537eb

defuse force-pushed the t915-enable-security-hardening branch from be0d1e1 to 73537eb Compare July 22, 2016 02:45

defuse changed the title ~~[WIP] Verify security hardening features are turned on~~ Verify security hardening features are turned on Jul 22, 2016

defuse added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2016

nathan-at-least modified the milestones: z8 release - core (excl. RPC) audit freeze, functional PoW, z9 Release - RPC, Audit tasks, Testing improvements Aug 1, 2016

nathan-at-least assigned nathan-at-least and unassigned nathan-at-least Aug 1, 2016

bitcartel mentioned this pull request Aug 3, 2016

Fix compile flag issue where -O2 is still being used #1168

Closed

zkbot pushed a commit that referenced this pull request Aug 3, 2016

Auto merge of #1064 - ThisIsNotOfficialCodeItsJustForks:t915-enable-s…

abd4c16

…ecurity-hardening, r=bitcartel Verify security hardening features are turned on

zkbot merged commit 73537eb into zcash:zc.v0.11.2.latest Aug 4, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 4, 2016

Use -O1 opimitization flag when building libzcash.

fbd9051

Continuation of zcash#1064 and related to zcash#1168.

This was referenced Aug 4, 2016

Use -O1 opimitization flag when building libzcash #1170

Merged

Eliminate some of the duplicates caused by truncating indices #1173

Merged

nathan-at-least mentioned this pull request Aug 11, 2016

Fix coverage reporting. #1235

Closed

This was referenced Aug 17, 2016

Test with -O2 and -O3 on the CI #1269

Closed

offer security-hardening and tests thereof to upstream #1071

Open

daira mentioned this pull request Oct 28, 2016

Build libsnark with -march=x86-64 instead of -march=native #1673

Merged

hazinstore mentioned this pull request Oct 30, 2016

Measure potential performance improvement from -O2 or -O3 optimization and enable where/if justified #1733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify security hardening features are turned on #1064

Verify security hardening features are turned on #1064

defuse commented Jul 1, 2016

daira commented Jul 5, 2016

defuse commented Jul 5, 2016

daira Jul 6, 2016

daira commented Jul 11, 2016

daira commented Jul 11, 2016

ebfull commented Jul 18, 2016

defuse commented Jul 21, 2016 •

edited

defuse commented Jul 22, 2016

ebfull Jul 22, 2016

ebfull commented Jul 22, 2016

defuse commented Jul 22, 2016

defuse commented Jul 22, 2016

defuse Jul 22, 2016

defuse Jul 22, 2016

ebfull Jul 22, 2016

nathan-at-least Aug 3, 2016

defuse commented Jul 22, 2016

defuse commented Jul 22, 2016

solardiz commented Jul 22, 2016

daira commented Jul 22, 2016

ebfull commented Jul 27, 2016

ebfull commented Aug 1, 2016

bitcartel commented Aug 1, 2016

bitcartel commented Aug 3, 2016 •

edited

daira commented Aug 3, 2016

bitcartel commented Aug 3, 2016

bitcartel commented Aug 3, 2016

bitcartel commented Aug 3, 2016

zkbot commented Aug 3, 2016

zkbot commented Aug 3, 2016

zkbot commented Aug 4, 2016

Verify security hardening features are turned on #1064

Verify security hardening features are turned on #1064

Conversation

defuse commented Jul 1, 2016

daira commented Jul 5, 2016

defuse commented Jul 5, 2016

daira Jul 6, 2016

Choose a reason for hiding this comment

daira commented Jul 11, 2016

daira commented Jul 11, 2016

ebfull commented Jul 18, 2016

defuse commented Jul 21, 2016 • edited

defuse commented Jul 22, 2016

ebfull Jul 22, 2016

Choose a reason for hiding this comment

ebfull commented Jul 22, 2016

defuse commented Jul 22, 2016

defuse commented Jul 22, 2016

defuse Jul 22, 2016

Choose a reason for hiding this comment

defuse Jul 22, 2016

Choose a reason for hiding this comment

ebfull Jul 22, 2016

Choose a reason for hiding this comment

nathan-at-least Aug 3, 2016

Choose a reason for hiding this comment

defuse commented Jul 22, 2016

defuse commented Jul 22, 2016

solardiz commented Jul 22, 2016

daira commented Jul 22, 2016

ebfull commented Jul 27, 2016

ebfull commented Aug 1, 2016

bitcartel commented Aug 1, 2016

bitcartel commented Aug 3, 2016 • edited

daira commented Aug 3, 2016

bitcartel commented Aug 3, 2016

bitcartel commented Aug 3, 2016

bitcartel commented Aug 3, 2016

zkbot commented Aug 3, 2016

zkbot commented Aug 3, 2016

zkbot commented Aug 4, 2016

defuse commented Jul 21, 2016 •

edited

bitcartel commented Aug 3, 2016 •

edited