Prevent feature unification from disabling ring proof blinding

.github/workflows/ci.yml

@@ -7,6 +7,11 @@ on:
 
 env:
   CARGO_TERM_COLOR: always
+  # `--all-features` is rejected at compile time: `std` (production prover)
+  # and `insecure-deterministic-no-std-prover` (deterministic no_std prover) are
+  # mutually exclusive. Every prover job picks one configuration explicitly.
+  PRODUCTION_FEATURES: mock,builder-params
+  DETERMINISTIC_FEATURES: insecure-deterministic-no-std-prover,builder-params,mock
 
 jobs:
   fmt:
@@ -30,7 +35,8 @@ jobs:
             ~/.cargo/git
             target
           key: clippy-${{ hashFiles('Cargo.lock') }}
-      - run: cargo clippy --all-features --tests -- -D warnings
+      - run: cargo clippy --features ${{ env.PRODUCTION_FEATURES }} --tests -- -D warnings
+      - run: cargo clippy --no-default-features --features ${{ env.DETERMINISTIC_FEATURES }} --tests -- -D warnings
 
   build:
     name: Build
@@ -44,7 +50,9 @@ jobs:
             ~/.cargo/git
             target
           key: build-${{ hashFiles('Cargo.lock') }}
-      - run: cargo build --all-features
+      - run: cargo build --features ${{ env.PRODUCTION_FEATURES }}
+      # `std` + `insecure-deterministic-no-std-prover` must be rejected at compile time.
+      - run: "! cargo build --all-features"
 
   no-std:
     name: Build no-std (wasm32)
@@ -60,6 +68,7 @@ jobs:
             target
           key: wasm-${{ hashFiles('Cargo.lock') }}
       - run: cargo build --target wasm32-unknown-unknown --no-default-features
+      - run: cargo build --target wasm32-unknown-unknown --no-default-features --features ${{ env.DETERMINISTIC_FEATURES }}
 
   test:
     name: Tests
@@ -73,5 +82,5 @@ jobs:
             ~/.cargo/git
             target
           key: test-${{ hashFiles('Cargo.lock') }}
-      - run: cargo test --release --all-features
-
+      - run: cargo test --release --features ${{ env.PRODUCTION_FEATURES }}
+      - run: cargo test --release --no-default-features --features ${{ env.DETERMINISTIC_FEATURES }}

CHANGELOG.md

@@ -52,10 +52,31 @@ All changes are relative to 0.2.0, the last published version.
   a reusable `MockMembers` newtype in the `mock` module.
 - **`secret-split` feature**: side-channel resistant secret scalar multiplication,
   bundled into `std` (the only place a production prover runs).
-- **`insecure-deterministic-prover` feature**: deterministic, non-zero-knowledge prover
+- **`insecure-deterministic-no-std-prover` feature**: deterministic, non-zero-knowledge prover
   for `no_std` test environments. Enabling `prover` on `no_std` without it is now a
   compile-time error, since the ring prover has no system RNG there and would panic.
 
+### Security
+- **Feature unification can no longer disable the ring proof's zero-knowledge blinding**
+  (SRLabs audit finding; [ring-proof#93](https://github.com/paritytech/ring-proof/pull/93),
+  [ark-vrf#97](https://github.com/davxy/ark-vrf/pull/97))
+  - `insecure-deterministic-no-std-prover` no longer enables `ark-vrf/test-vectors` (removed
+    upstream): the deterministic prover is selected by building this crate's prover
+    ring context via the new runtime `RingContext::new_without_blinding`, so other
+    `ark-vrf` users in the same build are unaffected
+  - Combining `insecure-deterministic-no-std-prover` with `std` (the production proving
+    configuration) is now a compile-time error, mirroring the existing no_std guard
+  - CI builds and tests both supported prover configurations explicitly instead of
+    `--all-features`; a regression test pins blinding on (production) and off
+    (deterministic prover)
+
+### Changed
+- **arkworks dependencies bumped to 0.6** (required by the upstream changes above).
+  Note: since arkworks 0.6 the identity is represented as `(0, 0)` for short
+  Weierstrass curves, so all-zero uncompressed G1 encodings (e.g. in
+  `MembersCommitment`) decode as valid identity points instead of being rejected;
+  such commitments still fail proof verification cleanly
+
 ### Fixed
 - **Validate curve points on decode to prevent panics** ([#44](https://github.com/paritytech/verifiable/pull/44))
   - ark-serialize validation is enabled when decoding types that may come from untrusted sources

Cargo.toml

@@ -13,9 +13,9 @@ exclude = ["src/ring/data/bls12-381/zcash-srs-2-16-uncompressed.bin"]
 bounded-collections = { version = "0.3.2", default-features = false, features = ["scale-codec"] }
 parity-scale-codec = { version = "3.7.4", default-features = false, features = ["derive","max-encoded-len"] }
 scale-info = { version = "2.11", default-features = false, features = ["derive"] }
-ark-serialize = { version = "0.5", default-features = false, features = ["derive"] }
-ark-scale = { version = "0.0.13", default-features = false }
-ark-vrf = { version = "0.5.1", default-features = false, features = ["bandersnatch", "ring"] }
+ark-serialize = { version = "0.6", default-features = false, features = ["derive"] }
+ark-scale = { version = "0.0.14", default-features = false }
+ark-vrf = { version = "0.5.2", default-features = false, features = ["bandersnatch", "ring"] }
 spin = { version = "0.9", default-features = false, features = ["once"] }
 smallvec = { version = "1", default-features = false }
 sha2 = { version = "0.10", default-features = false, optional = true }
@@ -57,14 +57,14 @@ prover = []
 # test prover does not need it.
 secret-split = ["ark-vrf/secret-split"]
 # Deterministic, NON-ZERO-KNOWLEDGE prover for `no_std`/test environments.
-# DO NOT USE IN PRODUCTION: this enables `ark-vrf/test-vectors`, which zeroes the
-# ring proof's blinding rows instead of sampling randomness. The resulting proofs
-# verify normally but are trivially deanonymizable by a passive observer (the ring
-# member index is recoverable from public data). Exists only so the prover can run
-# in `no_std` without a system RNG, for testing.
-insecure-deterministic-prover = [
+# DO NOT USE IN PRODUCTION: the prover's ring context is built without column
+# blinding, so proofs verify normally but are trivially deanonymizable by a
+# passive observer (the ring member index is recoverable from public data).
+# Exists only so the prover can run in `no_std` without a system RNG, for
+# testing. Rejected at compile time when combined with `std`. Only affects
+# provers built by this crate; other `ark-vrf` users in the build are untouched.
+insecure-deterministic-no-std-prover = [
     "prover",
-    "ark-vrf/test-vectors",
 ]
 # Exposes the `mock` module with a non-cryptographic `Mock` implementation
 # of `GenerateVerifiable`. Intended as a test double for downstream crates

README.md

@@ -49,7 +49,7 @@ supporting up to 16127 members.
 | `prover` | Proof generation (`open`, `create`, `create_multi_context`) |
 | `secret-split` | Side-channel-resistant secret scalar multiplication (masks the secret before EC multiplication). Requires a system RNG, so it is only enabled under `std` |
 | `builder-params` | Includes precomputed ring builder params for building ring commitments |
-| `insecure-deterministic-prover` | **Insecure, testing only.** Deterministic `no_std` prover whose proofs are trivially deanonymizable (non-zero-knowledge). Never enable for production |
+| `insecure-deterministic-no-std-prover` | **Insecure, testing only.** Deterministic `no_std` prover whose proofs are trivially deanonymizable (non-zero-knowledge). Rejected at compile time when combined with `std` |
 | `mock` | Exposes the `mock` module with a non-cryptographic `Mock` implementation for tests |
 
 For verifier-only builds (e.g. on-chain), disable default features.

src/lib.rs

@@ -1,22 +1,36 @@
-#![cfg_attr(not(feature = "std"), no_std)]
+// Unit tests use `std` unconditionally, so keep it linked for test builds of the
+// no_std configurations (e.g. `insecure-deterministic-no-std-prover` without `std`).
+#![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
 
 // A production prover needs a system RNG for the ring proof's zero-knowledge
 // blinding (sourced via `getrandom_or_panic` deep in the proving stack). That is
 // only wired up when `std` is enabled; the only no_std prover is the deterministic,
-// non-zero-knowledge one selected by `insecure-deterministic-prover` (test use
+// non-zero-knowledge one selected by `insecure-deterministic-no-std-prover` (test use
 // only). Any other no_std build with `prover` compiles but panics at proof
 // generation, so reject it here instead.
 #[cfg(all(
 	feature = "prover",
 	not(feature = "std"),
-	not(feature = "insecure-deterministic-prover")
+	not(feature = "insecure-deterministic-no-std-prover")
 ))]
 compile_error!(
-	"`prover` on a no_std target requires the `insecure-deterministic-prover` feature \
+	"`prover` on a no_std target requires the `insecure-deterministic-no-std-prover` feature \
 	 (deterministic, NON-ZERO-KNOWLEDGE, testing only). Without it the ring prover has no \
 	 system RNG and panics during proof generation. Use `std` for production proving."
 );
 
+// Cargo features are additive across the build graph: any crate enabling
+// `insecure-deterministic-no-std-prover` would silently switch every other user of this
+// crate in the same build to the non-zero-knowledge prover. Its proofs verify
+// normally but leak the ring member index, so a `std` build (the production
+// proving configuration) must never carry it.
+#[cfg(all(feature = "std", feature = "insecure-deterministic-no-std-prover"))]
+compile_error!(
+	"`insecure-deterministic-no-std-prover` is a no_std testing prover and must not be combined \
+	 with `std`: it disables the ring proof's zero-knowledge blinding, producing proofs \
+	 that verify normally but are trivially deanonymizable."
+);
+
 extern crate alloc;
 
 use alloc::vec::Vec;

src/ring/bandersnatch.rs

@@ -303,6 +303,35 @@ mod tests {
 		.unwrap();
 		assert_eq!(proof_multi.len(), ring_signature_size::<S>(3));
 	}
+
+	// The ring proof's zero-knowledge blinding must be active in the production
+	// configuration: repeating a proof over identical inputs must give different
+	// bytes (fresh randomness each time). A deterministic proof is a function of
+	// the witness, so the ring member index becomes recoverable from public data
+	// while the proof still verifies (SRLabs finding #710). With
+	// `insecure-deterministic-no-std-prover` blinding is intentionally off and the same
+	// inputs must reproduce the exact same proof. The compile-time feature guards
+	// cannot see an upstream regression (e.g. ring-proof disabling blinding
+	// again); this test does.
+	#[test]
+	fn ring_proof_blinding_regression() {
+		let domain_size = RingDomainSize::Domain11;
+		let secret = BandersnatchVrfVerifiable::new_secret([42; 32]);
+		let member = BandersnatchVrfVerifiable::member_from_secret(&secret);
+		let prove = || {
+			let commitment =
+				BandersnatchVrfVerifiable::open(domain_size, &member, [member].into_iter())
+					.unwrap();
+			BandersnatchVrfVerifiable::create(commitment, &secret, b"ctx", b"msg")
+				.unwrap()
+				.0
+		};
+		let (first, second) = (prove(), prove());
+		#[cfg(not(feature = "insecure-deterministic-no-std-prover"))]
+		assert_ne!(first, second);
+		#[cfg(feature = "insecure-deterministic-no-std-prover")]
+		assert_eq!(first, second);
+	}
 }
 
 #[cfg(test)]
@@ -1130,12 +1159,40 @@ mod builder_tests {
 		}
 	});
 
-	// Regression: decoding a bogus (all-zero) members commitment must fail rather than
-	// producing an invalid object that panics during verification.
+	// Regression: decoding a bogus members commitment must fail rather than
+	// producing an invalid object that panics during verification. Since
+	// arkworks 0.6 the all-zero encoding no longer works as the bogus witness:
+	// the identity is represented as (0, 0), so all-zero bytes decode to a
+	// commitment of identity points, a valid group element. Use a not-on-curve
+	// point instead, and check that the identity commitment, while decodable,
+	// fails verification cleanly.
 	#[test]
 	fn decode_bogus_commitment_fails() {
+		// x = 1, y = 0: not on the curve (y^2 = 0 != x^3 + 4).
+		let mut bad_bytes = vec![0u8; BandersnatchSha512Ell2::MEMBERS_COMMITMENT_SIZE];
+		bad_bytes[0] = 1;
+		assert!(MembersCommitment::decode(&mut &bad_bytes[..]).is_err());
+
 		let zero_bytes = vec![0u8; BandersnatchSha512Ell2::MEMBERS_COMMITMENT_SIZE];
-		assert!(MembersCommitment::decode(&mut &zero_bytes[..]).is_err());
+		let identity_members = MembersCommitment::decode(&mut &zero_bytes[..]).unwrap();
+
+		let domain_size = RingDomainSize::Domain11;
+		let secret = BandersnatchVrfVerifiable::new_secret([0; 32]);
+		let member = BandersnatchVrfVerifiable::member_from_secret(&secret);
+		let commitment =
+			BandersnatchVrfVerifiable::open(domain_size, &member, [member].into_iter()).unwrap();
+		let (proof, _) =
+			BandersnatchVrfVerifiable::create(commitment, &secret, b"ctx", b"msg").unwrap();
+		assert_eq!(
+			BandersnatchVrfVerifiable::validate(
+				domain_size,
+				&proof,
+				&identity_members,
+				b"ctx",
+				b"msg"
+			),
+			Err(crate::Error::VerificationFailed),
+		);
 	}
 
 	// The `DecodeUnchecked::decode_unchecked` entry point shares the wire format
@@ -1163,13 +1220,15 @@ mod builder_tests {
 	fn decode_unchecked_skips_validation() {
 		use crate::ring::DecodeUnchecked;
 
-		let zero_bytes = vec![0u8; BandersnatchSha512Ell2::MEMBERS_COMMITMENT_SIZE];
+		// x = 1, y = 0: not on the curve (y^2 = 0 != x^3 + 4).
+		let mut bad_bytes = vec![0u8; BandersnatchSha512Ell2::MEMBERS_COMMITMENT_SIZE];
+		bad_bytes[0] = 1;
 
 		// Validated path still rejects (sanity).
-		assert!(MembersCommitment::decode(&mut &zero_bytes[..]).is_err());
+		assert!(MembersCommitment::decode(&mut &bad_bytes[..]).is_err());
 
 		// Unchecked path accepts the same bytes.
-		assert!(MembersCommitment::decode_unchecked(&mut &zero_bytes[..]).is_ok());
+		assert!(MembersCommitment::decode_unchecked(&mut &bad_bytes[..]).is_ok());
 	}
 
 	// A proof with trailing garbage bytes must not be accepted, otherwise the same
@@ -1331,11 +1390,12 @@ mod builder_tests {
 
 		// A ring proof that verifies under the attacker setup (the forgery vehicle),
 		// assembled exactly as `create_multi_context` would, but with the attacker's
-		// prover key.
+		// prover key. Built without blinding: it needs system randomness, unavailable
+		// in the no_std deterministic configuration, and is irrelevant to the
+		// property under test.
 		let prover_key = attacker_setup.prover_key(&pks).unwrap();
-		let ring_prover = attacker_setup
-			.ring_context()
-			.ring_prover(prover_key, prover_idx);
+		let ring_prover = ark_vrf::ring::RingContext::<S>::new_without_blinding(ring_size)
+			.into_ring_prover(prover_key, prover_idx);
 		let input_msg = [<S as RingSuiteExt>::VRF_INPUT_DOMAIN, context].concat();
 		let input = ark_vrf::Input::<S>::new(&input_msg).unwrap();
 		let output = secrets[prover_idx].output(input);

src/ring/mod.rs

@@ -139,7 +139,16 @@ pub fn make_ring_setup<S: RingSuiteExt>(
 	let pcs_params =
 		ark_vrf::ring::PcsParams::<S>::deserialize_uncompressed_unchecked(data).unwrap();
 	let ring_size = domain_size.max_ring_size::<S>();
-	ark_vrf::ring::RingSetup::<S>::from_pcs_params(ring_size, pcs_params).unwrap()
+	let setup = ark_vrf::ring::RingSetup::<S>::from_pcs_params(ring_size, pcs_params).unwrap();
+	// Column blinding needs a system RNG, unavailable in no_std. Provers built
+	// from this context produce deterministic, NON-ZERO-KNOWLEDGE proofs, still
+	// valid for verifiers using the regular blinding-enabled context.
+	#[cfg(feature = "insecure-deterministic-no-std-prover")]
+	let setup = ark_vrf::ring::RingSetup {
+		ring_ctx: RingContext::new_without_blinding(ring_size),
+		..setup
+	};
+	setup
 }
 
 /// Get ring builder params for the given domain size.