From a0aa9606fdf10574b38845e1b48e89a8913c20c6 Mon Sep 17 00:00:00 2001 From: Andrew Gross Date: Mon, 5 Nov 2018 22:15:56 -0700 Subject: [PATCH] Linked to security audit in POA wiki --- README.md | 6 ++-- SECURITY_AUDIT.md | 91 ----------------------------------------------- 2 files changed, 3 insertions(+), 94 deletions(-) delete mode 100644 SECURITY_AUDIT.md diff --git a/README.md b/README.md index 922f4f6..02c8520 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ decrypted and authenticated only with cooperation from at least `threshold + ## Security Audit -An [official security audit](SECURITY_AUDIT.md) has been completed on `threshold_crypto` by [Jean-Philippe Aumasson](https://aumasson.jp/). No exploitable security issues were found, and potential improvements have been addressed. +An [official security audit](https://github.com/poanetwork/wiki/wiki/Threshold-Crypto-Audit) has been completed on `threshold_crypto` by [Jean-Philippe Aumasson](https://aumasson.jp/). No exploitable security issues were found, and potential improvements have been addressed. Outdated dependencies mentioned in the audit were updated in https://github.com/poanetwork/threshold_crypto/commit/54026f5fe7e0b5a52e446ac01a50469da1f15a71 with the exception of rand, which is currently pinned to version 0.4 (see https://github.com/poanetwork/hbbft/issues/145 for details). ## Usage @@ -52,7 +52,7 @@ fn main() { ### Testing -Run tests using the following command: +Run tests with: ``` $ cargo test @@ -117,7 +117,7 @@ We use the [`criterion`](https://crates.io/crates/criterion) benchmarking librar ### Mock cryptography -To speed up automatic tests of crates depending on `threshold_crypto`, the `use-insecure-test-only-mock-crypto` feature is available. **Activating this feature will effectively disable encryption and should only be used during tests!**. Essentially, the underlying elliptic curves will be replaced by small finite fields, yielding a 10-200X speed-up in execution. The resulting ciphers can be trivially broken in a number of ways and should never be used in production. +To speed up automatic tests of crates depending on `threshold_crypto`, the `use-insecure-test-only-mock-crypto` feature is available. **Activating this feature will effectively disable encryption and should only be used during tests!** Essentially, the underlying elliptic curves will be replaced by small finite fields, yielding a 10-200X speed-up in execution. The resulting ciphers can be trivially broken in a number of ways and should never be used in production. ## License diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md deleted file mode 100644 index 7cfff00..0000000 --- a/SECURITY_AUDIT.md +++ /dev/null @@ -1,91 +0,0 @@ -% POA Network threshold crypto audit -% Jean-Philippe Aumasson -% 20181024 - -# Introduction - -We reviewed for security defects (branch master, e28b77d). - -This implements the pairing-based threshold signature and encryption schemes, based on the [Boneh-Lynn-Shacham](https://www.iacr.org/archive/asiacrypt2001/22480516.pdf) pairing-based signature scheme, on its extension to threshold signatures using Lagrange interpolation by [Boldyreva](https://eprint.iacr.org/2002/118.pdf), and on the composition with an IND-CCA encryption construction as described by [Baek and Zheng](http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.119.1717&rep=rep1&type=pdf) (a.k.a. "signcryption"). - -The elliptic-curve arithmetic and pairings are over the BLS12 elliptic curve, and is based on Sean Bowe's `pairing` crate (also used in Zcash), which we have audited in the past months. - -We reviewed the source code by manual inspection, and used modified versions of the unit tests in order to perform more testing of edge cases, malformed inputs, and so on. -We have checked the following points (non-exhaustive list): - -* Primitives (curve, hash) and their implementation -* Use of rand? OsRng used for secret stuff -* Exploitable panics -* Unsafe unwraps -* Memory zeroizing completeness -* Equivalent keys using signed/unsigned indices -* Leakage from ciphertext data structure -* Incomplete/insufficient verification -* Threshold enforcement and edge cases -* Dependencies security/versions - -We did not find any exploitable security issue in the logic nor implementation of the threshold schemes, but only report potential improvements in the section Observations below. - -The audit work took approximately 6h30, including the review of -literature related to the threshold cryptosystems implemented, and the -review of patches following our initial observations. - -# Observations - -## Impossible error? - -``` -impl IntoFr for u64 { - fn into_fr(self) -> Fr { - Fr::from_repr(self.into()).expect("modulus is greater than u64::MAX") - } -} -``` -Can this really fail, since the trait is only defined for `u64` types, which cant be greater than `u64::MAX`? -(We noticed this when trying to overflow the value / crash decryption.) - -### Status - -Failure is indeed impossible, but the `expect()` is meant as an -clarification for the reader that the Result obtained must be less than -`u64::MAX`. - -## Set a threshold threshold? - -Threshold value in SecretKeySet::random() is unlimited (but to `usize`), high values can panic, -e.g. with `2^64-1` we get `thread 'tests::test_threshold_sig' panicked at 'capacity overflow', liballoc/raw_vec.rs:754:5`. - -Another panic occurs if calling `SecretKeySet::random(0, &mut rng);`. - -### Status - -The overflow was fixed, and we reviewed the patch. - -## Xored buffers length check - -The xor function does not check that input slices have an equal length, which is ok in the current context, since this function is always called with same-length slices. - -``` -fn xor_vec(x: &[u8], y: &[u8]) -> Vec { - x.iter().zip(y).map(|(a, b)| a ^ b).collect() -} -``` - -### Status - -This was addressed by integrating this operation in the hashing logic. - -## Outdated dependencies - -* byteorder (1.2.3 instead of 1.2.6) -* log (0.4.1 instead of 0.4.5) -* rand (0.4.2 instead of 0.5.5); however note that 0.5 and above have made API changes breaking backward compatibility -* rand_derive (0.3.1 instead of 0.5.0) -* serde (1.0.55 instead of 1.0.79) -* serde_derive (1.0.55 instead of 1.0.79) - -### Status - -The versions had not been updated at the time of our second review, but -should be before the next release. -