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

Add ConstantTimeSelect and ConstantTimeClone traits #118

Closed

Conversation

tarcieri
Copy link
Contributor

ConstantTimeSelect is intended as a replacement to ConditionallySelectable, which is preserved but deprecated. It replaces the previous Copy bound with a bound on a new ConstantTimeClone marker trait, which allows the trait to be impl'd for heap-allocated types.

No existing impls of ConditionallySelectable have been removed, however a blanket impl of ConstantTimeSelect for T: ConditionallySelectable has been added, allowing the two traits to interoperate and for ConstantTimeSelect to work on all types which currently impl ConditionallySelectable.

ConstantTimeClone likewise has a blanket impl for all types which impl Copy.

CtOption's combinator methods have been changed to bound on ConstantTimeSelect which unlocks using them with heap-allocated types, which otherwise is a major limitation. In theory these changes are all backwards compatible due to the blanket impl, which should allow all types which previously worked to continue to do so.

Closes #63, #94

@@ -752,17 +862,9 @@ impl<T> CtOption<T> {
#[inline]
pub fn map<U, F>(self, f: F) -> CtOption<U>
where
T: Default + ConditionallySelectable,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This resolves #63, although I do worry that removing the bounds is potentially breaking, as presently it should be possible to call Default::default or the ConditionallySelectable methods on T within the scope of these callbacks in generic code without explicitly adding these bounds.

While Default could be preserved (even if it's no longer used), unless ConditionallySelectable is removed this method can't be used with ConstantTimeSelect types.

For full backwards compatibility, an alternative would be to add new methods which use the new trait (something like map_ext or map_ct). Or perhaps we could try this and see if it actually causes any real-world breakages (we could always yank that release and ship a new one which adds new methods that use the new trait).

Comment on lines -759 to -763
f(T::conditional_select(
&T::default(),
&self.value,
self.is_some,
)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my notes here regarding this: #63 (comment)

It's actually quite problematic for the case of heap-allocated values, since they have a dynamically chosen size (e.g. number of limbs) and T::default() may not match that number of limbs, leading to timing variability.

Unconditionally invoking f(self.value) would prevent any possible timing variability from this function.

`ConstantTimeSelect` is intended as a replacement to
`ConditionallySelectable`, which is preserved but deprecated. It
replaces the previous `Copy` bound with a bound on a new
`ConstantTimeClone` marker trait, which allows the trait to be impl'd
for heap-allocated types.

No existing impls of `ConditionallySelectable` have been removed,
however a blanket impl of `ConstantTimeSelect` for
`T: ConditionallySelectable` has been added, allowing the two traits to
interoperate and for `ConstantTimeSelect` to work on all types which
currently impl `ConditionallySelectable`.

`ConstantTimeClone` likewise has a blanket impl for all types which impl
`Copy`.

`CtOption`'s combinator methods have been changed to bound on
`ConstantTimeSelect` which unlocks using them with heap-allocated types,
which otherwise is a major limitation. In theory these changes are all
backwards compatible due to the blanket impl, which should allow all
types which previously worked to continue to do so.

Closes dalek-cryptography#63, dalek-cryptography#94
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 1, 2023

Another option here would be to make the breaking changes necessary to solve #63 and #94, retaining the current name for ConditionallySelectable, but removing the Copy bound, and removing the default bounds in the CtOption combinator methods.

While technically breaking, most users of subtle could work with either version that way, and could change their version requirements to something like:

subtle = ">=2, <4"

This would allow compatible crates to work with either subtle v2 or v3, which would allow for non-breaking upgrades to e.g. curve25519-dalek and ed25519-dalek though testing both combinations might be a bit painful (we use a minimum-versions resolution in @RustCrypto to test this sort of thing).

Still, a major version bump would be annoying, but full backwards compatibility is a bit hard to preserve here.

@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 2, 2023

Yet another option would be to use the semver trick to provide backwards compatibility between subtle v2 and v3 which would enable an incremental upgrade, and also avoid changing trait/method names.

In this case, I think that would need to involve subtle v3 providing opt-in blanket impls for subtle v2 traits similar to the blanket impl of ConstantTimeEq for ConditionallySelectable in this PR (that's a bit different from the typical "semver trick", where the old crates import the new ones, but needed to make the blanket impls work since removing the Copy bound means not all types which can impl this PR's ConstantTimeSelect can impl ConditionallySelectable).

tarcieri added a commit to RustCrypto/crypto-bigint that referenced this pull request Dec 17, 2023
We can't impl `subtle::ConditionallySelectable` for `Boxed*` types due
to a bound on `Copy`.

I've proposed various ways to try to fix this upstream, e.g.
dalek-cryptography/subtle#118, from which the `ConstantTimeSelect` trait
has been extracted. It provides the same API as
`ConditionallySelectable` but without the `Copy` bound.

A blanket impl of `ConstantTimeSelect` for all `ConditionallySelectable`
types means that all stack allocated types can continue to use
`ConditionallySelectable` and will receive an impl of
`ConstantTimeSelect` as well.

A bound on `ConstantTimeSelect` has been added to the `Integer` trait as
well, allowing it to be used on all `*Uint` types in this crate with
`Integer` as the trait abstraction.
@tarcieri
Copy link
Contributor Author

I've been thinking more about the "semver trick" approach and how it could be done the proper way according to that blog: releasing a subtle v3 with breaking changes, but adding something like a v3 feature to a future subtle v2.x which pulls in subtle v3 as a dependency and re-exports everything which hasn't changed (which would be everything except ConditionallySelectable and CtOption), gating the rest of the old implementation out and using the subtle v3 traits/types instead.

subtle v2 could also blanket impl its ConditionallySelectable trait for subtle v3's ConditionallySelectable + Copy, and provide From impls for converting the CtOption types.

That would provide a huge degree of interop between the two versions to allow for a gradual transition, and also ensure that subtle v3 has no dependency on subtle v2.

tarcieri added a commit to RustCrypto/crypto-bigint that referenced this pull request Dec 17, 2023
We can't impl `subtle::ConditionallySelectable` for `Boxed*` types due
to a bound on `Copy`.

I've proposed various ways to try to fix this upstream, e.g.
dalek-cryptography/subtle#118, from which the `ConstantTimeSelect` trait
has been extracted. It provides the same API as
`ConditionallySelectable` but without the `Copy` bound.

A blanket impl of `ConstantTimeSelect` for all `ConditionallySelectable`
types means that all stack allocated types can continue to use
`ConditionallySelectable` and will receive an impl of
`ConstantTimeSelect` as well.

A bound on `ConstantTimeSelect` has been added to the `Integer` trait as
well, allowing it to be used on all `*Uint` types in this crate with
`Integer` as the trait abstraction.
tarcieri added a commit to tarcieri/subtle that referenced this pull request Aug 3, 2024
This commit contains a minimal set of proposed breaking changes, as an
alternative to trying to make additive changes which may be "subtly"
breaking:

- Add `ConstantTimeClone` trait and use it as
  `ConditionallySelectable`'s supertrait bound (alternative to dalek-cryptography#118):
  this makes it possible to implement `ConditionallySelectable` on
  heap-allocated types (fixes dalek-cryptography#94)
- Remove `Default` bound on `CtOption::map` and `CtOption::and_then`:
  allows using these methods with types that don't impl `Default` and
  avoids potential timing sidechannels if there might be timing
  variability in `Default` types (fixes dalek-cryptography#63)
@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 3, 2024

Note: opened #136 as an alternative which makes a deliberate breaking change to ConditionallySelectable's supertrait bound as an alternative to this PR, since this PR contains changes I'd consider subtly breaking

@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 3, 2024

Closing in favor of #137

@tarcieri tarcieri closed this Aug 3, 2024
@tarcieri tarcieri deleted the constant-time-select branch August 3, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting CtOption::map for non-Default types?
1 participant