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 #98

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cosmicexplorer
Copy link

@cosmicexplorer cosmicexplorer commented Dec 31, 2022

Problem

#79 proposed a ConstantTimePartialOrd trait, but that attempt was abandoned due to questions about how to create an Ordering without the use of match. This PR picks up where that left off and implements match-less and constant-time comparison operations for partially and totally orderable objects.

Solution

  1. Define traits ConstantTime{Partial,}Ord which return either a CtOption<Ordering> or an Ordering.
    • These are implemented automatically for types implementing ConstantTime{Eq,Greater}.
  2. Implement ConstantTime{Partial,}Ord 's comparison methods by extending ConditionallySelectable to Ordering.

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.

- copyright and rustfmt fix
- add ConstantTimeCmp
- use a lookup table to avoid branching
- implement ConstantTimePartialOrd
- expose `index_mutually_exclusive_logical_results()`
- make the constants static
- make ct_cmp() a default trait method for reasons described in docs
- implement ct_eq() for Ordering
src/lib.rs Outdated Show resolved Hide resolved
- instead, extend ConditionallySelectable to Ordering
@@ -852,7 +855,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.

This line here is the motivation for this change. The prior default implementation assumes that Self is totally ordered. We have moved this logic to ConstantTimeOrd's default implementation of ct_cmp(), requiring types to explicitly impl ConstantTimeOrd in order to use this "not equal or greater" check, while ConstantTimePartialOrd has to check all three of equals, less than, and greater.

@cosmicexplorer
Copy link
Author

cosmicexplorer commented Jan 4, 2023

This was part of the OP, but I've moved it out since it doesn't need to be part of the commit description:

Use Case

Justification for defining + implementing {Partial,}Ord traits upstream.

ConstantTime{Greater,Less}

#78 notes one application for the existing ConstantTime{Greater,Less}:

This would help implementers of some post-quantum algorithms which require constant-time sorting networks (such as lattice-based key exchanges where strong anonymity guarantees are required)

ConstantTime{Partial,}Ord

As noted in #79, rust does not make it easy to interact with an Ordering without branching via match, so to work around this the libsignal codebase hand rolled its own constant-time comparison operation constant_time_cmp(), which involves several complex bit manipulations. I was going to expose that method to our API consumers in signalapp/libsignal#469, but I was advised to raise this use case upstream instead.

libsignal uses constant_time_cmp() to implement Ord for our public keys after converting them into [u8] slices. We end 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.

@tarcieri
Copy link
Contributor

I think #102 might be a bit more minimal way of supporting Ordering.

It doesn't define any new traits, just impls the ConditionallySelectable, ConstantTimeEq, ConstantTimeGreater, and ConstantTimeLess traits, which makes it possible to write PartialOrd/Ord impls which are constant time.

That's not to say new traits aren't worth pursuing, but maybe a more minimal start would be easier to review than design for new traits.

@cosmicexplorer
Copy link
Author

@tarcieri:

It doesn't define any new traits, just impls the ConditionallySelectable, ConstantTimeEq, ConstantTimeGreater, and ConstantTimeLess traits, which makes it possible to write PartialOrd/Ord impls which are constant time.

This is super interesting and I hadn't considered this! However, I'm having some difficulty understanding how implementing ConstantTime{Greater,Less} for Ordering is actually used. This confusion is partially my fault: I mistakenly assumed that we would need to have Ordering implement ConstantTimeEq. In 1975526, you can see I have removed that impl because nothing was using it.

Please confirm: I cannot see how implementing ConstantTime{Eq,Less,Greater} for Ordering can be used to implement ConstantTime{Partial,}Ord! The Ordering (or CtOption<Ordering>) instance cannot be created without the ConstantTime{Partial,}Ord traits in this PR. Is this correct?

  1. We impl ConditionallySelectable for Ordering in this PR to enable the use of self.conditional_assign(...) to update an Ordering instance depending upon the result of some other constant-time operations (as suggested in define and implement ConstantTime{Partial,}Ord traits #98 (comment)).
  2. In totally ordered sets, a is strictly less than, equal to, or greater than b, so we only need two logical checks to determine the Ordering between a and b. self.conditional_assign() does the work of updating our Ordering result without branching.
  3. For partially ordered sets, a may additionally be incomparable to b, so we need three checks.

I do not believe that it is possible for a consumer of subtle to implement constant-time ordering without doing exactly what ConstantTime{Partial,}Ord do in this PR. Indeed, libsignal is essentially recreating ConstantTimeOrd by hand right now: https://github.com/signalapp/libsignal/blob/dd0315ad267d7bfc3118524285644a6d00fb7763/rust/protocol/src/utils.rs#L53-L70

pub(crate) fn constant_time_cmp(x: &[u8], y: &[u8]) -> Ordering {
    if x.len() < y.len() {
        return Ordering::Less;
    }
    if x.len() > y.len() {
        return Ordering::Greater;
    }

    let mut result: u8 = 0;

    for i in 0..x.len() {
        let a = x[x.len() - 1 - i];
        let b = y[x.len() - 1 - i];

        let is_eq = ct_is_eq(a, b);
        let is_lt = ct_is_lt(a, b);

        result = ct_select(is_eq, result, ct_select(is_lt, 1, 255));
    }

    debug_assert!(result == 0 || result == 1 || result == 255);

    if result == 0 {
        Ordering::Equal
    } else if result == 1 {
        Ordering::Less
    } else {
        Ordering::Greater
    }
}

@cosmicexplorer
Copy link
Author

cosmicexplorer commented Jan 17, 2023

I guess I'm trying to say two things:

  1. It's not clear to me that Add trait impls for core::cmp::Ordering #102 has any value, since it's still not possible to create an Ordering instance in constant time in the first place without the ConstantTime{Partial,}Ord traits in this PR. Please let me know if I'm right about that.
  2. We can see that libsignal is essentially writing out ConstantTimeOrd by hand right now, which implies other consumers of subtle may also be doing the same thing. This convergence may be further evidence that this PR's implementation of constant-time ordering is the minimal correct one.

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.

None yet

2 participants