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

fix: apply security audit results #1196

Merged
merged 2 commits into from
Jul 9, 2020
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
18 changes: 6 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ There are currently four different crates:


- [**Filecoin Proofs (`filecoin-proofs`)**](./filecoin-proofs)
A wrapper around `storage-proofs`, providing an FFI-exported API callable from C (and in practice called by [go-filecoin](https://github.com/filecoin-project/go-filecoin') via cgo). Filecoin-specific values of setup parameters are included here, and circuit parameters generated by Filecoin’s (future) trusted setup will also live here.
A wrapper around `storage-proofs`, providing an FFI-exported API callable from C (and in practice called by [go-filecoin](https://github.com/filecoin-project/go-filecoin) via cgo). Filecoin-specific values of setup parameters are included here, and circuit parameters generated by Filecoin’s (future) trusted setup will also live here.


![FPS crate dependencies](/img/fps-dependencies.png?raw=true)
Expand Down Expand Up @@ -81,23 +81,19 @@ Running them
> cargo bench --all
```

To benchmark the examples you can [bencher](src/bin/bencher.rs).
To benchmark the examples you can use [benchy](src/bin/benchy/main.rs), [stacked](src/bin/benchy/stacked.rs), [prodbench](src/bin/benchy/prodbench.rs), [merkleproofs](src/bin/benchy/merkleproofs.rs], [winning_post](src/bin/benchy/winning_post.rs), or [window_post](src/bin/benchy/window_post.rs).

```
# build the script
> cargo build
# run the benchmarks
> ./target/debug/bencher
> ./target/release/benchy
```

The results are written into the `.bencher` directory, as JSON files. The benchmarks are controlled through the [bench.config.toml](bench.config.toml) file.
The results are displyed at the command line, or alternatively written as JSON files.

Note: On macOS you need `gtime` (`brew install gnu-time`), as the built in `time` command is not enough.

## Profiling

For development purposes we have an (experimental) support for CPU and memory profiling in Rust through a [`gperftools`](https://github.com/dignifiedquire/rust-gperftools) binding library. These can be enabled though the `cpu-profile` and `heap-profile` features in `filecoin-proofs`. An example setup can be found in this [`Dockerfile`](./Dockerfile-profile) to profile CPU usage for the [`stacked`](https://github.com/filecoin-project/rust-fil-proofs/blob/master/filecoin-proofs/examples/stacked.rs#L40-L61) example.

## Logging

For better logging with backtraces on errors, developers should use `expects` rather than `expect` on `Result<T, E>` and `Option<T>`.
Expand Down Expand Up @@ -277,15 +273,13 @@ The **FPS** is accessed from [**go-filecoin**](https://github.com/filecoin-proje
The Rust source code serves as the source of truth defining the **FPS** APIs. View the source directly:

- [**filecoin-proofs**](https://github.com/filecoin-project/rust-fil-proofs/blob/master/filecoin-proofs/src/api/mod.rs)
- [**sector-base**](https://github.com/filecoin-project/rust-fil-proofs/blob/master/sector-base/README.md#api-reference).
- [**filecoin-proofs-api**](https://github.com/filecoin-project/rust-filecoin-proofs-api).


Or better, generate the documentation locally (until repository is public). Follow the instructions to generate documentation above. Then navigate to:
- **Sector Base API:** `…/rust-fil-proofs/target/doc/sector_base/api/index.html`
- **Filecoin Proofs API:** `…/rust-fil-proofs/target/doc/filecoin_proofs/api/index.html`

- [Go implementation of filecoin-proofs sectorbuilder API](https://github.com/filecoin-project/go-filecoin/blob/master/proofs/sectorbuilder/rustsectorbuilder.go) and [associated interface structures](https://github.com/filecoin-project/go-filecoin/blob/master/proofs/sectorbuilder/interface.go).
- [Go implementation of filecoin-proofs verifier API](https://github.com/filecoin-project/go-filecoin/blob/master/proofs/rustverifier.go) and [associated interface structures](https://github.com/filecoin-project/go-filecoin/blob/master/proofs/interface.go).
- [Go implementation of filecoin-proofs sectorbuilder API](https://github.com/filecoin-project/go-sectorbuilder/blob/master/sectorbuilder.go) and [associated interface structures](https://github.com/filecoin-project/go-sectorbuilder/blob/master/interface.go).


## Contributing
Expand Down
1 change: 0 additions & 1 deletion fil-proofs-tooling/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ rayon = "1.3.0"
flexi_logger = "0.14.7"
typenum = "1.11.2"
generic-array = "0.13.2"
tempdir = "0.3.7"

[features]
default = ["gpu", "measurements"]
Expand Down
3 changes: 1 addition & 2 deletions fil-proofs-tooling/src/bin/update_tree_r_cache/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use generic_array::typenum::Unsigned;
use memmap::MmapOptions;
use merkletree::merkle::get_merkle_tree_len;
use merkletree::store::{ExternalReader, ReplicaConfig, Store, StoreConfig};
use tempdir::TempDir;

use filecoin_proofs::constants::*;
use filecoin_proofs::types::*;
Expand Down Expand Up @@ -244,7 +243,7 @@ fn run_verify(sector_size: usize, cache: PathBuf, replica_path: PathBuf) -> Resu

// Rebuild each of the tree_r_last base trees (in a new temp dir so as not to interfere
// with any existing ones on disk) and check if the roots match what's cached on disk
let tmp_dir = TempDir::new("tree-r-last-verify")?;
let tmp_dir = tempfile::tempdir().unwrap();
let tmp_path = tmp_dir.path();
create_dir_all(&tmp_path)?;

Expand Down
6 changes: 3 additions & 3 deletions storage-proofs/core/src/compound_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,18 +202,18 @@ where
/// default trait methods.
fn circuit_proofs(
pub_in: &S::PublicInputs,
vanilla_proof: Vec<S::Proof>,
vanilla_proofs: Vec<S::Proof>,
pub_params: &S::PublicParams,
groth_params: &groth16::MappedParameters<Bls12>,
priority: bool,
) -> Result<Vec<groth16::Proof<Bls12>>> {
let mut rng = OsRng;
ensure!(
!vanilla_proof.is_empty(),
!vanilla_proofs.is_empty(),
"cannot create a circuit proof over missing vanilla proofs"
);

let circuits = vanilla_proof
let circuits = vanilla_proofs
.into_par_iter()
.enumerate()
.map(|(k, vanilla_proof)| {
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/core/src/crypto/feistel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ mod tests {
// Since every element in the set is reversibly mapped to another element also in the set,
// this is indeed a permutation.
assert_eq!(i, v, "failed to permute");
assert!(p <= *n, "output number is too big");
assert!(p < *n, "output number is too big");
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions storage-proofs/core/src/sector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ impl fmt::Display for SectorId {
}

impl SectorId {
pub fn as_fr_safe(self) -> [u8; 31] {
let mut buf: [u8; 31] = [0; 31];
pub fn as_fr_safe(self) -> [u8; 32] {
let mut buf: [u8; 32] = [0; 32];
byteorder::LittleEndian::write_u64(&mut buf[0..8], self.0);
buf
}
Expand Down
1 change: 0 additions & 1 deletion storage-proofs/porep/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ hex = "0.4.2"
byteorder = "1.3.4"

[dev-dependencies]
tempdir = "0.3.7"
tempfile = "3"
rand_xorshift = "0.2.0"
criterion = "0.3.2"
Expand Down
4 changes: 4 additions & 0 deletions storage-proofs/porep/src/stacked/circuit/column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ impl Column {
}

impl AllocatedColumn {
pub fn len(&self) -> usize {
self.rows.len()
}

/// Creates the column hash of this column.
pub fn hash<CS: ConstraintSystem<Bls12>>(
&self,
Expand Down
1 change: 1 addition & 0 deletions storage-proofs/porep/src/stacked/circuit/create_label.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ where
E: JubjubEngine,
CS: ConstraintSystem<E>,
{
assert!(replica_id.len() >= 32, "replica id is too small");
assert!(replica_id.len() <= 256, "replica id is too large");
assert_eq!(parents.len(), TOTAL_PARENTS, "invalid sized parents");

Expand Down
7 changes: 6 additions & 1 deletion storage-proofs/porep/src/stacked/circuit/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ impl<Tree: MerkleTreeTrait, G: 'static + Hasher> Proof<Tree, G> {
..
} = self;

assert!(!drg_parents_proofs.is_empty());
assert!(!exp_parents_proofs.is_empty());

// -- verify initial data layer

// PrivateInput: data_leaf
Expand All @@ -112,11 +115,12 @@ impl<Tree: MerkleTreeTrait, G: 'static + Hasher> Proof<Tree, G> {
// -- verify replica column openings

// Private Inputs for the DRG parent nodes.
let mut drg_parents = Vec::new();
let mut drg_parents = Vec::with_capacity(layers);

for (i, parent) in drg_parents_proofs.into_iter().enumerate() {
let (parent_col, inclusion_path) =
parent.alloc(cs.namespace(|| format!("drg_parent_{}_num", i)))?;
assert_eq!(layers, parent_col.len());

// calculate column hash
let val = parent_col.hash(cs.namespace(|| format!("drg_parent_{}_constraint", i)))?;
Expand All @@ -136,6 +140,7 @@ impl<Tree: MerkleTreeTrait, G: 'static + Hasher> Proof<Tree, G> {
for (i, parent) in exp_parents_proofs.into_iter().enumerate() {
let (parent_col, inclusion_path) =
parent.alloc(cs.namespace(|| format!("exp_parent_{}_num", i)))?;
assert_eq!(layers, parent_col.len());

// calculate column hash
let val = parent_col.hash(cs.namespace(|| format!("exp_parent_{}_constraint", i)))?;
Expand Down
4 changes: 4 additions & 0 deletions storage-proofs/porep/src/stacked/vanilla/proof_scheme.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ impl<'a, 'c, Tree: 'static + MerkleTreeTrait, G: 'static + Hasher> ProofScheme<'
) -> bool {
let partition_challenges = public_params.layer_challenges.challenges_count_all();

assert_eq!(
partition_challenges.checked_mul(partitions),
Some(partition_challenges * partitions)
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, it would be okay if this overflows because the result would be conservative and, if anything, lead to requirements being judged unsatisfied when they 'should' be satisfied. Of course this cannot happen for correct protocol values anyway, and I agree a check is helpful for development.

That said, if this 'error' were hit in reality, it would probably mean we need to revise the code. Technically, values which would overflow could be 'correct'. None of this is a practical issue. I'm just thinking through the significance of the assertion.

I don't recommend or request it, but we could 'technically' return true if the assertion fails (even though this would undoubtedly be papering over a usage error).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'll leave in the assert as-is, but realize they should not trigger in normal circumstances and the downside is that we're calculating values more than needed. If we return true if the check fails, there's probably no reason to do it since I think even logging it would likely be lost/ignored.

partition_challenges * partitions >= requirements.minimum_challenges
}
}
1 change: 0 additions & 1 deletion storage-proofs/post/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ neptune = { version = "1.0.1", features = ["gpu"] }
num_cpus = "1.10.1"

[dev-dependencies]
tempdir = "0.3.7"
tempfile = "3"
pretty_assertions = "0.6.1"
rand_xorshift = "0.2.0"
2 changes: 1 addition & 1 deletion storage-proofs/post/src/election/circuit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ mod tests {
let mut trees = BTreeMap::new();

// Construct and store an MT using a named store.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

for i in 0..5 {
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/election/compound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ mod tests {
let mut trees = BTreeMap::new();

// Construct and store an MT using a named store.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

for i in 0..5 {
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/election/vanilla.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ mod tests {
let mut trees = BTreeMap::new();

// Construct and store an MT using a named store.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

for i in 0..5 {
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/fallback/circuit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ mod tests {
};

// Construct and store an MT using a named DiskStore.
let temp_dir = tempdir::TempDir::new("level_cache_tree_v1").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let mut pub_sectors = Vec::new();
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/fallback/compound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ mod tests {
};

// Construct and store an MT using a named DiskStore.
let temp_dir = tempdir::TempDir::new("level_cache_tree_v1").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let mut pub_sectors = Vec::new();
Expand Down
16 changes: 13 additions & 3 deletions storage-proofs/post/src/fallback/vanilla.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,18 @@ impl<'a, Tree: 'a + MerkleTreeTrait> ProofScheme<'a> for FallbackPoSt<'a, Tree>
requirements: &Self::Requirements,
partitions: usize,
) -> bool {
partitions * public_params.sector_count * public_params.challenge_count
>= requirements.minimum_challenge_count
let checked = partitions * public_params.sector_count;

assert_eq!(
partitions.checked_mul(public_params.sector_count),
Some(checked)
);
assert_eq!(
checked.checked_mul(public_params.challenge_count),
Some(checked * public_params.challenge_count)
);

checked * public_params.challenge_count >= requirements.minimum_challenge_count
}
}

Expand Down Expand Up @@ -497,7 +507,7 @@ mod tests {
let prover_id = <Tree::Hasher as Hasher>::Domain::random(rng);

// Construct and store an MT using a named DiskStore.
let temp_dir = tempdir::TempDir::new("level_cache_tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let mut pub_sectors = Vec::new();
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/rational/circuit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ mod tests {
};

// Construct and store an MT using a named DiskStore.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let (_data1, tree1) = generate_tree::<Tree, _>(rng, leaves, Some(temp_path.to_path_buf()));
Expand Down
2 changes: 1 addition & 1 deletion storage-proofs/post/src/rational/compound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ mod tests {
let pub_params = RationalPoStCompound::<Tree>::setup(&setup_params).expect("setup failed");

// Construct and store an MT using a named DiskStore.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let (_data1, tree1) = generate_tree::<Tree, _>(rng, leaves, Some(temp_path.to_path_buf()));
Expand Down
4 changes: 2 additions & 2 deletions storage-proofs/post/src/rational/vanilla.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ mod tests {
};

// Construct and store an MT using a named store.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let (_data1, tree1) = generate_tree::<Tree, _>(rng, leaves, Some(temp_path.to_path_buf()));
Expand Down Expand Up @@ -438,7 +438,7 @@ mod tests {
};

// Construct and store an MT using a named store.
let temp_dir = tempdir::TempDir::new("tree").unwrap();
let temp_dir = tempfile::tempdir().unwrap();
let temp_path = temp_dir.path();

let (_data, tree) = generate_tree::<Tree, _>(rng, leaves, Some(temp_path.to_path_buf()));
Expand Down