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

Supporting CtOption::map for non-Default types? #63

Open
gendx opened this issue Nov 28, 2019 · 5 comments · May be fixed by #118
Open

Supporting CtOption::map for non-Default types? #63

gendx opened this issue Nov 28, 2019 · 5 comments · May be fixed by #118

Comments

@gendx
Copy link

gendx commented Nov 28, 2019

For now, CtOption::map requires a Default type.

This is especially problematic for types which require some constant-time validation of the input in their public constructors, say with the following API.

impl T {
    pub fn from_bytes(bytes: &[u8; N]) -> CtOption<T> { ... }
}

In that case, exposing a Default for T would allow the caller to bypass from_bytes validation by creating a Default.

Could there be a variant of map with an explicitly provided dummy value? Same question for CtOption::and_then.
@hdevalence
Copy link
Contributor

Hmm, regardless of the Default bound, that code really looks like it should be using Option<T> or Result<T,E>, because if the input is invalid, the computation can't sensibly continue.

@hdevalence
Copy link
Contributor

One thing that might work to relax the Default bound would be to unconditionally apply the closure to the CtOption's T value, but I haven't thought it through yet...

@tarcieri
Copy link
Contributor

If #94 were addressed which would unlock using combinators like CtOption::map with heap-allocated types (which we need for the rsa and dsa crates), the use of Default in map would become a problem, because our T values can vary in precision (i.e. number of limbs), and the Default impl can't match precision to an existing value of T except by chance.

Unconditionally applying the closure to T would address this problem and prevent a spurious heap allocation (i.e. getting the Default) every time map is called.

It's also problematic to use Default because it's typically a "zero value" which may not make sense given the mathematical operation being applied in the closure, e.g. it could be a divisor and division-by-zero could potentially cause a panic, precluding the use of map in cases where it would work if the closure were unilaterally applied to the contained value instead of Default.

@tarcieri tarcieri mentioned this issue Nov 28, 2023
Open
tarcieri added a commit to tarcieri/subtle that referenced this issue Nov 29, 2023
@tarcieri
Add ConstantTimeSelect and ConstantTimeClone traits
c67166b 
`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 tarcieri linked a pull request Nov 29, 2023 that will close this issue
Open
tarcieri added a commit to tarcieri/subtle that referenced this issue Nov 29, 2023
@tarcieri
Add ConstantTimeSelect and ConstantTimeClone traits
3a8e7ca 
`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

tarcieri commented Nov 29, 2023 •
edited

I have proposed removing this bound in #118, although I worry it's a breaking change.

An non-breaking alternative would be to add new methods which do the same thing but don't have the bound.

@nuttycom
Copy link

nuttycom commented Feb 15, 2024 •
edited

I'm going to add my voice here that Default is a bad trait to begin with; if we force types to have a Default impl in order to make them interoperable with subtle, this also means that the Default impl is available in other places, which could cause the default value to inadvertently be constructed and incorrectly interpreted as valid data. This is a general type safety problem; Default just shouldn't exist, and forcing users to implement it is detrimental to the overall safety of systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ConstantTimeSelect and ConstantTimeClone traits tarcieri/subtle
4 participants
@tarcieri @nuttycom @hdevalence @gendx