Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: audit notes #91

Merged
merged 2 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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

Expand Down
65 changes: 65 additions & 0 deletions docs/quarkslab-audit/README.md
Original file line number Diff line number Diff line change
@@ -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.
Binary file added docs/quarkslab-audit/report.pdf
Binary file not shown.