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

define and implement ConstantTime{Partial,}Ord traits #5

Closed

Conversation

cosmicexplorer
Copy link

@cosmicexplorer cosmicexplorer commented Jun 30, 2022

dalek-cryptography#79 proposed a ConstantTimePartialOrd trait, but this was abandoned since (I am guessing) there wasn't a clear general use case, and there were concerns about how to create an Ordering without the use of match. This PR describes a general use case for constant-time ordering, and amends that PR with a naive attempt to avoid branching when creating an Ordering.

Use Case

In the signal rust codebase, we (I am not affiliated with Signal) handroll a function constant_time_cmp() to essentially implement Ord for [u8] slices, which we use to directly implement Ord for our public keys. I was going to expose that method to our API consumers in signalapp/libsignal#469, but I was advised to raise this use case to the subtle-ng crate instead.

Signal ends up consuming the Ord impl for public keys in our client library's FFI, which is then used to implement Ord interfaces in the foreign languages.

I think that if the proposed solution correctly achieves constant-time comparisons, it would benefit users of this library, who can now avoid hand-rolling their own Ord implementations which may not have been audited to correctly run in constant time.

Proposed Solution

  1. Define traits ConstantTime{PartialOrd,Ord} which return either a CtOption<Ordering> or an Ordering.
    • These are implemented automatically for types implementing ConstantTime{Eq,Greater}.

Note: I do not know if this implementation correctly avoids branching in the implementation of ct_cmp. It is directly based off of the code in dalek-cryptography#79, adding the use of a lookup table ORDERS instead of a match.

Result

Consumers of this library were already able to impl core::cmp::{Partial,}Eq with ConstantTimeEq to make ConstantTimeEq implementors usable as keys in HashSets, for example. This change provides a safe constant-time implementation of an equivalent to core::cmp::{Partial,}Ord so that users can ensure constant-time comparisons between keys in an ordered collection such as BTreeSet as well.

@cosmicexplorer
Copy link
Author

cosmicexplorer commented Jun 30, 2022

Note that unlike dalek-cryptography#79, I do not attempt to provide a constant-time analogue of PartialOrd named ConstantTimePartialOrd using CtOption<Ordering>. This is because while ConstantTimePartialOrd might be a useful trait in itself, it can't be automatically implemented the way ConstantTimeCmp in this PR can, in terms of ConstantTime{Eq,Greater} (since we have a total ordering if ConstantTime{Eq,Greater} can be defined).

However, if we think we'll want to have a separate ConstantTimePartialOrd trait in the future, then I can easily rename ConstantTimeCmp to ConstantTimeOrd -- I wanted to name it after the ct_cmp operation to be more similar to ct_{eq,gt,lt}.

EDIT: have now added back the ConstantTimePartialOrd trait!

@cosmicexplorer cosmicexplorer force-pushed the constant-time-cmp branch 3 times, most recently from 4fd40e6 to 8dce821 Compare June 30, 2022 08:23
@cosmicexplorer cosmicexplorer changed the title provide a ConstantTimeCmp trait provide a ConstantTimeCmp trait Jun 30, 2022
@cosmicexplorer cosmicexplorer changed the title provide a ConstantTimeCmp trait define and implement ConstantTime{Partial,}Ord traits Jul 1, 2022
@cosmicexplorer
Copy link
Author

After reminding myself that partial orders still have the equality, I have revised this PR to create a ConstantTimePartialOrd trait and slightly modified the default implementation of ct_lt().

@@ -778,7 +783,7 @@ pub trait ConstantTimeLess: ConstantTimeEq + ConstantTimeGreater {
/// ```
#[inline]
fn ct_lt(&self, other: &Self) -> Choice {
!self.ct_gt(other) & !self.ct_eq(other)
other.ct_gt(self)
Copy link
Author

Choose a reason for hiding this comment

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

The current default definition assumes a total ordering over Self, which could introduce sneaky logic errors for users! It also makes two calls to comparison functions instead of one. I believe this new default definition will now be correct for partially-ordered Self and will only ever need to be modified for cases where x < y does not imply y > x (which I believe is a strict subset of partially ordered sets).

@cosmicexplorer
Copy link
Author

ping @hdevalence @isislovecruft! Let me know if this is something the project would be interested in merging. I think the explanation for the change in this comment #5 (comment) is a good argument for introducing these traits.

@cosmicexplorer
Copy link
Author

This repo is unmaintained, use the original instead: https://github.com/dalek-cryptography/subtle.

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.

1 participant