diff --git a/README.md b/README.md index d7ceba7..133be66 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ In particular, it supports: - **Proof aggregation**. You can generate a proof containing multiple range assertions in an efficient way. - **Extended commitments**. Commitments may contain multiple masks. - **Batch verification**. Verifying a set of multiple proofs is extremely fast. -- **Minimum value promises**. You can additionally prove that a commitment binds to at least a specified value. +- **Minimum value promises**. You can prove that a commitment binds to at least a specified value. - **Mask extraction**. If the prover and verifier agree on a shared secret, the verifier can use it to recover the mask used for the commitment in a non-aggregated proof. Compared to an [updated fork](https://github.com/tari-project/bulletproofs) of the `dalek-cryptography` [Bulletproofs](https://github.com/dalek-cryptography/bulletproofs) implementation, this Bulletproofs+ implementation is: @@ -25,7 +25,7 @@ Compared to an [updated fork](https://github.com/tari-project/bulletproofs) of t As always, your mileage may vary. -This implementation uses the excellent [`zeroize`](https://crates.io/crates/zeroize) library to make a best-effort approach at minimizing exposure of value- and mask-related data in memory, as this is often considered sensitive. However, it is difficult in general to guarantee that there are no coding patterns leading to [unintended copies](https://docs.rs/zeroize/1.6.0/zeroize/#stackheap-zeroing-notes) of data, so care should always be taken not to make too many assumptions about the contents of memory. +This library underwent a code audit by [Quarkslab](https://www.quarkslab.com/) at a [specific point](https://github.com/tari-project/bulletproofs-plus/releases/tag/pre-audit-commit) in the repository history. You can read the [report and issue responses](docs/quarkslab-audit/README.md) in this repository. ## Testing diff --git a/docs/quarkslab-audit/README.md b/docs/quarkslab-audit/README.md new file mode 100644 index 0000000..fa8184e --- /dev/null +++ b/docs/quarkslab-audit/README.md @@ -0,0 +1,65 @@ +# Code audit by Quarkslab + +A code audit of this library was conducted by [Quarkslab](https://www.quarkslab.com/), primarily focusing on determination of the correctness and soundness of the implementation, as well as optimizations and extensions. +As the audit was conducted at a [specific point](https://github.com/tari-project/bulletproofs-plus/releases/tag/pre-audit-commit) in the repository history, readers should carefully note that subsequent changes may not have been examined by the auditors. + +The [full report](report.pdf) is available in this repository. + +The report did not identify any particular security issues, but provided recommendations to improve the codebase and make it more robust against future changes. +An initial draft version of the report identified three findings: `LOW 1`, `INFO 1`, and `INFO 2`. +We provide context and responses to each finding here, but direct readers to the full report for details. + +## `LOW 1` + +This issue noted that the [`merlin`](https://crates.io/crates/merlin) crate used by the implementation for Fiat-Shamir transcripting appears to be unmaintained, as it had not been updated in over two years at the time of writing. +The report recommended that Tari produce a fork of this repository. + +### Response + +To our knowledge, no security-related issues have been reported that would render the use of the `merlin` crate unsafe. +Further, no particular dependency conflicts have been identified due to the lack of apparent maintenance. + +For these reasons, producing a fork does not appear to be necessary, as this would merely increase complexity and technical debt. +As with any core dependency, any future security-related issues arising in `merlin` would be addressed as appropriate. + +## `INFO 1` + +This issue noted the curve library [fork](https://crates.io/crates/tari-curve25519-dalek) used in the implementation is not up to date with the [upstream](https://crates.io/crates/curve25519-dalek) repository. +The report recommended that Tari bring the fork up to date. + +### Response + +The fork was initially made due to the upstream repository being apparently unmaintained, which resulted in a number of dependency conflicts. +Additionally, the fork added support for multiscalar multiplication [partial precomputation](https://github.com/tari-project/curve25519-dalek/pull/1) that was not supported by upstream. +Since that time, upstream has undergone a flurry of activity and updates; during this process, the fork was rebased against the upstream [`4.0.0-rc.3`](https://github.com/dalek-cryptography/curve25519-dalek/releases/tag/4.0.0-rc.3) tag. +Subsequent to this rebase, the upstream repository was significantly restructured, and the fork has not been updated to reflect this. +We did not identify any particular security-related issues addressed since this time. +Unfortunately, the versioning scheme used in the fork conflicts somewhat with the upstream versioning; the fork currently is on the [`v4.0.3`](https://github.com/tari-project/curve25519-dalek/releases/tag/v4.0.3) tag. + +Regardless of the lack of known security issues, Tari agreed that updating the fork would be beneficial. +Developers are in the process of testing an updated version of the fork, in order to ensure such an update does not introduce conflicts in Tari repositories. + +Further, an open [upstream pull request](https://github.com/dalek-cryptography/curve25519-dalek/pull/546) would add partial precomputation support; if it is accepted, the implementation could change its curve library dependency to upstream. + +## `INFO 2` + +This issue ran the `clippy` linter against several lints, and identified a number of warnings arising from them. +These particularly dealt with arithmetic operation side effects, slicing, and indexing that could lead to unintended behavior or panics if triggered. +The report recommended that Tari address these warnings as appropriate, but noted that their testing and analysis did not identify particular circumstances under which unintended behavior or panics would occur. + +### Response + +Many of the identified lint warnings were for curve-related group and scalar operations, which do not introduce risk. +These warnings do not need to be addressed. + +Even though it was determined that triggering the other warnings was unlikely, Tari agreed that mitigating the warnings (as feasible) is good practice as part of a defense-in-depth approach to design. +After an early discussion with the auditors and prior to the release of an initial draft version of the report, several updates were made to the implementation that mitigated many of the warnings, particularly relating to proof deserialization and verification. +Subsequent to the release of an initial draft version of the report, additional updates were made to the implementation for further mitigation. + +There remain instances in the implementation that perform unchecked arithmetic operations or indexing where mitigation would be unwieldy, complex, and more likely to introduce errors at the expense of clarity. +These cases were examined and determined to be safe as written. +As the implementation is updated, existing and future such warnings will be carefully examined. + +Additionally, the implementation's CI workflow already included a [list](https://github.com/tari-project/bulletproofs-plus/blob/main/lints.toml) of lints that are automatically flagged during the development process. +This list has been expanded in order to better identify coding practices that could introduce problems. +We note that the arithmetic side-effect lint in question has not been added to this list, as it flags curve-related operations that are not at risk of unintended behavior. diff --git a/docs/quarkslab-audit/report.pdf b/docs/quarkslab-audit/report.pdf new file mode 100644 index 0000000..2b267e3 Binary files /dev/null and b/docs/quarkslab-audit/report.pdf differ