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

Additional SecretKeyFactory constructors #64

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Jul 12, 2021

Note that the derived key size is still 64 bytes in secret_key_by_label() to reduce aliasing when converting the derived bytes into a scalar (with modulo reduction). Since they pass through a hash function, I'm not sure if that's important.

Also, in Discord @cygnusv suggested using Scalar::from_bytes_reduced() instead of from_digest() to avoid the hashing step in secret_key_by_label(). I have the following concerns about it:

  • it takes a FieldBytes argument, which indicates that it's a semi-internal function - there is no "official" way to create FieldBytes. Technically, it is an alias for GenericArray, so it can be created, but seems a little fragile.
  • from_bytes_reduced() just takes a modulus, so if we're passing 32 bytes to it (the current size of FieldBytes), it can introduce aliasing.

@fjarri fjarri added enhancement New feature or request cryptography Needs attention of someone who knows what they're doing API Related to public API labels Jul 12, 2021
@fjarri fjarri added this to the v1.0.0 milestone Jul 12, 2021
@fjarri fjarri force-pushed the skf-constructors branch from 4f227cc to 26dfde6 Compare July 12, 2021 23:20
@fjarri fjarri changed the title Additional SecretKeyFactory constructors Additional SecretKeyFactory constructors Jul 12, 2021
@fjarri fjarri force-pushed the skf-constructors branch from 26dfde6 to 7fbe2f3 Compare July 18, 2021 04:33
@cygnusv
Copy link
Member

cygnusv commented Jul 26, 2021

  • from_bytes_reduced() just takes a modulus, so if we're passing 32 bytes to it (the current size of FieldBytes), it can introduce aliasing.

Not sure what you mean here by aliasing, but FWIW, the intermediate key material, resulting from the KDF, should be 64 bytes (_DERIVED_KEY_SIZE in pyUmbral), so the modulo is taken on a 64-bytes scalar.

@fjarri
Copy link
Contributor Author

fjarri commented Jul 26, 2021

Not sure what you mean here by aliasing, but FWIW, the intermediate key material, resulting from the KDF, should be 64 bytes (_DERIVED_KEY_SIZE in pyUmbral)

It is.

so the modulo is taken on a 64-bytes scalar.

But this, unfortunately, is not the case. Current implementation of from_bytes_reduced() in RustCrypto takes 32 bytes.

@cygnusv
Copy link
Member

cygnusv commented Jul 27, 2021

Current implementation of from_bytes_reduced() in RustCrypto takes 32 bytes.

If that's the case, then we'd need to use some modular arithmetic properties to work with 32 byte inputs.
If X is a 64-byte input, we can write it as X = A * 2^256 + B, where both A and B are 32 bytes.

Then X mod N = (A mod N) * (2^256 mod N) + (B mod N), where * and + are both modulo N operations. Granted that it will have an increased cost of 1 mod multiplication, 1 mod addition and 2 mod reductions, but we get rid of the aliasing problem. We do something similar in ReEncryptionValidator.sol, method extendedKeccakToBN().

@cygnusv
Copy link
Member

cygnusv commented Jul 27, 2021

@fjarri BTW, is the 32-byte restriction on from_bytes_reduced(), coming from this declaration on scalar.rs? If so, is it context-dependent? Trying to understand rust a bit.

//! Scalar field arithmetic.

use cfg_if::cfg_if;

cfg_if! {
    if #[cfg(any(target_pointer_width = "32", feature = "force-32-bit"))] {
        mod scalar_8x32;
        use scalar_8x32::Scalar8x32 as ScalarImpl;
        #[cfg(feature = "rand")]
        use scalar_8x32::WideScalar16x32 as WideScalarImpl;
    } else if #[cfg(target_pointer_width = "64")] {
        mod scalar_4x64;
        use scalar_4x64::Scalar4x64 as ScalarImpl;
        #[cfg(feature = "rand")]
        use scalar_4x64::WideScalar8x64 as WideScalarImpl;
    }
}

In any case, I don't see why the input to from_bytes_reduced() should be restricted to 32 bytes. Maybe the solution is to fix this upstream.

@fjarri
Copy link
Contributor Author

fjarri commented Aug 19, 2021

I'm going to merge that for the added functionality. The hashing to scalar is to be handled when fixing #35

@fjarri fjarri merged commit 849f468 into nucypher:master Aug 19, 2021
@fjarri fjarri deleted the skf-constructors branch August 19, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Related to public API cryptography Needs attention of someone who knows what they're doing enhancement New feature or request
Projects
None yet
3 participants