-
Notifications
You must be signed in to change notification settings - Fork 19
Fix Citadel security audit findings #137
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ use sha2::{Digest, Sha256}; | |
| use std::env; | ||
| use std::fs::{self, File}; | ||
| use std::io::prelude::*; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::path::PathBuf; | ||
| use zk_citadel::circuit; | ||
|
|
||
| static LABEL: &[u8; 12] = b"dusk-network"; | ||
|
|
@@ -22,90 +22,115 @@ const CRS_17_HASH: &str = "6161605616b62356cf09fa28252c672ef53b2c8489ad5f81d87af | |
| const PROVER_PATH: &str = "../target/prover"; | ||
| const VERIFIER_PATH: &str = "../target/verifier"; | ||
| const CIRCUIT_MARKER_PATH: &str = "../target/license-circuit-build-id"; | ||
| const CIRCUIT_BUILD_ID: &str = "zk-citadel-license-circuit"; | ||
| const OUT_DIR_VERIFIER_FILENAME: &str = "license_verifier"; | ||
| const CIRCUIT_BUILD_DOMAIN: &[u8] = b"zk-citadel-license-circuit-v2"; | ||
| const CIRCUIT_SOURCE_PATHS: &[(&str, &str)] = &[ | ||
| ("core/Cargo.toml", "../core/Cargo.toml"), | ||
| ("core/src/helpers.rs", "../core/src/helpers.rs"), | ||
| ("core/src/signatures.rs", "../core/src/signatures.rs"), | ||
| ("core/src/zk/circuit.rs", "../core/src/zk/circuit.rs"), | ||
| ("core/src/zk/gadgets.rs", "../core/src/zk/gadgets.rs"), | ||
| ]; | ||
|
|
||
| #[tokio::main] | ||
| async fn main() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the advantages of all these changes to the contract built?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prior setup files could be reused after circuit source changes because the marker was static. These changes make Cargo track the circuit sources, regenerating the prover/verifier together, embedding the verifier produced by the build and expose the fingerprints so wallets can reject mismatched artifacts. It gives stronger guarantees |
||
| if setup_material_stale() { | ||
| let response = reqwest::get(CRS_URL).await; | ||
|
|
||
| match response { | ||
| Ok(pp_bytes) => { | ||
| // If setup didn't exist locally, we download the setup again from server | ||
| let pp_bytes = pp_bytes.bytes().await.unwrap(); | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(pp_bytes.clone()); | ||
| let hash = format!("{:x}", hasher.finalize()); | ||
|
|
||
| // We check the file integrity | ||
| assert_eq!(hash, CRS_17_HASH); | ||
|
|
||
| let pp = PublicParameters::from_slice(pp_bytes.to_vec().as_slice()) | ||
| .expect("Creating PublicParameters from slice failed."); | ||
|
|
||
| // Compile the license circuit | ||
| let (prover, verifier) = Compiler::compile::<circuit::LicenseCircuit>(&pp, LABEL) | ||
| .expect("failed to compile circuit"); | ||
|
|
||
| // Write prover key to disk | ||
| let mut file = File::create(PROVER_PATH).unwrap(); | ||
| file.write_all(&prover.to_bytes()).unwrap(); | ||
|
|
||
| // Write verifier key to disk | ||
| let mut file = File::create(VERIFIER_PATH).unwrap(); | ||
| file.write_all(&verifier.to_bytes()).unwrap(); | ||
| write_circuit_marker(); | ||
|
|
||
| info!("Local trusted setup not found, a new one was downloaded."); | ||
| } | ||
| Err(_e) => { | ||
| // If download fails, we create a setup from scratch | ||
| let pp = PublicParameters::setup(1 << circuit::CAPACITY, &mut OsRng).unwrap(); | ||
|
|
||
| // Compile the license circuit | ||
| let (prover, verifier) = Compiler::compile::<circuit::LicenseCircuit>(&pp, LABEL) | ||
| .expect("failed to compile circuit"); | ||
|
|
||
| // Write prover key to disk | ||
| let mut file = File::create(PROVER_PATH).unwrap(); | ||
| file.write_all(&prover.to_bytes()).unwrap(); | ||
|
|
||
| // Write verifier key to disk | ||
| let mut file = File::create(VERIFIER_PATH).unwrap(); | ||
| file.write_all(&verifier.to_bytes()).unwrap(); | ||
| write_circuit_marker(); | ||
|
|
||
| warn!( | ||
| "Download of trusted setup from server failed. A new one was generated from scratch. USE AT YOUR OWN RISK." | ||
| ); | ||
| } | ||
| } | ||
| emit_rerun_directives(); | ||
|
|
||
| let circuit_build_id = circuit_build_id(); | ||
| let response = reqwest::get(CRS_URL).await; | ||
| let verifier_bytes = match response { | ||
| Ok(pp_bytes) => compile_from_trusted_crs(pp_bytes, &circuit_build_id).await, | ||
| Err(_e) => compile_from_fresh_setup(&circuit_build_id), | ||
| }; | ||
|
|
||
| write_metadata_hashes(&circuit_build_id, &verifier_bytes); | ||
| } | ||
|
|
||
| fn emit_rerun_directives() { | ||
| std::println!("cargo:rerun-if-changed=build.rs"); | ||
| for (_, disk_path) in CIRCUIT_SOURCE_PATHS { | ||
| std::println!("cargo:rerun-if-changed={disk_path}"); | ||
| } | ||
| } | ||
|
|
||
| async fn compile_from_trusted_crs(pp_bytes: reqwest::Response, circuit_build_id: &str) -> Vec<u8> { | ||
| let pp_bytes = pp_bytes.bytes().await.unwrap(); | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(pp_bytes.clone()); | ||
| let hash = format!("{:x}", hasher.finalize()); | ||
|
|
||
| // We check the file integrity | ||
| assert_eq!(hash, CRS_17_HASH); | ||
|
|
||
| write_metadata_hashes(); | ||
| let pp = PublicParameters::from_slice(pp_bytes.to_vec().as_slice()) | ||
| .expect("Creating PublicParameters from slice failed."); | ||
| let verifier_bytes = compile_and_write_setup_material(&pp, circuit_build_id); | ||
|
|
||
| info!("License circuit setup material was regenerated from the trusted CRS."); | ||
| verifier_bytes | ||
| } | ||
|
|
||
| fn setup_material_stale() -> bool { | ||
| if !(Path::new(PROVER_PATH).exists()) || !(Path::new(VERIFIER_PATH).exists()) { | ||
| return true; | ||
| fn compile_from_fresh_setup(circuit_build_id: &str) -> Vec<u8> { | ||
| // If download fails, we create a setup from scratch | ||
| let pp = PublicParameters::setup(1 << circuit::CAPACITY, &mut OsRng).unwrap(); | ||
| let verifier_bytes = compile_and_write_setup_material(&pp, circuit_build_id); | ||
|
|
||
| warn!( | ||
| "Download of trusted setup from server failed. A new one was generated from scratch. USE AT YOUR OWN RISK." | ||
| ); | ||
| verifier_bytes | ||
| } | ||
|
|
||
| fn compile_and_write_setup_material(pp: &PublicParameters, circuit_build_id: &str) -> Vec<u8> { | ||
| let (prover, verifier) = | ||
| Compiler::compile::<circuit::LicenseCircuit>(pp, LABEL).expect("failed to compile circuit"); | ||
| let prover_bytes = prover.to_bytes(); | ||
| let verifier_bytes = verifier.to_bytes(); | ||
|
|
||
| // Keep the legacy target paths for wallets/tests, but embed the verifier | ||
| // from OUT_DIR so deployed bytecode cannot observe a stale target file. | ||
| let mut file = File::create(PROVER_PATH).unwrap(); | ||
| file.write_all(&prover_bytes).unwrap(); | ||
|
|
||
| let mut file = File::create(VERIFIER_PATH).unwrap(); | ||
| file.write_all(&verifier_bytes).unwrap(); | ||
|
|
||
| let out_dir_verifier = out_dir().join(OUT_DIR_VERIFIER_FILENAME); | ||
| fs::write(out_dir_verifier, &verifier_bytes).expect("OUT_DIR verifier should be written"); | ||
|
|
||
| write_circuit_marker(circuit_build_id); | ||
| verifier_bytes | ||
| } | ||
|
|
||
| fn circuit_build_id() -> String { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(CIRCUIT_BUILD_DOMAIN); | ||
|
|
||
| for (canonical_path, disk_path) in CIRCUIT_SOURCE_PATHS { | ||
| let source = fs::read(disk_path) | ||
| .unwrap_or_else(|error| panic!("failed to read {disk_path}: {error}")); | ||
| hasher.update((*canonical_path).as_bytes()); | ||
| hasher.update((source.len() as u64).to_le_bytes()); | ||
| hasher.update(source); | ||
| } | ||
|
|
||
| fs::read_to_string(CIRCUIT_MARKER_PATH) | ||
| .map(|marker| marker.trim() != CIRCUIT_BUILD_ID) | ||
| .unwrap_or(true) | ||
| format!("{:x}", hasher.finalize()) | ||
| } | ||
|
|
||
| fn write_circuit_marker() { | ||
| fs::write(CIRCUIT_MARKER_PATH, CIRCUIT_BUILD_ID).expect("circuit marker should be written"); | ||
| fn write_circuit_marker(circuit_build_id: &str) { | ||
| let prover_hash = file_sha256_hex(PROVER_PATH).expect("prover hash should be computed"); | ||
| let verifier_hash = file_sha256_hex(VERIFIER_PATH).expect("verifier hash should be computed"); | ||
| let marker = format!( | ||
| "circuit_build_id={circuit_build_id}\nprover_hash={prover_hash}\nverifier_hash={verifier_hash}\n" | ||
| ); | ||
| fs::write(CIRCUIT_MARKER_PATH, marker).expect("circuit marker should be written"); | ||
| } | ||
|
|
||
| fn write_metadata_hashes() { | ||
| let verifier = fs::read(VERIFIER_PATH).expect("verifier key should exist after build"); | ||
| let verifier_key_hash = scalar_from_sha256(b"CITADEL_VERIFIER_KEY_HASH_V1", &verifier); | ||
| let circuit_hash = scalar_from_sha256(b"CITADEL_CIRCUIT_HASH_V1", CIRCUIT_BUILD_ID.as_bytes()); | ||
| fn write_metadata_hashes(circuit_build_id: &str, verifier: &[u8]) { | ||
| let verifier_key_hash = scalar_from_sha256(b"CITADEL_VERIFIER_KEY_HASH_V1", verifier); | ||
| let circuit_hash = scalar_from_sha256(b"CITADEL_CIRCUIT_HASH_V1", circuit_build_id.as_bytes()); | ||
|
|
||
| let out_dir = PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR should be set")); | ||
| let generated = out_dir.join("metadata_hashes.rs"); | ||
| let generated = out_dir().join("metadata_hashes.rs"); | ||
| let contents = format!( | ||
| "const VERIFIER_KEY_HASH: BlsScalar = BlsScalar::from_raw({verifier_key_hash:?});\n\ | ||
| const CIRCUIT_HASH: BlsScalar = BlsScalar::from_raw({circuit_hash:?});\n", | ||
|
|
@@ -114,6 +139,10 @@ fn write_metadata_hashes() { | |
| fs::write(generated, contents).expect("metadata hash constants should be written"); | ||
| } | ||
|
|
||
| fn out_dir() -> PathBuf { | ||
| PathBuf::from(env::var("OUT_DIR").expect("OUT_DIR should be set")) | ||
| } | ||
|
|
||
| fn scalar_from_sha256(domain: &[u8], bytes: &[u8]) -> [u64; 4] { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(domain); | ||
|
|
@@ -131,3 +160,13 @@ fn scalar_from_sha256(domain: &[u8], bytes: &[u8]) -> [u64; 4] { | |
|
|
||
| limbs | ||
| } | ||
|
|
||
| fn file_sha256_hex(path: &str) -> std::io::Result<String> { | ||
| fs::read(path).map(|bytes| sha256_hex(&bytes)) | ||
| } | ||
|
|
||
| fn sha256_hex(bytes: &[u8]) -> String { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(bytes); | ||
| format!("{:x}", hasher.finalize()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ pub mod license_contract { | |
| const MERKLE_ARITY: usize = 4; | ||
| const DEPTH: usize = 16; // the depth of LicenseCircuit's Merkle tree | ||
| const ROOT_HISTORY_SIZE: usize = 8; | ||
| const MAX_SESSIONS: usize = 1 << DEPTH; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get this. Why is 2^16 the max number of sessions?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was done as a defensive state bound, but tying it to the Merkle depth was arbitrary and way too constraining. I agree it should be removed |
||
| const CITADEL_CONTEXT_V1_TAG: BlsScalar = BlsScalar::zero(); | ||
| const DEFAULT_DEPLOYMENT_ID: BlsScalar = BlsScalar::zero(); | ||
| const CITADEL_LICENSE_HASH_V1_TAG: u64 = 0x04; | ||
|
|
@@ -148,6 +149,9 @@ pub mod license_contract { | |
| if self.sessions.get(&session_id).is_some() { | ||
| panic!("License already nullified"); | ||
| } | ||
| if self.sessions.len() >= MAX_SESSIONS { | ||
| panic!("Session registry is full"); | ||
| } | ||
|
|
||
| Self::assert_proof( | ||
| verifier_data_license_circuit(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why v2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to version the build ID as is. The fingerprint is opaque and checked by exact equality, and we do not support both the old static marker and this source derived format. I'll replace v2 with a descriptive source fingerprint domain.