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.

Sign up for GitHub

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

Jump to bottom

Remove non-constant-time comparisons of secret values. #734

Merged
merged 7 commits into from
Sep 8, 2023
Merged

Remove non-constant-time comparisons of secret values. #734

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fe2e421
Remove non-constant-time comparisons of secret values.
branlwyd Sep 6, 2023
0b624de
Code review.
branlwyd Sep 7, 2023
1d54c6d
Tests.
branlwyd Sep 7, 2023
676609b
Code review.
branlwyd Sep 8, 2023
7618ecf
One last review comment (oops).
branlwyd Sep 8, 2023
00fef14
Last, last review comment.
branlwyd Sep 8, 2023
c1d5494
cargo fmt
branlwyd Sep 8, 2023
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
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ itertools = "0.11.0"
modinverse = "0.1.0"
num-bigint = "0.4.4"
once_cell = "1.18.0"
prio = { path = ".", features = ["crypto-dependencies"] }
prio = { path = ".", features = ["crypto-dependencies", "test-util"] }
rand = "0.8"
serde_json = "1.0"
statrs = "0.16.0"
Expand All @@ -58,6 +58,7 @@ experimental = ["bitvec", "fiat-crypto", "fixed", "num-bigint", "num-rational",
multithreaded = ["rayon"]
prio2 = ["crypto-dependencies", "hmac", "sha2"]
crypto-dependencies = ["aes", "ctr"]
test-util = []

[workspace]
members = [".", "binaries"]
Expand Down
24 changes: 21 additions & 3 deletions src/vdaf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use crate::{
};
use serde::{Deserialize, Serialize};
use std::{fmt::Debug, io::Cursor};
use subtle::{Choice, ConstantTimeEq};

/// A component of the domain-separation tag, used to bind the VDAF operations to the document
/// version. This will be revised with each draft with breaking changes.
Expand Down Expand Up @@ -57,7 +58,9 @@ pub enum VdafError {
}

/// An additive share of a vector of field elements.
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that this API might be too permissive. The intent is to fence the unsafe comparison to testing, but IIUC, the user can still shoot themself in the foot if they're not careful. Let's say I enable "test-util" in my crate that I also deploy to production; then I get to use Eq in my production code, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

One way we could resolve this is to write ConstantTimeEq implementations for the relevant structs/enums, and write PartialEq implementations that dispatch to them. (for an example of this pattern, see IdpfPublicShare) This would provide safe APIs for both downstream dependents and for test code, avoid the need for the test-util Cargo feature, and still give us the convenience benefit of being able to use assert_eq!().

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds reasonable to me.

Copy link
Member Author

@branlwyd branlwyd Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The concern that led to this implementation was (from #722):

But the risk there is that new VDAFs will use derived PartialEq implementations that aren't constant time. So perhaps we would want to remove all PartialEq + Eq trait bounds and instead define some custom ConstantTimeEq trait.

That is, if we expose PartialEq + Eq by default (and expect users of the library to use these traits for comparison), we risk future VDAFs implementing variable-time versions of these traits. This would lead to any DAP implementations built on these VDAFs silently becoming insecure, in a way that would be particularly hard to notice.

OTOH, I like the idea of safe-by-default implementations and especially not requiring callers to know about subtle at all to use the library. I have moved everything relevant over to a constant-time implmementation of PartialEq + Eq.

This required implementing a few more constant-time equality checks than I otherwise would have had to, since a few types had test-only equality derivations before.

Let's say I enable "test-util" in my crate that I also deploy to production; then I get to use Eq in my production code, right?

Correct; if you enable a feature called "test-util" in your production binaries, IMO you are responsible for what comes next. :) This is a somewhat-common pattern for libraries -- for example, subtle's documentation contains the following: "Note: the subtle crate contains debug_asserts to check invariants during debug builds. These invariant checks involve secret-dependent branches, and are not present when compiled in release mode. This crate is intended to be used in release mode."

pub enum Share<F, const SEED_SIZE: usize> {
/// An uncompressed share, typically sent to the leader.
Leader(Vec<F>),
Expand All @@ -78,6 +81,18 @@ impl<F: Clone, const SEED_SIZE: usize> Share<F, SEED_SIZE> {
}
}

impl<F: ConstantTimeEq, const SEED_SIZE: usize> ConstantTimeEq for Share<F, SEED_SIZE> {
fn ct_eq(&self, other: &Self) -> subtle::Choice {
// We allow short-circuiting on the type (Leader vs Helper) of the value, but not the types'
// contents.
match (self, other) {
(Share::Leader(self_val), Share::Leader(other_val)) => self_val.ct_eq(other_val),
(Share::Helper(self_val), Share::Helper(other_val)) => self_val.ct_eq(other_val),
_ => Choice::from(0),
}
}
}

/// Parameters needed to decode a [`Share`]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ShareDecodingParameter<const SEED_SIZE: usize> {
Expand Down Expand Up @@ -310,7 +325,7 @@ pub trait Aggregatable: Clone + Debug + From<Self::OutputShare> {
}

/// An output share comprised of a vector of field elements.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
pub struct OutputShare<F>(Vec<F>);

impl<F> AsRef<[F]> for OutputShare<F> {
Expand Down Expand Up @@ -339,7 +354,10 @@ impl<F: FieldElement> Encode for OutputShare<F> {
///
/// This is suitable for VDAFs where both output shares and aggregate shares are vectors of field
/// elements, and output shares need no special transformation to be merged into an aggregate share.
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Clone, Debug, Serialize, Deserialize)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]

pub struct AggregateShare<F>(Vec<F>);

impl<F: FieldElement> AsRef<[F]> for AggregateShare<F> {
Expand Down
38 changes: 33 additions & 5 deletions src/vdaf/poplar1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ impl<P, const SEED_SIZE: usize> ParameterizedDecode<Poplar1<P, SEED_SIZE>> for P
///
/// This is comprised of an IDPF key share and the correlated randomness used to compute the sketch
/// during preparation.
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Poplar1InputShare<const SEED_SIZE: usize> {
/// IDPF key share.
idpf_key: Seed<16>,
Expand All @@ -128,6 +130,24 @@ pub struct Poplar1InputShare<const SEED_SIZE: usize> {
corr_leaf: [Field255; 2],
}

impl<const SEED_SIZE: usize> ConstantTimeEq for Poplar1InputShare<SEED_SIZE> {
fn ct_eq(&self, other: &Self) -> Choice {
// We short-circuit on the length of corr_inner being different. Only the content is
// protected.
if self.corr_inner.len() != other.corr_inner.len() {
return Choice::from(0);
}

let mut rslt = self.idpf_key.ct_eq(&other.idpf_key)
branlwyd marked this conversation as resolved.
Show resolved Hide resolved
& self.corr_seed.ct_eq(&other.corr_seed)
& self.corr_leaf.ct_eq(&other.corr_leaf);
for (x, y) in self.corr_inner.iter().zip(other.corr_inner.iter()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can avoid manually checking lengths and iterating by using the blanket impl<T: ConstantTimeEq> ConstantTimeEq for [T]. Just as the code here does, it short-circuits on slice length. Then, this method body could be simplified to a single four-way & expression.

Copy link
Member Author

@branlwyd branlwyd Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried this, unfortunately it doesn't work with an error like:

error[E0599]: the method `ct_eq` exists for struct `Vec<[Field64; 2]>`, but its trait bounds were not satisfied
   --> src/vdaf/poplar1.rs:159:31
    |
159 |             & self.corr_inner.ct_eq(&other.corr_inner)
    |                               ^^^^^ method cannot be called on `Vec<[Field64; 2]>` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `[Field64; 2]: ConstantTimeEq`
            which is required by `[[Field64; 2]]: ConstantTimeEq`

I think the reason [Field64; 2] doesn't satisfy ConstantTimeEq is that it can't be automatically converted to a [Field64]. subtle's const-generics feature seems like it should help -- but it actually only provides a blanket implementation of ConditionallySelectable for [T; N], no ConstantTimeEq implementation. I don't see why one couldn't be written, but this would require waiting on a release of the subtle crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I filed dalek-cryptography/subtle#114, we'll see if it lands. The last release was 6 months ago so we may be waiting awhile in any case.

rslt &= x.ct_eq(y);
}
rslt
}
}

impl<const SEED_SIZE: usize> Encode for Poplar1InputShare<SEED_SIZE> {
fn encode(&self, bytes: &mut Vec<u8>) {
self.idpf_key.encode(bytes);
Expand Down Expand Up @@ -174,7 +194,9 @@ impl<'a, P, const SEED_SIZE: usize> ParameterizedDecode<(&'a Poplar1<P, SEED_SIZ
}

/// Poplar1 preparation state.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Poplar1PrepareState(PrepareStateVariant);

impl Encode for Poplar1PrepareState {
Expand All @@ -201,7 +223,9 @@ impl<'a, P, const SEED_SIZE: usize> ParameterizedDecode<(&'a Poplar1<P, SEED_SIZ
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
enum PrepareStateVariant {
Inner(PrepareState<Field64>),
Leaf(PrepareState<Field255>),
Expand Down Expand Up @@ -252,7 +276,9 @@ impl<'a, P, const SEED_SIZE: usize> ParameterizedDecode<(&'a Poplar1<P, SEED_SIZ
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
struct PrepareState<F> {
sketch: SketchState<F>,
output_share: Vec<F>,
Expand Down Expand Up @@ -450,7 +476,9 @@ impl ParameterizedDecode<Poplar1PrepareState> for Poplar1PrepareMessage {
}

/// A vector of field elements transmitted while evaluating Poplar1.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub enum Poplar1FieldVec {
/// Field type for inner nodes of the IDPF tree.
Inner(Vec<Field64>),
Expand Down
4 changes: 3 additions & 1 deletion src/vdaf/prio2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ impl Client<16> for Prio2 {
}

/// State of each [`Aggregator`] during the Preparation phase.
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Prio2PrepareState(Share<FieldPrio2, 32>);

impl Encode for Prio2PrepareState {
Expand Down
61 changes: 55 additions & 6 deletions src/vdaf/prio3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ use std::fmt::Debug;
use std::io::Cursor;
use std::iter::{self, IntoIterator};
use std::marker::PhantomData;
use subtle::{Choice, ConstantTimeEq};

const DST_MEASUREMENT_SHARE: u16 = 1;
const DST_PROOF_SHARE: u16 = 2;
Expand Down Expand Up @@ -595,7 +596,7 @@ where
}

/// Message broadcast by the [`Client`] to every [`Aggregator`] during the Sharding phase.
#[derive(Clone, Debug, PartialEq, Eq)]
#[derive(Clone, Debug)]
pub struct Prio3PublicShare<const SEED_SIZE: usize> {
/// Contributions to the joint randomness from every aggregator's share.
joint_rand_parts: Option<Vec<Seed<SEED_SIZE>>>,
Expand All @@ -620,6 +621,22 @@ impl<const SEED_SIZE: usize> Encode for Prio3PublicShare<SEED_SIZE> {
}
}

impl<const SEED_SIZE: usize> PartialEq for Prio3PublicShare<SEED_SIZE> {
fn eq(&self, other: &Self) -> bool {
branlwyd marked this conversation as resolved.
Show resolved Hide resolved
// Handle case that both join_rand_parts are populated.
if let Some(self_joint_rand_parts) = &self.joint_rand_parts {
branlwyd marked this conversation as resolved.
Show resolved Hide resolved
if let Some(other_joint_rand_parts) = &other.joint_rand_parts {
return self_joint_rand_parts.ct_eq(other_joint_rand_parts).into();
}
}

// Handle case that at least one joint_rand_parts is not populated.
self.joint_rand_parts.is_none() && other.joint_rand_parts.is_none()
}
}

impl<const SEED_SIZE: usize> Eq for Prio3PublicShare<SEED_SIZE> {}

impl<T, P, const SEED_SIZE: usize> ParameterizedDecode<Prio3<T, P, SEED_SIZE>>
for Prio3PublicShare<SEED_SIZE>
where
Expand All @@ -646,7 +663,9 @@ where
}

/// Message sent by the [`Client`] to each [`Aggregator`] during the Sharding phase.
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Prio3InputShare<F, const SEED_SIZE: usize> {
/// The measurement share.
measurement_share: Share<F, SEED_SIZE>,
Expand All @@ -659,6 +678,24 @@ pub struct Prio3InputShare<F, const SEED_SIZE: usize> {
joint_rand_blind: Option<Seed<SEED_SIZE>>,
}

impl<F: ConstantTimeEq, const SEED_SIZE: usize> ConstantTimeEq for Prio3InputShare<F, SEED_SIZE> {
fn ct_eq(&self, other: &Self) -> Choice {
// We allow short-circuiting on the existence (but not contents) of the joint_rand_blind,
// as its existence is a property of the type in use.
let joint_rand_eq = match (&self.joint_rand_blind, &other.joint_rand_blind) {
(Some(self_joint_rand), Some(other_joint_rand)) => {
self_joint_rand.ct_eq(other_joint_rand)
}
(None, None) => Choice::from(1),
_ => Choice::from(0),
};

joint_rand_eq
& self.measurement_share.ct_eq(&other.measurement_share)
& self.proof_share.ct_eq(&other.proof_share)
}
}

impl<F: FftFriendlyFieldElement, const SEED_SIZE: usize> Encode for Prio3InputShare<F, SEED_SIZE> {
fn encode(&self, bytes: &mut Vec<u8>) {
if matches!(
Expand Down Expand Up @@ -726,7 +763,9 @@ where
}
}

#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
/// Message broadcast by each [`Aggregator`] in each round of the Preparation phase.
pub struct Prio3PrepareShare<F, const SEED_SIZE: usize> {
/// A share of the FLP verifier message. (See [`Type`].)
Expand Down Expand Up @@ -783,7 +822,9 @@ impl<F: FftFriendlyFieldElement, const SEED_SIZE: usize>
}
}

#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
/// Result of combining a round of [`Prio3PrepareShare`] messages.
pub struct Prio3PrepareMessage<const SEED_SIZE: usize> {
/// The joint randomness seed computed by the Aggregators.
Expand Down Expand Up @@ -841,7 +882,9 @@ where
}

/// State of each [`Aggregator`] during the Preparation phase.
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Prio3PrepareState<F, const SEED_SIZE: usize> {
measurement_share: Share<F, SEED_SIZE>,
joint_rand_seed: Option<Seed<SEED_SIZE>>,
Expand Down Expand Up @@ -1111,7 +1154,13 @@ where
) -> Result<PrepareTransition<Self, SEED_SIZE, 16>, VdafError> {
if self.typ.joint_rand_len() > 0 {
// Check that the joint randomness was correct.
if step.joint_rand_seed.as_ref().unwrap() != msg.joint_rand_seed.as_ref().unwrap() {
if (!step
.joint_rand_seed
.as_ref()
.unwrap()
.ct_eq(msg.joint_rand_seed.as_ref().unwrap()))
branlwyd marked this conversation as resolved.
Show resolved Hide resolved
.into()
{
return Err(VdafError::Uncategorized(
"joint randomness mismatch".to_string(),
));
Expand Down
10 changes: 3 additions & 7 deletions src/vdaf/xof.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ use std::{
use subtle::{Choice, ConstantTimeEq};

/// Input of [`Xof`].
#[derive(Clone, Debug, Eq)]
#[derive(Clone, Debug)]
// Only derive equality checks in test code, as the content of this type is sometimes a secret.
#[cfg_attr(feature = "test-util", derive(PartialEq, Eq))]
pub struct Seed<const SEED_SIZE: usize>(pub(crate) [u8; SEED_SIZE]);

impl<const SEED_SIZE: usize> Seed<SEED_SIZE> {
Expand Down Expand Up @@ -67,12 +69,6 @@ impl<const SEED_SIZE: usize> ConstantTimeEq for Seed<SEED_SIZE> {
}
}

impl<const SEED_SIZE: usize> PartialEq for Seed<SEED_SIZE> {
fn eq(&self, other: &Self) -> bool {
self.ct_eq(other).into()
}
}

impl<const SEED_SIZE: usize> Encode for Seed<SEED_SIZE> {
fn encode(&self, bytes: &mut Vec<u8>) {
bytes.extend_from_slice(&self.0[..]);
Expand Down