Land program addresses on the curve

[jackcmay]

Problem

The Kudelski report identified that create_program_address may produce invalid public keys (off the curve)

Summary of Changes

land all those public keys on the curve

Fixes #10131

[codecov]

Codecov Report

Merging #11174 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #11174     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         318      318             
  Lines       74053    74070     +17     
=========================================
+ Hits        60846    60855      +9     
- Misses      13207    13215      +8     

[jackcmay]

@t-nelson @sakridge Sounds like you guys have a good grasp on curve stuff, would love any input you may have

[jackcmay]

I'm looking specifically to find out if the method in these changes is ok, I'm thinking that by generating the pubkey based on hashed bytes as a secret key we are essentially giving away the secret key and these addresses are not secure. There is no guarantee that the bytes we provide for the secret key are a valid key but there is a chance that they are. Maybe is sufficient to check that the secure key bytes are invalid?

If not secure we will have to implement the hash2curve math and that's going to take some time to get right.

[jackcmay]

Before the latest revision I was essentially feeding the hashed bytes to this function to create the public key which I think is secure:

    /// Internal utility function for mangling the bits of a (formerly
    /// mathematically well-defined) "scalar" and multiplying it to produce a
    /// public key.
    fn mangle_scalar_bits_and_multiply_by_basepoint_to_produce_public_key(bits: &mut [u8; 32]) -> PublicKey {
        bits[0]  &= 248;
        bits[31] &= 127;
        bits[31] |= 64;

        let pk = (&Scalar::from_bits(*bits) * &constants::ED25519_BASEPOINT_TABLE).compress().to_bytes();

        PublicKey(CompressedEdwardsY(pk))
    }

But mirroring the same on web3 resulted in a pulling a lot of internal code from nacl

[sakridge]

Before the latest revision I was essentially feeding the hashed bytes to this function to create the public key which I think is secure:

    /// Internal utility function for mangling the bits of a (formerly
    /// mathematically well-defined) "scalar" and multiplying it to produce a
    /// public key.
    fn mangle_scalar_bits_and_multiply_by_basepoint_to_produce_public_key(bits: &mut [u8; 32]) -> PublicKey {
        bits[0]  &= 248;
        bits[31] &= 127;
        bits[31] |= 64;

        let pk = (&Scalar::from_bits(*bits) * &constants::ED25519_BASEPOINT_TABLE).compress().to_bytes();

        PublicKey(CompressedEdwardsY(pk))
    }

But mirroring the same on web3 resulted in a pulling a lot of internal code from nacl

This one seems fine to me. Yea, I think you're right the PR is insecure in that the private key is easily derived.

[t-nelson]

This one seems fine to me. Yea, I think you're right the PR is insecure in that the private key is easily derived.

Agreed here. Using an open secret, like in this PR, would be a footgun for anyone who naively choose to reuse it as a signing authority

[jackcmay]

@t-nelson Do you concur that the scalar multiply is a sufficient method?

[t-nelson]

@jackcmay I believe so. I should review the math though.

I'm curious as to why they want us to put the addresses on the curve, where a private key definitely exists, but is currently difficult to find, rather than off the curve where any correct implementation would reject attempting to verify a signature with the derived pubkeys. Perhaps it's not a case of "on" and "off" so much as "definitely on" and "maybe on"?

There's some huge IETF spec about this I really should have read by now...

[jackcmay]

@t-nelson heh, yeah, interesting point. Here is the issue that spawned this effort: #10131

[aeyakovenko]

@t-nelson actually, not a bad idea. can we fail those sigchecks for pubkeys that are off-curve easily? @jackcmay would this be a cheaper computation?

[jackcmay]

@aeyakovenko I'm checking the computation load to check on curve.