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

branlwyd · 2023-09-06T22:09:04Z

This affects the types representing:

Input shares
Preparation states
Output shares
Aggregate shares

(and their constituent types)

Mostly, the comparisons were either dropped entirely or updated to be test-only. Input shares were instead given a constant-time equality implementation, as it is believed this is required for DAP implementations.

PartialEq & Eq implementations are still derived when the "test-util" feature is enabled. This is to ease testing for users of this library.

Closes #722.

branlwyd · 2023-09-06T22:12:39Z

A few comparisons in constant-time equality implementations were left non-constant-time; only the content of secret values was considered as required for constant-time comparisons. Specifically:

The Share type's ConstantTimeEq implementation will short-circuit on the type of the share (Leader vs Helper); this is thought to be OK since the content of the secrets inside is still compared in constant time.
The Prio3InputShare type's ConstantTimeEq implementation will short-circuit on the presence/absence of joint_rand_blind; since this is a property of the type of VDAF in use, it is not considered a secret. The content (if any) of joint_rand_blind is compared in constant time, as well as all other fields.
The Poplar1InputShare type's ConstantTimeEq implementation will short-circuit on the length of corr_inner; since only the content should be a secret, this should be OK. All field contents are otherwise verified in constant time.

Please verify these assertions as part of review. Please also verify that no other types should have their non-constant-time comparisons dropped. I have modified Janus-at-main to use this change successfully, so I am confident that at least Janus can work with these traits.

This affects the types representing: * Input shares * Preparation states * Output shares * Aggregate shares Mostly, the comparisons were either dropped entirely or updated to be test-only. Input shares were instead given a constant-time equality implementation, as it is believed this is required for DAP implementations. PartialEq & Eq implementations are still derived when the "test-util" feature is enabled. This is to ease testing for users of this library.

cjpatton · 2023-09-07T16:09:33Z

@branlwyd that seems like the right definition. Basically what our goal is to make runtime depend as little as possible on secrets. For other things the side channel is not important. We don't much care if the attacker knows that the party doing the computation is the Leader or Helper, for example.

cjpatton

Implementations of ConstantTimeEq look good. I'm a bit concerned about API safety, see inline comment.

src/vdaf/poplar1.rs

cjpatton · 2023-09-07T16:16:16Z

src/vdaf.rs

-#[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))]


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?

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!().

That sounds reasonable to me.

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."

src/vdaf/prio3.rs

divergentdave · 2023-09-07T16:45:22Z

src/vdaf.rs

-#[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))]


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!().

divergentdave · 2023-09-07T16:48:28Z

src/vdaf/poplar1.rs

+        let mut rslt = self.idpf_key.ct_eq(&other.idpf_key)
+            & 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()) {


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.

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.

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.

branlwyd · 2023-09-07T23:19:23Z

I added tests for all of the manually-implemented equality traits. They test PartialEq, but since all of the relevant PartialEq implementations fall through to ConstantTimeEq, this is sufficient to cover both traits.

src/vdaf/prio3.rs

cjpatton

Thanks for putting your back into making this API-safe. It was a lot more work than we initially expected, but I think this is a really great improvement. Two high-level comments:

For consistency, I think we should short-circuit any comparisons that don't need to be constant time. The only things that need to be constant time are secrets and secret shares. (See inline comments for specific suggestions.)
It's important to keep in mind that constant time code is "best effort" and doesn't guarantee that side channels are eliminated. One day some academic lab might take a radio receiver or electron microscope to libprio-rs and find they can pull off an attack if both Aggregators are on old Pentium laptops in the same room or something. The documentation and comments should reflect that we have done our best to mitigate some timing side channels, but others may exist. I don't have a specific suggestion here, so maybe this is just something to keep in mind.

src/vdaf/poplar1.rs

src/vdaf/prio3.rs

branlwyd · 2023-09-08T22:10:46Z

For consistency, I think we should short-circuit any comparisons that don't need to be constant time. The only things that need to be constant time are secrets and secret shares. (See inline comments for specific suggestions.)

Done, but I'm relying on y'all for which fields specifically are safe to compare in variable-time.

It's important to keep in mind that constant time code is "best effort" and doesn't guarantee that side channels are eliminated. One day some academic lab might take a radio receiver or electron microscope to libprio-rs and find they can pull off an attack if both Aggregators are on old Pentium laptops in the same room or something. The documentation and comments should reflect that we have done our best to mitigate some timing side channels, but others may exist. I don't have a specific suggestion here, so maybe this is just something to keep in mind.

Agreed; I think this should go in something like a "security considerations" section, once one is written.

Getting `option_ct_eq` to work in the last place it needed to required a few changes.

branlwyd force-pushed the bran/constant-time branch 3 times, most recently from 55ba66b to d74428f Compare September 7, 2023 00:29

branlwyd force-pushed the bran/constant-time branch from d74428f to fe2e421 Compare September 7, 2023 00:34

branlwyd marked this pull request as ready for review September 7, 2023 00:47

branlwyd requested a review from a team as a code owner September 7, 2023 00:47

branlwyd requested review from divergentdave and cjpatton September 7, 2023 00:54

cjpatton reviewed Sep 7, 2023

View reviewed changes

divergentdave reviewed Sep 7, 2023

View reviewed changes

branlwyd added 2 commits September 7, 2023 14:39

Code review.

0b624de

Tests.

1d54c6d

branlwyd requested review from divergentdave and cjpatton September 7, 2023 23:19

divergentdave approved these changes Sep 8, 2023

View reviewed changes

src/vdaf/prio3.rs Outdated Show resolved Hide resolved

src/vdaf/prio3.rs Outdated Show resolved Hide resolved

cjpatton approved these changes Sep 8, 2023

View reviewed changes

src/vdaf/poplar1.rs Outdated Show resolved Hide resolved

src/vdaf/prio3.rs Show resolved Hide resolved

src/vdaf/prio3.rs Outdated Show resolved Hide resolved

src/vdaf/prio3.rs Outdated Show resolved Hide resolved

src/vdaf/prio3.rs Outdated Show resolved Hide resolved

Code review.

676609b

branlwyd added 3 commits September 8, 2023 15:10

One last review comment (oops).

7618ecf

Last, last review comment.

00fef14

Getting `option_ct_eq` to work in the last place it needed to required a few changes.

cargo fmt

c1d5494

branlwyd merged commit 753a461 into main Sep 8, 2023
6 checks passed

branlwyd deleted the bran/constant-time branch September 8, 2023 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

branlwyd commented Sep 6, 2023 •

edited

branlwyd commented Sep 6, 2023 •

edited

cjpatton commented Sep 7, 2023

cjpatton left a comment

cjpatton Sep 7, 2023

divergentdave Sep 7, 2023

cjpatton Sep 7, 2023

branlwyd Sep 7, 2023 •

edited

divergentdave Sep 7, 2023

divergentdave Sep 7, 2023

branlwyd Sep 7, 2023 •

edited

branlwyd Sep 7, 2023

branlwyd commented Sep 7, 2023

cjpatton left a comment •

edited

branlwyd commented Sep 8, 2023

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

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

Conversation

branlwyd commented Sep 6, 2023 • edited

branlwyd commented Sep 6, 2023 • edited

cjpatton commented Sep 7, 2023

cjpatton left a comment

Choose a reason for hiding this comment

cjpatton Sep 7, 2023

Choose a reason for hiding this comment

divergentdave Sep 7, 2023

Choose a reason for hiding this comment

cjpatton Sep 7, 2023

Choose a reason for hiding this comment

branlwyd Sep 7, 2023 • edited

Choose a reason for hiding this comment

divergentdave Sep 7, 2023

Choose a reason for hiding this comment

divergentdave Sep 7, 2023

Choose a reason for hiding this comment

branlwyd Sep 7, 2023 • edited

Choose a reason for hiding this comment

branlwyd Sep 7, 2023

Choose a reason for hiding this comment

branlwyd commented Sep 7, 2023

cjpatton left a comment • edited

Choose a reason for hiding this comment

branlwyd commented Sep 8, 2023

branlwyd commented Sep 6, 2023 •

edited

branlwyd commented Sep 6, 2023 •

edited

branlwyd Sep 7, 2023 •

edited

branlwyd Sep 7, 2023 •

edited

cjpatton left a comment •

edited