From ce854a8d22d5056cc1a47a0d4d4251f93a0c667c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 Apr 2016 18:59:40 -0400 Subject: [PATCH 1/7] Add -ftrapv to gcc-hardening ... mostly! We know there are overflows in curve25519-donna-c32, so we'll have to have that one be fwrapv. Only apply the asan, ubsan, and trapv options to the code that does not need to run in constant time. Those options introduce branches to the code they instrument. (These introduced branches should never actually be taken, so it might _still_ be constant time after all, but branch predictors are complicated enough that I'm not really confident here. Let's aim for safety.) Closes 17983. --- .gitignore | 3 +++ Makefile.am | 4 ++-- acinclude.m4 | 20 +++++++++++++++----- changes/bug17983 | 11 +++++++++++ configure.ac | 23 ++++++++++++++++++++--- src/common/include.am | 17 ++++++++++++++--- src/ext/include.am | 9 ++++++--- src/or/include.am | 3 ++- src/test/include.am | 6 ++++++ src/tools/include.am | 11 +++++++++-- 10 files changed, 88 insertions(+), 19 deletions(-) create mode 100644 changes/bug17983 diff --git a/.gitignore b/.gitignore index b141e80e8..7103dbed0 100644 --- a/.gitignore +++ b/.gitignore @@ -132,6 +132,9 @@ uptime-*.json /src/common/libor.a /src/common/libor-testing.a /src/common/libor.lib +/src/common/libor-ctime.a +/src/common/libor-ctime-testing.a +/src/common/libor-ctime.lib /src/common/libor-crypto.a /src/common/libor-crypto-testing.a /src/common/libor-crypto.lib diff --git a/Makefile.am b/Makefile.am index 92ba2b868..e9abfc6b9 100644 --- a/Makefile.am +++ b/Makefile.am @@ -15,8 +15,8 @@ noinst_PROGRAMS= DISTCLEANFILES= bin_SCRIPTS= AM_CPPFLAGS= -AM_CFLAGS = @TOR_SYSTEMD_CFLAGS@ -SHELL = @SHELL@ +AM_CFLAGS=@TOR_SYSTEMD_CFLAGS@ @CFLAGS_BUGTRAP@ +SHELL=@SHELL@ if COVERAGE_ENABLED TESTING_TOR_BINARY="$(top_builddir)/src/or/tor-cov" diff --git a/acinclude.m4 b/acinclude.m4 index 7b1aab2f9..4b9f0953e 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -42,10 +42,11 @@ AC_DEFUN([TOR_DEFINE_CODEPATH], AC_SUBST(TOR_LDFLAGS_$2) ]) -dnl 1:flags -dnl 2:also try to link (yes: non-empty string) -dnl will set yes or no in $tor_can_link_$1 (as modified by AS_VAR_PUSHDEF) -AC_DEFUN([TOR_CHECK_CFLAGS], [ +dnl 1: flags +dnl 2: try to link too if this is nonempty. +dnl 3: what to do on success compiling +dnl 4: what to do on failure compiling +AC_DEFUN([TOR_TRY_COMPILE_WITH_CFLAGS], [ AS_VAR_PUSHDEF([VAR],[tor_cv_cflags_$1]) AC_CACHE_CHECK([whether the compiler accepts $1], VAR, [ tor_saved_CFLAGS="$CFLAGS" @@ -63,11 +64,20 @@ AC_DEFUN([TOR_CHECK_CFLAGS], [ CFLAGS="$tor_saved_CFLAGS" ]) if test x$VAR = xyes; then - CFLAGS="$CFLAGS $1" + $3 + else + $4 fi AS_VAR_POPDEF([VAR]) ]) +dnl 1:flags +dnl 2:also try to link (yes: non-empty string) +dnl will set yes or no in $tor_can_link_$1 (as modified by AS_VAR_PUSHDEF) +AC_DEFUN([TOR_CHECK_CFLAGS], [ + TOR_TRY_COMPILE_WITH_CFLAGS($1, $2, CFLAGS="$CFLAGS $1", /bin/true) +]) + dnl 1:flags dnl 2:extra ldflags dnl 3:extra libraries diff --git a/changes/bug17983 b/changes/bug17983 new file mode 100644 index 000000000..db52a3761 --- /dev/null +++ b/changes/bug17983 @@ -0,0 +1,11 @@ + o Minor features (bug-finding): + - Tor now builds with -ftrapv by default on compilers that support it. + This option detects signed integer overflow, and turns it into a + hard-failure. We do not apply this option to code that needs to run + in constant time to avoid side-channels; instead, we use -fwrapv. + Closes ticket 17983. + - When --enable-expensive-hardening is selected, stop applying the clang/gcc + sanitizers to code that needs to run in constant-time to avoid side + channels: although we are aware of no introduced side-channels, we + are not able to prove that this is safe. Related to ticket 17983. + diff --git a/configure.ac b/configure.ac index 4bdd2d396..626be6c11 100644 --- a/configure.ac +++ b/configure.ac @@ -755,6 +755,11 @@ dnl use it with a build of a library. all_ldflags_for_check="$TOR_LDFLAGS_zlib $TOR_LDFLAGS_openssl $TOR_LDFLAGS_libevent" all_libs_for_check="$TOR_ZLIB_LIBS $TOR_LIB_MATH $TOR_LIBEVENT_LIBS $TOR_OPENSSL_LIBS $TOR_SYSTEMD_LIBS $TOR_LIB_WS32 $TOR_LIB_GDI $TOR_CAP_LIBS" +CFLAGS_FTRAPV= +CFLAGS_FWRAPV= +CFLAGS_ASAN= +CFLAGS_UBSAN= + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [ #if !defined(__clang__) #error @@ -777,20 +782,32 @@ m4_ifdef([AS_VAR_IF],[ AS_VAR_POPDEF([can_link]) AS_VAR_POPDEF([can_compile]) TOR_CHECK_CFLAGS(-Wstack-protector) - TOR_CHECK_CFLAGS(-fwrapv) TOR_CHECK_CFLAGS(--param ssp-buffer-size=1) if test "$bwin32" = "false"; then TOR_CHECK_CFLAGS(-fPIE) TOR_CHECK_LDFLAGS(-pie, "$all_ldflags_for_check", "$all_libs_for_check") fi + TOR_TRY_COMPILE_WITH_CFLAGS(-ftrapv, , CFLAGS_FTRAPV="-ftrapv", /bin/true) + TOR_TRY_COMPILE_WITH_CFLAGS(-fwrapv, , CFLAGS_FWRAPV="-fwrapv", /bin/true) fi if test "x$enable_expensive_hardening" = "xyes"; then - TOR_CHECK_CFLAGS([-fsanitize=address]) - TOR_CHECK_CFLAGS([-fsanitize=undefined]) + TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=address], , CFLAGS_ASAN="-fsanitize=address", /bin/true) + TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=undefined], , CFLAGS_UBSAN="-fsanitize=undefined", /bin/true) TOR_CHECK_CFLAGS([-fno-omit-frame-pointer]) fi +CFLAGS_BUGTRAP="$CFLAGS_FTRAPV $CFLAGS_ASAN $CFLAGS_UBSAN" +CFLAGS_CONSTTIME="$CFLAGS_FWRAPV" + +dnl These cflags add bunches of branches, and we haven't been able to +dnl persuade ourselves that they're suitable for code that needs to be +dnl constant time. +AC_SUBST(CFLAGS_BUGTRAP) +dnl These cflags are variant ones sutable for code that needs to be +dnl constant-time. +AC_SUBST(CFLAGS_CONSTTIME) + if test "x$enable_linker_hardening" != "xno"; then TOR_CHECK_LDFLAGS(-z relro -z now, "$all_ldflags_for_check", "$all_libs_for_check") fi diff --git a/src/common/include.am b/src/common/include.am index 5afb30da6..96fc329aa 100644 --- a/src/common/include.am +++ b/src/common/include.am @@ -1,12 +1,14 @@ noinst_LIBRARIES += \ src/common/libor.a \ + src/common/libor-ctime.a \ src/common/libor-crypto.a \ src/common/libor-event.a if UNITTESTS_ENABLED noinst_LIBRARIES += \ src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ src/common/libor-crypto-testing.a \ src/common/libor-event-testing.a endif @@ -27,12 +29,14 @@ src_common_libcurve25519_donna_a_CFLAGS= if BUILD_CURVE25519_DONNA src_common_libcurve25519_donna_a_SOURCES=\ src/ext/curve25519_donna/curve25519-donna.c +# See bug 13538 -- this code is known to have signed overflow issues. src_common_libcurve25519_donna_a_CFLAGS+=\ - @F_OMIT_FRAME_POINTER@ + @F_OMIT_FRAME_POINTER@ @CFLAGS_CONSTTIME@ noinst_LIBRARIES+=src/common/libcurve25519_donna.a LIBDONNA=src/common/libcurve25519_donna.a else if BUILD_CURVE25519_DONNA_C64 +src_common_libcurve25519_donna_a_CFLAGS+=@CFLAGS_CONSTTIME@ src_common_libcurve25519_donna_a_SOURCES=\ src/ext/curve25519_donna/curve25519-donna-c64.c noinst_LIBRARIES+=src/common/libcurve25519_donna.a @@ -58,13 +62,21 @@ else readpassphrase_source= endif +LIBOR_CTIME_A_SOURCES = \ + src/ext/csiphash.c \ + src/common/di_ops.c + +src_common_libor_ctime_a_SOURCES = $(LIBOR_CTIME_A_SOURCES) +src_common_libor_ctime_testing_a_SOURCES = $(LIBOR_CTIME_A_SOURCES) +src_common_libor_ctime_a_CFLAGS = @CFLAGS_CONSTTIME@ +src_common_libor_ctime_testing_a_CFLAGS = @CFLAGS_CONSTTIME@ $(TEST_CFLAGS) + LIBOR_A_SOURCES = \ src/common/address.c \ src/common/backtrace.c \ src/common/compat.c \ src/common/compat_threads.c \ src/common/container.c \ - src/common/di_ops.c \ src/common/log.c \ src/common/memarea.c \ src/common/util.c \ @@ -72,7 +84,6 @@ LIBOR_A_SOURCES = \ src/common/util_process.c \ src/common/sandbox.c \ src/common/workqueue.c \ - src/ext/csiphash.c \ src/ext/trunnel/trunnel.c \ $(libor_extra_source) \ $(threads_impl_source) \ diff --git a/src/ext/include.am b/src/ext/include.am index bf678f2c9..2a0227a85 100644 --- a/src/ext/include.am +++ b/src/ext/include.am @@ -16,7 +16,8 @@ EXTHEADERS = \ noinst_HEADERS+= $(EXTHEADERS) -src_ext_ed25519_ref10_libed25519_ref10_a_CFLAGS= +src_ext_ed25519_ref10_libed25519_ref10_a_CFLAGS=\ + @CFLAGS_CONSTTIME@ src_ext_ed25519_ref10_libed25519_ref10_a_SOURCES= \ src/ext/ed25519/ref10/fe_0.c \ @@ -93,7 +94,8 @@ noinst_HEADERS += $(ED25519_REF10_HDRS) LIBED25519_REF10=src/ext/ed25519/ref10/libed25519_ref10.a noinst_LIBRARIES += $(LIBED25519_REF10) -src_ext_ed25519_donna_libed25519_donna_a_CFLAGS= \ +src_ext_ed25519_donna_libed25519_donna_a_CFLAGS=\ + @CFLAGS_CONSTTIME@ \ -DED25519_CUSTOMRANDOM \ -DED25519_SUFFIX=_donna @@ -135,7 +137,8 @@ noinst_HEADERS += $(ED25519_DONNA_HDRS) LIBED25519_DONNA=src/ext/ed25519/donna/libed25519_donna.a noinst_LIBRARIES += $(LIBED25519_DONNA) -src_ext_keccak_tiny_libkeccak_tiny_a_CFLAGS= +src_ext_keccak_tiny_libkeccak_tiny_a_CFLAGS=\ + @CFLAGS_CONSTTIME@ src_ext_keccak_tiny_libkeccak_tiny_a_SOURCES= \ src/ext/keccak-tiny/keccak-tiny-unrolled.c diff --git a/src/or/include.am b/src/or/include.am index 712ae1840..19f1a7fe0 100644 --- a/src/or/include.am +++ b/src/or/include.am @@ -109,7 +109,7 @@ src_or_libtor_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_or_tor_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ @TOR_LDFLAGS_libevent@ -src_or_tor_LDADD = src/or/libtor.a src/common/libor.a \ +src_or_tor_LDADD = src/or/libtor.a src/common/libor.a src/common/libor-ctime.a \ src/common/libor-crypto.a $(LIBKECCAK_TINY) $(LIBDONNA) \ src/common/libor-event.a src/trunnel/libor-trunnel.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ @TOR_OPENSSL_LIBS@ \ @@ -121,6 +121,7 @@ src_or_tor_cov_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) src_or_tor_cov_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_or_tor_cov_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ @TOR_LDFLAGS_libevent@ src_or_tor_cov_LDADD = src/or/libtor-testing.a src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ src/common/libor-crypto-testing.a $(LIBKECCAK_TINY) $(LIBDONNA) \ src/common/libor-event-testing.a src/trunnel/libor-trunnel-testing.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ @TOR_OPENSSL_LIBS@ \ diff --git a/src/test/include.am b/src/test/include.am index 7d80fdf15..c4ef30fe0 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -147,6 +147,7 @@ src_test_test_switch_id_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_test_test_switch_id_LDFLAGS = @TOR_LDFLAGS_zlib@ src_test_test_switch_id_LDADD = \ src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ src_test_test_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @@ -156,6 +157,7 @@ src_test_test_LDADD = src/or/libtor-testing.a \ $(LIBKECCAK_TINY) \ $(LIBDONNA) \ src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ src/common/libor-event-testing.a \ src/trunnel/libor-trunnel-testing.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ \ @@ -175,6 +177,7 @@ src_test_test_memwipe_LDFLAGS = $(src_test_test_LDFLAGS) src_test_bench_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ src_test_bench_LDADD = src/or/libtor.a src/common/libor.a \ + src/common/libor-ctime.a \ src/common/libor-crypto.a $(LIBKECCAK_TINY) $(LIBDONNA) \ src/common/libor-event.a src/trunnel/libor-trunnel.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ \ @@ -185,6 +188,7 @@ src_test_test_workqueue_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ src_test_test_workqueue_LDADD = src/or/libtor-testing.a \ src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ src/common/libor-crypto-testing.a $(LIBKECCAK_TINY) $(LIBDONNA) \ src/common/libor-event-testing.a \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ @TOR_LIBEVENT_LIBS@ \ @@ -208,6 +212,7 @@ noinst_PROGRAMS+= src/test/test-ntor-cl src_test_test_ntor_cl_SOURCES = src/test/test_ntor_cl.c src_test_test_ntor_cl_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ src_test_test_ntor_cl_LDADD = src/or/libtor.a src/common/libor.a \ + src/common/libor-ctime.a \ src/common/libor-crypto.a $(LIBKECCAK_TINY) $(LIBDONNA) \ @TOR_ZLIB_LIBS@ @TOR_LIB_MATH@ \ @TOR_OPENSSL_LIBS@ @TOR_LIB_WS32@ @TOR_LIB_GDI@ @CURVE25519_LIBS@ @@ -217,6 +222,7 @@ src_test_test_ntor_cl_AM_CPPFLAGS = \ noinst_PROGRAMS += src/test/test-bt-cl src_test_test_bt_cl_SOURCES = src/test/test_bt_cl.c src_test_test_bt_cl_LDADD = src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ @TOR_LIB_MATH@ \ @TOR_LIB_WS32@ @TOR_LIB_GDI@ src_test_test_bt_cl_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) diff --git a/src/tools/include.am b/src/tools/include.am index 38ed57546..b4ea82d35 100644 --- a/src/tools/include.am +++ b/src/tools/include.am @@ -7,19 +7,23 @@ endif src_tools_tor_resolve_SOURCES = src/tools/tor-resolve.c src_tools_tor_resolve_LDFLAGS = -src_tools_tor_resolve_LDADD = src/common/libor.a @TOR_LIB_MATH@ @TOR_LIB_WS32@ +src_tools_tor_resolve_LDADD = src/common/libor.a \ + src/common/libor-ctime.a \ + @TOR_LIB_MATH@ @TOR_LIB_WS32@ if COVERAGE_ENABLED src_tools_tor_cov_resolve_SOURCES = src/tools/tor-resolve.c src_tools_tor_cov_resolve_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) src_tools_tor_cov_resolve_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_tools_tor_cov_resolve_LDADD = src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ @TOR_LIB_MATH@ @TOR_LIB_WS32@ endif src_tools_tor_gencert_SOURCES = src/tools/tor-gencert.c src_tools_tor_gencert_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ src_tools_tor_gencert_LDADD = src/common/libor.a src/common/libor-crypto.a \ + src/common/libor-ctime.a \ $(LIBKECCAK_TINY) \ $(LIBDONNA) \ @TOR_LIB_MATH@ @TOR_ZLIB_LIBS@ @TOR_OPENSSL_LIBS@ \ @@ -31,6 +35,7 @@ src_tools_tor_cov_gencert_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) src_tools_tor_cov_gencert_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) src_tools_tor_cov_gencert_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ src_tools_tor_cov_gencert_LDADD = src/common/libor-testing.a \ + src/common/libor-ctime-testing.a \ src/common/libor-crypto-testing.a \ $(LIBKECCAK_TINY) \ $(LIBDONNA) \ @@ -40,7 +45,9 @@ endif src_tools_tor_checkkey_SOURCES = src/tools/tor-checkkey.c src_tools_tor_checkkey_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ -src_tools_tor_checkkey_LDADD = src/common/libor.a src/common/libor-crypto.a \ +src_tools_tor_checkkey_LDADD = src/common/libor.a \ + src/common/libor-ctime.a \ + src/common/libor-crypto.a \ $(LIBKECCAK_TINY) \ $(LIBDONNA) \ @TOR_LIB_MATH@ @TOR_ZLIB_LIBS@ @TOR_OPENSSL_LIBS@ \ From 20432fc541bf0fb8f136f0d4635b264c624ce3eb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 27 Apr 2016 10:16:43 -0400 Subject: [PATCH 2/7] Refactor out u64_dbl_t This type saved a tiny amount of allocation, but not enough to be worth keeping. (This is in preparation for moving choose_array_element_by_weight) --- src/or/routerlist.c | 58 ++++++++++++++++++++++++++------------------- src/or/routerlist.h | 13 +++------- src/test/test_dir.c | 45 ++++++++++++++++++----------------- 3 files changed, 59 insertions(+), 57 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 80d01970c..620b61449 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -67,7 +67,7 @@ typedef struct cert_list_t cert_list_t; /* static function prototypes */ static int compute_weighted_bandwidths(const smartlist_t *sl, bandwidth_weight_rule_t rule, - u64_dbl_t **bandwidths_out); + double **bandwidths_out); static const routerstatus_t *router_pick_trusteddirserver_impl( const smartlist_t *sourcelist, dirinfo_type_t auth, int flags, int *n_busy_out); @@ -1799,20 +1799,23 @@ dirserver_choose_by_weight(const smartlist_t *servers, double authority_weight) { int n = smartlist_len(servers); int i; - u64_dbl_t *weights; + double *weights_dbl; + uint64_t *weights_u64; const dir_server_t *ds; - weights = tor_calloc(n, sizeof(u64_dbl_t)); + weights_dbl = tor_calloc(n, sizeof(double)); + weights_u64 = tor_calloc(n, sizeof(uint64_t)); for (i = 0; i < n; ++i) { ds = smartlist_get(servers, i); - weights[i].dbl = ds->weight; + weights_dbl[i] = ds->weight; if (ds->is_authority) - weights[i].dbl *= authority_weight; + weights_dbl[i] *= authority_weight; } - scale_array_elements_to_u64(weights, n, NULL); - i = choose_array_element_by_weight(weights, n); - tor_free(weights); + scale_array_elements_to_u64(weights_u64, weights_dbl, n, NULL); + i = choose_array_element_by_weight(weights_u64, n); + tor_free(weights_dbl); + tor_free(weights_u64); return (i < 0) ? NULL : smartlist_get(servers, i); } @@ -2075,7 +2078,8 @@ router_get_advertised_bandwidth_capped(const routerinfo_t *router) * much of the range of uint64_t. If total_out is provided, set it to * the sum of all elements in the array _before_ scaling. */ STATIC void -scale_array_elements_to_u64(u64_dbl_t *entries, int n_entries, +scale_array_elements_to_u64(uint64_t *entries_out, const double *entries_in, + int n_entries, uint64_t *total_out) { double total = 0.0; @@ -2085,13 +2089,13 @@ scale_array_elements_to_u64(u64_dbl_t *entries, int n_entries, #define SCALE_TO_U64_MAX ((int64_t) (INT64_MAX / 4)) for (i = 0; i < n_entries; ++i) - total += entries[i].dbl; + total += entries_in[i]; if (total > 0.0) scale_factor = SCALE_TO_U64_MAX / total; for (i = 0; i < n_entries; ++i) - entries[i].u64 = tor_llround(entries[i].dbl * scale_factor); + entries_out[i] = tor_llround(entries_in[i] * scale_factor); if (total_out) *total_out = (uint64_t) total; @@ -2119,7 +2123,7 @@ gt_i64_timei(uint64_t a, uint64_t b) * an index at random. Return -1 on error. */ STATIC int -choose_array_element_by_weight(const u64_dbl_t *entries, int n_entries) +choose_array_element_by_weight(const uint64_t *entries, int n_entries) { int i, i_chosen=-1, n_chosen=0; uint64_t total_so_far = 0; @@ -2127,7 +2131,7 @@ choose_array_element_by_weight(const u64_dbl_t *entries, int n_entries) uint64_t total = 0; for (i = 0; i < n_entries; ++i) - total += entries[i].u64; + total += entries[i]; if (n_entries < 1) return -1; @@ -2140,7 +2144,7 @@ choose_array_element_by_weight(const u64_dbl_t *entries, int n_entries) rand_val = crypto_rand_uint64(total); for (i = 0; i < n_entries; ++i) { - total_so_far += entries[i].u64; + total_so_far += entries[i]; if (gt_i64_timei(total_so_far, rand_val)) { i_chosen = i; n_chosen++; @@ -2206,17 +2210,21 @@ static const node_t * smartlist_choose_node_by_bandwidth_weights(const smartlist_t *sl, bandwidth_weight_rule_t rule) { - u64_dbl_t *bandwidths=NULL; + double *bandwidths_dbl=NULL; + uint64_t *bandwidths_u64=NULL; - if (compute_weighted_bandwidths(sl, rule, &bandwidths) < 0) + if (compute_weighted_bandwidths(sl, rule, &bandwidths_dbl) < 0) return NULL; - scale_array_elements_to_u64(bandwidths, smartlist_len(sl), NULL); + bandwidths_u64 = tor_calloc(smartlist_len(sl), sizeof(uint64_t)); + scale_array_elements_to_u64(bandwidths_u64, bandwidths_dbl, + smartlist_len(sl), NULL); { - int idx = choose_array_element_by_weight(bandwidths, + int idx = choose_array_element_by_weight(bandwidths_u64, smartlist_len(sl)); - tor_free(bandwidths); + tor_free(bandwidths_dbl); + tor_free(bandwidths_u64); return idx < 0 ? NULL : smartlist_get(sl, idx); } } @@ -2229,14 +2237,14 @@ smartlist_choose_node_by_bandwidth_weights(const smartlist_t *sl, static int compute_weighted_bandwidths(const smartlist_t *sl, bandwidth_weight_rule_t rule, - u64_dbl_t **bandwidths_out) + double **bandwidths_out) { int64_t weight_scale; double Wg = -1, Wm = -1, We = -1, Wd = -1; double Wgb = -1, Wmb = -1, Web = -1, Wdb = -1; uint64_t weighted_bw = 0; guardfraction_bandwidth_t guardfraction_bw; - u64_dbl_t *bandwidths; + double *bandwidths; /* Can't choose exit and guard at same time */ tor_assert(rule == NO_WEIGHTING || @@ -2318,7 +2326,7 @@ compute_weighted_bandwidths(const smartlist_t *sl, Web /= weight_scale; Wdb /= weight_scale; - bandwidths = tor_calloc(smartlist_len(sl), sizeof(u64_dbl_t)); + bandwidths = tor_calloc(smartlist_len(sl), sizeof(double)); // Cycle through smartlist and total the bandwidth. static int warned_missing_bw = 0; @@ -2405,7 +2413,7 @@ compute_weighted_bandwidths(const smartlist_t *sl, final_weight = weight*this_bw; } - bandwidths[node_sl_idx].dbl = final_weight + 0.5; + bandwidths[node_sl_idx] = final_weight + 0.5; } SMARTLIST_FOREACH_END(node); log_debug(LD_CIRC, "Generated weighted bandwidths for rule %s based " @@ -2426,7 +2434,7 @@ double frac_nodes_with_descriptors(const smartlist_t *sl, bandwidth_weight_rule_t rule) { - u64_dbl_t *bandwidths = NULL; + double *bandwidths = NULL; double total, present; if (smartlist_len(sl) == 0) @@ -2443,7 +2451,7 @@ frac_nodes_with_descriptors(const smartlist_t *sl, total = present = 0.0; SMARTLIST_FOREACH_BEGIN(sl, const node_t *, node) { - const double bw = bandwidths[node_sl_idx].dbl; + const double bw = bandwidths[node_sl_idx]; total += bw; if (node_has_descriptor(node)) present += bw; diff --git a/src/or/routerlist.h b/src/or/routerlist.h index bc48c2087..2166cf511 100644 --- a/src/or/routerlist.h +++ b/src/or/routerlist.h @@ -217,17 +217,10 @@ int hex_digest_nickname_matches(const char *hexdigest, const char *nickname, int is_named); #ifdef ROUTERLIST_PRIVATE -/** Helper type for choosing routers by bandwidth: contains a union of - * double and uint64_t. Before we call scale_array_elements_to_u64, it holds - * a double; after, it holds a uint64_t. */ -typedef union u64_dbl_t { - uint64_t u64; - double dbl; -} u64_dbl_t; - -STATIC int choose_array_element_by_weight(const u64_dbl_t *entries, +STATIC int choose_array_element_by_weight(const uint64_t *entries, int n_entries); -STATIC void scale_array_elements_to_u64(u64_dbl_t *entries, int n_entries, +STATIC void scale_array_elements_to_u64(uint64_t *entries_out, + const double *entries_in, int n_entries, uint64_t *total_out); STATIC const routerstatus_t *router_pick_directory_server_impl( dirinfo_type_t auth, int flags, diff --git a/src/test/test_dir.c b/src/test/test_dir.c index ea179fb02..747c4c4aa 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -2149,56 +2149,57 @@ test_dir_scale_bw(void *testdata) 1.0/7, 12.0, 24.0 }; - u64_dbl_t vals[8]; + double vals_dbl[8]; + uint64_t vals_u64[8]; uint64_t total; int i; (void) testdata; for (i=0; i<8; ++i) - vals[i].dbl = v[i]; + vals_dbl[i] = v[i]; - scale_array_elements_to_u64(vals, 8, &total); + scale_array_elements_to_u64(vals_u64, vals_dbl, 8, &total); tt_int_op((int)total, OP_EQ, 48); total = 0; for (i=0; i<8; ++i) { - total += vals[i].u64; + total += vals_u64[i]; } tt_assert(total >= (U64_LITERAL(1)<<60)); tt_assert(total <= (U64_LITERAL(1)<<62)); for (i=0; i<8; ++i) { /* vals[2].u64 is the scaled value of 1.0 */ - double ratio = ((double)vals[i].u64) / vals[2].u64; + double ratio = ((double)vals_u64[i]) / vals_u64[2]; tt_double_op(fabs(ratio - v[i]), OP_LT, .00001); } /* test handling of no entries */ total = 1; - scale_array_elements_to_u64(vals, 0, &total); + scale_array_elements_to_u64(vals_u64, vals_dbl, 0, &total); tt_assert(total == 0); /* make sure we don't read the array when we have no entries * may require compiler flags to catch NULL dereferences */ total = 1; - scale_array_elements_to_u64(NULL, 0, &total); + scale_array_elements_to_u64(NULL, NULL, 0, &total); tt_assert(total == 0); - scale_array_elements_to_u64(NULL, 0, NULL); + scale_array_elements_to_u64(NULL, NULL, 0, NULL); /* test handling of zero totals */ total = 1; - vals[0].dbl = 0.0; - scale_array_elements_to_u64(vals, 1, &total); + vals_dbl[0] = 0.0; + scale_array_elements_to_u64(vals_u64, vals_dbl, 1, &total); tt_assert(total == 0); - tt_assert(vals[0].u64 == 0); + tt_assert(vals_u64[0] == 0); - vals[0].dbl = 0.0; - vals[1].dbl = 0.0; - scale_array_elements_to_u64(vals, 2, NULL); - tt_assert(vals[0].u64 == 0); - tt_assert(vals[1].u64 == 0); + vals_dbl[0] = 0.0; + vals_dbl[1] = 0.0; + scale_array_elements_to_u64(vals_u64, vals_dbl, 2, NULL); + tt_assert(vals_u64[0] == 0); + tt_assert(vals_u64[1] == 0); done: ; @@ -2209,7 +2210,7 @@ test_dir_random_weighted(void *testdata) { int histogram[10]; uint64_t vals[10] = {3,1,2,4,6,0,7,5,8,9}, total=0; - u64_dbl_t inp[10]; + uint64_t inp_u64[10]; int i, choice; const int n = 50000; double max_sq_error; @@ -2219,12 +2220,12 @@ test_dir_random_weighted(void *testdata) * in a scrambled order to make sure we don't depend on order. */ memset(histogram,0,sizeof(histogram)); for (i=0; i<10; ++i) { - inp[i].u64 = vals[i]; + inp_u64[i] = vals[i]; total += vals[i]; } tt_u64_op(total, OP_EQ, 45); for (i=0; i Date: Wed, 27 Apr 2016 10:33:46 -0400 Subject: [PATCH 3/7] Move the ctime part of choose_array_element_by_weight into di_ops This way it gets the ctime options. --- src/common/di_ops.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/common/di_ops.h | 2 ++ src/or/routerlist.c | 35 +++-------------------------------- 3 files changed, 50 insertions(+), 32 deletions(-) diff --git a/src/common/di_ops.c b/src/common/di_ops.c index 5dfe82806..e671af6fa 100644 --- a/src/common/di_ops.c +++ b/src/common/di_ops.c @@ -226,3 +226,48 @@ safe_mem_is_zero(const void *mem, size_t sz) return 1 & ((total - 1) >> 8); } +/** Time-invariant 64-bit greater-than; works on two integers in the range + * (0,INT64_MAX). */ +#if SIZEOF_VOID_P == 8 +#define gt_i64_timei(a,b) ((a) > (b)) +#else +static inline int +gt_i64_timei(uint64_t a, uint64_t b) +{ + int64_t diff = (int64_t) (b - a); + int res = diff >> 63; + return res & 1; +} +#endif + +/** + * Given an array of list of n_entries uint64_t values, whose sum is + * total, find the first i such that the total of all elements 0...i is + * greater than rand_val. + * + * Try to perform this operation in a constant-time way. + */ +int +select_array_member_cumulative_timei(const uint64_t *entries, int n_entries, + uint64_t total, uint64_t rand_val) +{ + int i, i_chosen=-1, n_chosen=0; + uint64_t total_so_far = 0; + + for (i = 0; i < n_entries; ++i) { + total_so_far += entries[i]; + if (gt_i64_timei(total_so_far, rand_val)) { + i_chosen = i; + n_chosen++; + /* Set rand_val to INT64_MAX rather than stopping the loop. This way, + * the time we spend in the loop does not leak which element we chose. */ + rand_val = INT64_MAX; + } + } + tor_assert(total_so_far == total); + tor_assert(n_chosen == 1); + tor_assert(i_chosen >= 0); + tor_assert(i_chosen < n_entries); + + return i_chosen; +} diff --git a/src/common/di_ops.h b/src/common/di_ops.h index 6e77b5cfd..f1050a00d 100644 --- a/src/common/di_ops.h +++ b/src/common/di_ops.h @@ -42,6 +42,8 @@ void dimap_add_entry(di_digest256_map_t **map, const uint8_t *key, void *val); void *dimap_search(const di_digest256_map_t *map, const uint8_t *key, void *dflt_val); +int select_array_member_cumulative_timei(const uint64_t *entries, int n_entries, + uint64_t total, uint64_t rand_val); #endif diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 620b61449..b6247a171 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2103,20 +2103,6 @@ scale_array_elements_to_u64(uint64_t *entries_out, const double *entries_in, #undef SCALE_TO_U64_MAX } -/** Time-invariant 64-bit greater-than; works on two integers in the range - * (0,INT64_MAX). */ -#if SIZEOF_VOID_P == 8 -#define gt_i64_timei(a,b) ((a) > (b)) -#else -static inline int -gt_i64_timei(uint64_t a, uint64_t b) -{ - int64_t diff = (int64_t) (b - a); - int res = diff >> 63; - return res & 1; -} -#endif - /** Pick a random element of n_entries-element array entries, * choosing each element with a probability proportional to its (uint64_t) * value, and return the index of that element. If all elements are 0, choose @@ -2125,8 +2111,7 @@ gt_i64_timei(uint64_t a, uint64_t b) STATIC int choose_array_element_by_weight(const uint64_t *entries, int n_entries) { - int i, i_chosen=-1, n_chosen=0; - uint64_t total_so_far = 0; + int i; uint64_t rand_val; uint64_t total = 0; @@ -2143,22 +2128,8 @@ choose_array_element_by_weight(const uint64_t *entries, int n_entries) rand_val = crypto_rand_uint64(total); - for (i = 0; i < n_entries; ++i) { - total_so_far += entries[i]; - if (gt_i64_timei(total_so_far, rand_val)) { - i_chosen = i; - n_chosen++; - /* Set rand_val to INT64_MAX rather than stopping the loop. This way, - * the time we spend in the loop does not leak which element we chose. */ - rand_val = INT64_MAX; - } - } - tor_assert(total_so_far == total); - tor_assert(n_chosen == 1); - tor_assert(i_chosen >= 0); - tor_assert(i_chosen < n_entries); - - return i_chosen; + return select_array_member_cumulative_timei( + entries, n_entries, total, rand_val); } /** When weighting bridges, enforce these values as lower and upper From ef011099327e5547b4786e4c911b7f107aea8306 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 12 May 2016 11:21:07 -0400 Subject: [PATCH 4/7] Rename SOURCES to SRC for things in include.am --- src/common/include.am | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/common/include.am b/src/common/include.am index 96fc329aa..33baa1200 100644 --- a/src/common/include.am +++ b/src/common/include.am @@ -62,16 +62,16 @@ else readpassphrase_source= endif -LIBOR_CTIME_A_SOURCES = \ +LIBOR_CTIME_A_SRC = \ src/ext/csiphash.c \ src/common/di_ops.c -src_common_libor_ctime_a_SOURCES = $(LIBOR_CTIME_A_SOURCES) -src_common_libor_ctime_testing_a_SOURCES = $(LIBOR_CTIME_A_SOURCES) +src_common_libor_ctime_a_SOURCES = $(LIBOR_CTIME_A_SRC) +src_common_libor_ctime_testing_a_SOURCES = $(LIBOR_CTIME_A_SRC) src_common_libor_ctime_a_CFLAGS = @CFLAGS_CONSTTIME@ src_common_libor_ctime_testing_a_CFLAGS = @CFLAGS_CONSTTIME@ $(TEST_CFLAGS) -LIBOR_A_SOURCES = \ +LIBOR_A_SRC = \ src/common/address.c \ src/common/backtrace.c \ src/common/compat.c \ @@ -92,7 +92,7 @@ LIBOR_A_SOURCES = \ src/common/src_common_libor_testing_a-log.$(OBJEXT) \ src/common/log.$(OBJEXT): micro-revision.i -LIBOR_CRYPTO_A_SOURCES = \ +LIBOR_CRYPTO_A_SRC = \ src/common/aes.c \ src/common/crypto.c \ src/common/crypto_pwbox.c \ @@ -104,17 +104,17 @@ LIBOR_CRYPTO_A_SOURCES = \ src/common/crypto_curve25519.c \ src/common/crypto_ed25519.c -LIBOR_EVENT_A_SOURCES = \ +LIBOR_EVENT_A_SRC = \ src/common/compat_libevent.c \ src/common/procmon.c -src_common_libor_a_SOURCES = $(LIBOR_A_SOURCES) -src_common_libor_crypto_a_SOURCES = $(LIBOR_CRYPTO_A_SOURCES) -src_common_libor_event_a_SOURCES = $(LIBOR_EVENT_A_SOURCES) +src_common_libor_a_SOURCES = $(LIBOR_A_SRC) +src_common_libor_crypto_a_SOURCES = $(LIBOR_CRYPTO_A_SRC) +src_common_libor_event_a_SOURCES = $(LIBOR_EVENT_A_SRC) -src_common_libor_testing_a_SOURCES = $(LIBOR_A_SOURCES) -src_common_libor_crypto_testing_a_SOURCES = $(LIBOR_CRYPTO_A_SOURCES) -src_common_libor_event_testing_a_SOURCES = $(LIBOR_EVENT_A_SOURCES) +src_common_libor_testing_a_SOURCES = $(LIBOR_A_SRC) +src_common_libor_crypto_testing_a_SOURCES = $(LIBOR_CRYPTO_A_SRC) +src_common_libor_event_testing_a_SOURCES = $(LIBOR_EVENT_A_SRC) src_common_libor_testing_a_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) src_common_libor_crypto_testing_a_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) From b1dce55b8236cc7ecf66f87c0bd96b0dcf874fcc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 2 May 2016 12:56:18 -0400 Subject: [PATCH 5/7] Do not apply bugtrapping flags to test-memwipe, since testing memwipe requires bugs. Fixes bug 18901. --- src/test/include.am | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/include.am b/src/test/include.am index c4ef30fe0..b71415212 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -170,9 +170,9 @@ src_test_test_slow_LDADD = $(src_test_test_LDADD) src_test_test_slow_LDFLAGS = $(src_test_test_LDFLAGS) src_test_test_memwipe_CPPFLAGS = $(src_test_test_CPPFLAGS) -src_test_test_memwipe_CFLAGS = $(src_test_test_CFLAGS) +src_test_test_memwipe_CFLAGS = $(TEST_CFLAGS) src_test_test_memwipe_LDADD = $(src_test_test_LDADD) -src_test_test_memwipe_LDFLAGS = $(src_test_test_LDFLAGS) +src_test_test_memwipe_LDFLAGS = $(src_test_test_LDFLAGS) @CFLAGS_BUGTRAP@ src_test_bench_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \ @TOR_LDFLAGS_libevent@ From a3615a988ebb1181ae636d0f9111ade0f1d3f6cb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 12 May 2016 12:54:15 -0400 Subject: [PATCH 6/7] Prefer builtin true. --- configure.ac | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 626be6c11..0a78a9f9c 100644 --- a/configure.ac +++ b/configure.ac @@ -787,13 +787,13 @@ m4_ifdef([AS_VAR_IF],[ TOR_CHECK_CFLAGS(-fPIE) TOR_CHECK_LDFLAGS(-pie, "$all_ldflags_for_check", "$all_libs_for_check") fi - TOR_TRY_COMPILE_WITH_CFLAGS(-ftrapv, , CFLAGS_FTRAPV="-ftrapv", /bin/true) - TOR_TRY_COMPILE_WITH_CFLAGS(-fwrapv, , CFLAGS_FWRAPV="-fwrapv", /bin/true) + TOR_TRY_COMPILE_WITH_CFLAGS(-ftrapv, , CFLAGS_FTRAPV="-ftrapv", true) + TOR_TRY_COMPILE_WITH_CFLAGS(-fwrapv, , CFLAGS_FWRAPV="-fwrapv", true) fi if test "x$enable_expensive_hardening" = "xyes"; then - TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=address], , CFLAGS_ASAN="-fsanitize=address", /bin/true) - TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=undefined], , CFLAGS_UBSAN="-fsanitize=undefined", /bin/true) + TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=address], , CFLAGS_ASAN="-fsanitize=address", true) + TOR_TRY_COMPILE_WITH_CFLAGS([-fsanitize=undefined], , CFLAGS_UBSAN="-fsanitize=undefined", true) TOR_CHECK_CFLAGS([-fno-omit-frame-pointer]) fi From fb999abea6edc99f151f546a7a4a60cf5b5f221b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 12 May 2016 12:56:47 -0400 Subject: [PATCH 7/7] Document why we build memwipe that way. --- src/test/include.am | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/include.am b/src/test/include.am index b71415212..d2909b2dc 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -170,8 +170,11 @@ src_test_test_slow_LDADD = $(src_test_test_LDADD) src_test_test_slow_LDFLAGS = $(src_test_test_LDFLAGS) src_test_test_memwipe_CPPFLAGS = $(src_test_test_CPPFLAGS) +# Don't use bugtrap cflags here: memwipe tests require memory violations. src_test_test_memwipe_CFLAGS = $(TEST_CFLAGS) src_test_test_memwipe_LDADD = $(src_test_test_LDADD) +# The LDFLAGS need to include the bugtrap cflags, or else we won't link +# successfully with the libraries built with them. src_test_test_memwipe_LDFLAGS = $(src_test_test_LDFLAGS) @CFLAGS_BUGTRAP@ src_test_bench_LDFLAGS = @TOR_LDFLAGS_zlib@ @TOR_LDFLAGS_openssl@ \