elliptic-curve: make ExpandMsg fallible
#871
Conversation
As noted on #865 there are some potential overflow problems constructing the messages used for expansion, particularly around input lengths. This commit changes `ExpandMsg::expand_message`, and all of its transitive callers, to be fallible in order to handle these cases.
|
I'm currently working on making it safe on the type-level instead. In any case, we don't want to limit the message-length to 255 I believe. Looking into it. |
How? I'm unclear what part of this can be addressed with type-level safety.
It's limited to 65,535 bytes, as it's being encoded as a 16-bit integer (although I'm not sure what the additional trailing zero is for).
It's a possible alternative, although in this particular case it really feels like an error. As it were, |
|
Hash2curve is also supposed to be constant time. |
|
The implementation is still constant time with respect to the set of valid input messages. This is just adding error handling for invalid inputs (or inputs I think are invalid... I haven't actually read the relevant parts of the spec to confirm that). If you really want, we could have an accumulator for invalid inputs, miscompute the result via overflow, and make it a CtOption predicated on that flag, but I'm not sure that's necessarily desirable. |
I'm still looking into it, but for example the length in bytes should be addressable on a type level, because we only use a length of one or two internally. The reason is to make all of this infallible, as it's totally possible. |
|
It’s probably fine the way it is. I just noticed the similarity |
|
I'm already on it, if you give me just a bit I will submit another PR to do that :). |
|
I’m curious since the calculation comes from multiplying the number of needed bytes per field element by the number of field elements to create |
|
It's possible to compute it with The bounds can also get quite gnarly, and are "viral" when writing generic code. |
|
I'm attempting to make is a light-weight as possible, looking forward to your feedback then 😃 |
|
See #872. |
|
I can take a crack at reworking this with CtOption as well, at least for comparison |
|
I migrated fixes here: https://github.com/khonsulabs/traits/tree/expand-msg-improvements-2. Will notate them for your convenience here, but if you like to merge this as is and me making a PR, say the word. |
| let mut u = [Self::FieldElement::default(), Self::FieldElement::default()]; | ||
| hash_to_field::<X, _>(msg, dst, &mut u); | ||
| hash_to_field::<X, _>(msg, dst, &mut u)?; |
There was a problem hiding this comment.
Theoretically this can't fail if users are only using ExpandMsg specified in the spec and don't implement FromOkm on a curve with an incredibly high number, again, not specified in the spec.
WDYT? Should we unwrap? Personally, for VOPRF and OPAQUE I'm fine with fallible now.
There was a problem hiding this comment.
How about I merge this PR and you can open a follow-up to play with that?
|
Going to merge this and we can experiment with things like converting to |
As noted on #865 there are some potential overflow problems constructing the messages used for expansion, particularly around input lengths.
This commit changes
ExpandMsg::expand_message, and all of its transitive callers, to be fallible in order to handle these cases.cc @mikelodder7 @daxpedda