Today we are announcing the security audit results from five expert security auditors who we contracted last year. We hired these experts to conduct comprehensive security and design audits in support of the Overwinter and Sapling releases. The summary and implication of the results are available here in addition to the result details, our reasoning and response below.
Least Authority, Part 1
Least Authority have published their work.
Issue A: pow leaks in windowed_exp
… and …
Issue B: Exponent leaks via power function
These issues correctly identify cache-snooping attacks that are exploitable by an attacker who is able to run code on the same core. Zcash is not intended to be safe to side-channel attacks in these scenarios, and this is listed in the security warnings in our repository.
Issue C: Undefined behavior in crypto/common.h
We raised issue #1249 which was then later closed by PR 3181, which includes a commit that clarifies the pointer size and number of points in a way that works on all platforms.
Issue D: Undefined behavior in CBaseDataStream::read
We raised issue #3182 which was then later closed by PR 3183 which added explicit checks for a NULL pointer value before the value gets used.
Issue E: CTxMemPool::check() does nothing when turned on
We raised issue #3134 which was then later closed by PR 3160 which adds an explicit cast as recommended in the issue, and also adds a test to mempool_tests.cpp to check for regressions in this issue.
Issue F: Transaction expiry reduces safety in reorgs
Since this issue was not listed as a vulnerability, no changes were made in response to this issue.
Coinspect
Coinspect have published their assessment of Overwinter.
The numbering of these issues is a continuation of the work that Coinspect did on Zcash in 2016.
ZEC-012: Transaction Expiry Enables Node Isolation Attack High
This attack describes the sequence of events that could use a combination of transaction expiry and network banning to isolate any node from the network.
ZEC-013: Transaction Expiry Enables Transaction Flooding at No Cost
We made modifications to our code to fix both these issues in PR 3144.
Placeholder issues #3141 and #3142 were used prior to the above implementation.
NCC Group
NCC have published their work.
Overwinter Issues
NCC-2018-001: Asynchronous Chain Updates Can Lead To Network Bans
This issue is similar to those discovered by Coinspect and remediated there (see Coinspect audit results post). The report includes a retest of this issue, referring to the fix commit which was included in PR 3148 and fixes both the Coinspect reported issues and this one to a level we are satisfied with.
NCC-2018-002: Mempool Not Properly Cleared if Connecting or Disconnecting Tips Fails
We decided to temporarily accept this small risk, and it ended up not causing a problem during activation of Overwinter or Sapling. We have raised issue #3797 to track for future network upgrades.
Sapling Issues
These are ordered to match the report to make the task of correlating the findings in the report with our responses simpler.
NCC-2018-004: Curve BLS12-381 Security Is Less Than 128 Bits
As the issue rightly points out, the existing analysis of this curve puts a practical attack well beyond the foreseeable computing power available to humanity, although we will monitor the research situation as time progresses.
The research mentioned in the issue was also referenced in our blog post on curve selection.
We are satisfied that use of this curve strikes an appropriate balance between security and performance.
NCC-2018-009: RedDSA / RedJubjub Are Not Strongly Unforgeable With Re-Randomized Keys
This issue correctly identified an error in the construction of the RedDSA signature scheme relative to its claimed security properties. There are two relevant ways of formalizing unforgeability for signature schemes: “existential unforgeability” (sufficient for most uses) and “strong unforgeability” (also preventing some forms of malleability). The latter property was claimed but not met in general by the specified scheme, even though it was met (for a designed reason, not by accident) for its uses in Zcash. This error has been corrected in version 2018.0-beta-20 and later of the protocol specification, so that the scheme is now conjectured (and provisionally proven, subject to further review of the proof) to meet the stronger property in general. The relevant change was to move responsibility for ensuring that the public key is included in the hash function input, from the protocol using RedDSA to the RedDSA scheme itself. This correction did not require any visible consensus rule change for Zcash, but it makes RedDSA more safely reusable in other protocols.
NCC-2018-010: Bit Ordering Convention Leads To Mismatches And Confusion
We took the point that the bit ordering was confusing and issued PR #72 in sapling-crypto to move the implementation to all little-endian and made changes to reflect this in the specification, which were logged in the changelog under version 2018.0-beta-15 of the protocol specification.
NCC-2018-003: Private Key Decoding Has Limited Interoperability
We opened issue #3155 to discuss this but since the issue is not exploitable, no changes have been made to the code in response.
NCC-2018-007: Discrepancies Between Specification and Circuit Implementation
We have harmonized the implementation and protocol by altering the definition in section 5.4.7.2 “Windowed Pederson commitments” to reflect the order of operations used in the circuit implementation.
NCC-2018-008: Typographical Inconsistencies in Zcash Specification
This finding was addressed in four parts:
- We have modified the specification to match the order of the two commutative inputs for those definitions to provide clarity in the specification;
- We have corrected the definition of Pederson hash chunking in the specification to bring it in line with the implementation;
- We have corrected the typo, both words now read “RedDSA.RandomizePublic”;
- We have added the definition used in the implementation.
The changes recommended in this issue were made in version 2018.0-beta-20 of the protocol specification and are listed in the changelog for that version.
NCC-2018-011: Bias in Random Field Element Generators for Underlying Representation
This issue is listed as not exploitable and no changes have been made in response to this issue.
Least Authority, Part 2
Least Authority have published their work.
Issue A: Spec is inconsistent regarding fOverwintered
This was indeed a typo, as the issue text suggests, which was fixed in a later version. Additionally, we took their general advice to make consensus rules more clear and provided Sapling- and Overwinter-specific consensus changes in different colors for clarity.
Issue B: Transparent address encoding is compressed
This was a legitimate oversight on our part and the text has been updated in-line with the recommendation from Least Authority. Section 5.6.1 now reads “of a compressed ECDSA key encoding”.
Kudelski Security
Kudelski Security have published a blog post on the work they did for us. Their official report is available.
PAIRING-001: Unbounded exponent in exponentiation
The concern was that without an upper bound on the 64-bit integers passed into this part of the code, performance might be degraded to the point of causing a denial-of-service condition. In fact, the calling code ensures that such large numbers are never passed to this location, and we decided that adding a redundant check would incur an unacceptable performance penalty for no gain. Other users of this library should be sure to use it safely, too.
PAIRING-002: RNG choice delegated to callers
We deliberately left the choice of random number generators to the caller to improve testability of the code. In Zcash, we’re happy that the rng we pass to the library is appropriate.
BELLMAN-001: Outdated and unmaintained dependencies
While it’s true that these dependencies are marked as unmaintained, we do not currently believe them to be dangerous. Nevertheless we’ve added them to our list of dependencies to swap out and will do so at a more appropriate time.
BELLMAN-002: Lossy cast to usize
The use of this variable is defined as being limited to the scope of available memory, and so its use as a length is at least theoretically-valid; however, code changes were made to prevent an out-of-range value from being lost. Specifically the storage of that value is now 32-bit throughout. In practice this makes no appreciable difference since the elements are each 96 bytes long. Any error handling resulting from indexes that are too large to be stored in the array should be handled by the caller.
These changes were merged into master of zkcrypto/bellman PR 22.
BELLMAN-003: Potential DoS through user-controlled length argument
Although the length value might be user-controlled, each time the loop over len executes, it calls a generic reader interface provided by the caller (e.g. through read_g1 to R) which should fail in such a way that the loop does not execute again when no more data is available, and so no attempt is made to address memory beyond the available values. It is the responsibility of the caller to prevent the DoS condition within this design pattern, and that is the case in Zcash.
QED-it
QED-it have published a blog post on the work they did for us. Their official report is available from their site.
Issues
Incorrect assertion – Pedersen hash generators linear relation validation
… and …
Window base patterns generation not matching the spec
These two issues refer to an unmerged pull request to sapling-crypto. That PR has been closed and superseded by PR 95, which only changes comments and does not have the mentioned issues.
jubjub::Fs usage is sometimes confusing regarding the prime subgroup
Fs is intended to represent scalars in the prime subgroup. It is not possible to make the Jubjub implementation only usable within the prime subgroup because the specification allows points outside the subgroup to appear in transaction fields, for performance reasons. We acknowledge that this requires extreme care; alternatives such as Ristretto were not sufficiently developed/understood at the time most of Sapling was designed, and have disadvantages for circuit efficiency.
We filed issue #89 to consider other approaches in future protocol upgrades.
Pedersen hash circuit implementation can not calculate inputs larger than 63*3*4
This does not affect correctness of the circuit for the input sizes used, but will be fixed. We filed issue #90 to track this.
The bit unpacking between layers of the Merkle tree
In fact it does not reduce the concrete security of the Merkle tree, even by a fraction of a bit. Observe that in attempting to find a second-preimage authentication path for a given root, an adversary has a free choice of the sibling hashes in the path. The choice of congruency gives the adversary no additional power, since at each node in the path, varying the congruency would produce a different root hash in the same way as varying the sibling hash. This issue was considered in the design. The only property needed of the Pedersen hash MerkleCRHSapling, in order for a different congruency to produce a different root hash, is that it is collision-resistant [*] on bit-sequence inputs. This tightly reduces to hardness of the DLP on the prime-order subgroup of Jubjub, assuming that FindGroupHash outputs generators that are randomly distributed on this subgroup.
[*] Collision-resistance of the hash function implies collision resistance of the Merkle tree, which in turn implies second-preimage resistance of the Merkle tree. In the general case, there are subtle technical obstacles to proving second-preimage resistance of the tree based only on second-preimage resistance of the hash (the heuristic of proving security for a keyed hash and extrapolating to an unkeyed hash does not work). In this context we have to assume hardness of DLP on the prime-order subgroup of Jubjub in any case for the rest of the Sapling protocol, so there would be no benefit to making a second-preimage assumption about MerkleCRHSapling (in fact, breaking DLP breaks second-preimage resistance as well as collision resistance of this hash, so the two assumptions are equivalent in strength).
Testing is done with another implementation of ConstraintSystem
Note that both implementations (here and here) are very simple and short; they are essentially only keeping track of lists of auxiliary variables, inputs, and R1CS constraints. On the other hand, it does seem as though it might be possible to alleviate this concern for future reviews by sharing more code. This is filed as issue #92.
params.montgomery_2a doesn’t seem to be used anywhere
Indeed, it only seems to be used in this test. This issue is filed as issue #93.
Spec notation mismatches and missing parts
Most of these observations have merit, the most severe of them were fixed right away, and for the remainder we have filed issues: #176, #177, #180, #181 and #182.
In section A.3.3, Theorem A.3.3 is actually stated for any Montgomery curve, so it is was incorrect to reference A𝕄 or B𝕄 in the proof; it should have referenced just A and B. This is fixed in version 2018.0-beta-31.
In respect to Theorem A.3.2, the intended condition was “u = 0 or 1 − v = 0”. That there are also no other points in the subgroup with v = 0 is true, but not needed. This is fixed in version 2018.0-beta-31 (the correction to the proof requires a property of complete twisted Edwards curves that was not previously mentioned, so it wasn’t entirely trivial).
We filed issue #183 for the specification and issue #91 for the sapling-crypto implementation.
Least Authority, Part 3
Least Authority have published their work.
Issue A: RPC HTTP Server is Vulnerable to DNS-Rebinding Attacks
We have a password mechanism in place, and even recently selectively incorporated Bitcoin’s auth cookies in the case where you haven’t set a password. Nevertheless, in response to this finding we have updated our documentation on the use of passwords in the RPC interface, and have raised issue #3791 to look at further defense-in-depth measures.
Issue B: Instance of Undefined Behavior
This is a useful finding, and we have raised issue #3792 to fix it. Note that it is Low impact and requires RPC authentication to access.
Issue C: Transaction Details Disclosure Vector
This is a useful finding, and we have raised issue #3793 to discuss possible changes to improve this situation. Note that users should never run zcashd on the same computer that adversarial users can execute code on, because of known weakness to side-channel attacks.
Issue D: Address Interpretation Surprise
Bech32 encoding is not accepted for Sapling addresses, however we have opened issue #3794 and plan to add tests that confirm this to be the case on a continuous basis.
Issue E: Unclear Transaction Interpretation
We have opened issue #3795 for this issue to remove the ambiguity here so that users don’t get surprised.
Suggestion1: Strengthen the wallet password system
We have opened issue #3796 to discuss the ways we could improve the wallet encryption experience for users.