Crypt::PK::DH Improvements

[lkinley]

Moving discussion from email.
You said:

---QUOTE---
in my opinion the right way is to enhance https://metacpan.org/pod/Crypt::PK::DH#generate_key

Now it supports only:
my $pk = Crypt::PK::DH->new();
$pk->generate_key($keysize);

like:
$pk->generate_key(256);

it should be extended to - let's say:
$pk->generate_key({ p=> '62D031C83F4294F64....', g=> '02' });

It would be also nice to patch https://metacpan.org/pod/Crypt::PK::DH#key2hash so that key dump contains also p + g (and maybe import/export routines will need some attention as well).

I did similar kind of extension for EC crypto in https://metacpan.org/pod/Crypt::PK::ECC#generate_key some time ago.

But keep in mind that it will need some nontrivial changes to cryptx/src/ltc/pk/dh/dh.c especially dh_make_key.

Basically I am not against major redesign of src/ltc/pk/dh/dh.c (I am member of libtomcrypt project so can try to push the changes upstream) but it is definitely a bunch of work.
---END QUOTE---

Other than supplying p and g to generate the key, I also need the following functionality, which is largely already written:
-Verify that a supplied public key is valid for my private key -- essentially a range check, y > 1 && y < (p - 1) and if g == 2 make sure more than one bit is set in y.
-Return public key in binary format. Not entirely necessary because we could unpack this from key2hash but nice nonetheless.

To modify Crypt::PK:DH, the path forward I see is:
-Add p,g to dh_key
-Remove index field from dh_key
-Redo import/export to include p,g in the packet and not index
-Change dh_get_size to return the used portion of the mp_int instead of looking up in table
-Free p,g parts of key in dh_free
-Change dh_encrypt_key, dh_decrypt_key to use p,g in key

I am not sure what to do with:
-dh_sizes function (probably leave alone).

Let me know your thoughts.

[karel-m]

He is how I see the necessary steps:

1/ in tomcrypt_pk.h extending dh_key struct like this

  typedef struct Dh_key {
      int idx, type;
      void *x;
      void *y;
+     void *base;
+     void *prime;
  } dh_key;

2/ in dh.c adding a new function dh_make_key_ex like this

int dh_make_key_ex(prng_state *prng, int wprng, char *prime_hex, char *base_hex, dh_key *key)

3/ turning dh_make_key into a wrapper fuction around the new dh_make_key_ex, dh_make_key will be used for backwards compatibility

4/ fixing the following functions so that they work with the updated dh_key structure

dh_free
dh_encrypt_key
dh_decrypt_key
dh_sign_hash
dh_verify_hash
dh_shared_secret

5/ In CryptX_PK_DH.xs.inc we need to change generate_key so that it accepts:

$pk->generate_key($keysize);
# or
$pk->generate_key($p_hex, $g_hex);
# or
$pk->generate_key({ p=> $p_hex, g => $g_hex });
# or whatever else

which means changing XS function like this (untested, just idea):

void
generate_key(Crypt::PK::DH self, ...)
    PPCODE:
    {
        int rv;
        /* add a small random entropy before generating key - not necessary as we have initialized prng with 256bit entropy in _new() */
        rv = rng_make_prng(64, self->yarrow_prng_index, &self->yarrow_prng_state, NULL);
        if (rv != CRYPT_OK) croak("FATAL: rng_make_prng failed: %s", error_to_string(rv));
        /* gen the key */
        if (items == 2) {
          rv = dh_make_key(&self->yarrow_prng_state, self->yarrow_prng_index, SvIV(ST(1)), &self->key);
        }
        if (items == 3) {
          rv = dh_make_key_ex(&self->yarrow_prng_state, self->yarrow_prng_index, SvPV_nolen(ST(1)), SvPV_nolen(ST(2)), &self->key);
        }
        if (rv != CRYPT_OK) croak("FATAL: dh_make_key failed: %s", error_to_string(rv));
        XPUSHs(ST(0)); /* return self */
    }

6/ at this point the old (current) test suite for DH in CryptX should PASS

7/ in the end we will need to fix dh_import + dh_export which will need some research whether it will be possible to make it somehow backwards compatible, we could definitely make it handy for binary key export (you have mentioned)

8/ in the end it might be a good idea to also support $dh->generate_key($dh_params_file) but you know better what DH related features have higher priority

[lkinley]

2/ int dh_make_key_ex(prng_state *prng, int wprng, char *prime_hex, char *base_hex, dh_key *key)

Did you mean 'dh_make_key_hex'? Are you against doing the hex->bin conversion in the XS layer, or do you feel it should be done in libtom? The code I've already written does it in the XS and the key generation function takes mp_int arguments. The code I've written will take binary, octal, hex, base64, or binary input for the prime, base arguments.

3/ Do a wrapper using a macro?

7/ The prime value will need to be stored otherwise a shared secret calculation won't be possible. At first glance, adding prime shouldn't break backwards compatibility.

[karel-m]

ad 2/ I really mean dh_make_key_ex like extended

As for passing mp_int arguments I do no think it is a good idea as libtomcrypt interface completely hides math c-types. If I recall correctly there is no function in public libtomcrypt interface that takes mp_int argument (and in the end we want dh_make_key_ex to become part of libtomcrypt public interface). In ecc_makekey_ex we do similar thing see https://github.com/DCIT/perl-CryptX/blob/master/src/ltc/pk/ecc/ecc_make_key.c#L96

ad 3/ I mean a wrapper like this

int dh_make_key(prng_state *prng, int wprng, int size, dh_key *key) {
  if (size == 128) {
    dh_make_key_ex(prng, wprng, CONST_DH_P128, CONST_DH_G128, key);
  }
  else if (size == 256) {
    dh_make_key_ex(prng, wprng, CONST_DH_P256, CONST_DH_G256, key);
  }
  //...

Simply implementing dh_make_key by calling dh_make_key_ex

[lkinley]

The predefined set of DH primes is in base64... So we can either convert the sets to hex in dh_static.c or do the conversion before passing it to dh_make_key_ex. Or do you prefer a different method given the above example using defined constants?

[karel-m]

I think the constants should be converted to HEX

[lkinley]

The primes in dh_static.c are not the length they are supposed to be... I think this was alluded to here: https://github.com/libtom/libtomcrypt/pull/111

For example, the DH-768 entry is 786 bits (131 base64 chars, 131 * 6 == 786)
The DH-4096 entry is 686 chars for 686 * 6 == 4116 bits

I wonder if there was a reason for this?

[karel-m]

You are right here is the complete list:

DH0768 real bits:784
DH1024 real bits:1036
DH1280 real bits:1288
DH1536 real bits:1540
DH1792 real bits:1792
DH2048 real bits:2072
DH2560 real bits:2576
DH3072 real bits:3080
DH4096 real bits:4116

[karel-m]

Just a note here that further discussion happens in PR #10

[karel-m]

Excellent work! Merged and released to CPAN as CryptX-0.032

I may try to bring some of these DH improvements upstream into libtomcrypt which is licensed as public domain (CryptX module is perl5). Do you agree with moving your contribution in PR #10 under public domain?

[lkinley]

                                                                                  Absolutely! That was my intent all along.                                                                                                                                                                                                                                                                                                                                         - Lance                                                                                                                                                                                                                From: karel-mSent: Wednesday, May 4, 2016 10:50 AMTo: DCIT/perl-CryptXReply To: DCIT/perl-CryptXCc: Lance Kinley; AuthorSubject: Re: [DCIT/perl-CryptX] Crypt::PK::DH Improvements (#4)Excellent work! Merged and released to CPAN as CryptX-0.032

I may try to bring some of these DH improvements upstream into libtomcrypt which is licensed as public domain (CryptX module is perl5). Do you agree with moving your contribution in PR #10 under public domain?

—You are receiving this because you authored the thread.Reply to this email directly or view it on GitHub

[karel-m]

Great, thanks for your time.

Closing.