ECDH and DH secret derivation: openssl_pkey_derive() #3197

commercebyte · 2018-03-26T22:35:25Z

string openssl_dh_compute_key(resource ec_pub_key,resource ec_priv_key)   [new]
string openssl_dh_compute_key(resource dh_pub_key,resource dh_priv_key)   [new]
string openssl_dh_compute_key(string dh_pub,resource dh_priv_key)         [original]

string openssl_dh_compute_key(resource ec_pub_key,resource ec_priv_key) [new] string openssl_dh_compute_key(resource dh_pub_key,resource dh_priv_key) [new] string openssl_dh_compute_key(string dh_pub,resource dh_priv_key) [original]

cmb69 · 2018-03-27T12:33:55Z

Apparently, this does not compile on Windows, see https://ci.appveyor.com/project/php/php-src/build/master.build.6583/job/72ja8je11sgdwbut#L2529.

nikic · 2018-03-27T14:46:54Z

/cc @bukka

commercebyte · 2018-03-27T19:03:45Z

@cmb69 thanks for the heads up - fixed

bukka · 2018-03-30T19:50:22Z

First of all, thanks for the PR!

There are couple of things:

I would prefer a new function for this so it's clear that it's for ecdh as it might be a bit confusing that dh also works for ecdh. The obvious name would be openssl_ecdh_compute_key :)
It would be good to add a test just checking that the function returns a non empty string for example.
Please could you also fix the CS issues - mainly indent after some if { and else {

bukka · 2018-03-30T20:01:36Z

Alternatively possibly even better solution would to do it on EVP level and use EVP_PKEY_derive. It means introducing something like openssl_pkey_derive that would handle both cases and use just EVP functions.

…ULL)

commercebyte · 2018-03-31T19:58:38Z

@bukka The latter definitely makes sense.
I believe openssl_derive($peer_pub_key,$priv_key,$keylen=NULL) is a better name, because openssl_pkey_* are functions that specifically operate on private/public keys.
We can leave the original openssl_dh_compute_key() unchanged as a legacy.
I added tests for openssl_derive(), both DH and ECDH.

Just an observation - when the optional keylen argument is supplied and is less then the maximum secret length, ECDH truncates the derived secret, while DH fails. It appears to be the normal behavior of the openssl lib.

…st as well

… reason -

bukka · 2018-04-03T17:35:44Z

ext/openssl/openssl.c

+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "rr|l", &peer_pub_key, &priv_key, &keylen) == FAILURE) {
+		return;
+	}
+	if ((pkey = (EVP_PKEY *)zend_fetch_resource(Z_RES_P(priv_key), "OpenSSL key", le_key)) == NULL


To be a bit more flexible and consistent with other functions in openssl ext, it might be better to fetch it using php_openssl_evp_from_zval and changing ZPP to "zz|l".

bukka · 2018-04-03T17:37:29Z

ext/openssl/openssl.c

+	if (!ctx) RETURN_FALSE;
+	if (EVP_PKEY_derive_init(ctx) > 0
+	 && EVP_PKEY_derive_set_peer(ctx, peer_key) > 0
+	 && (keylen > 0 || EVP_PKEY_derive(ctx, NULL, &keylen) > 0)


we should probably check keylen before and warn if it's negative.

bukka · 2018-04-03T17:40:04Z

ext/openssl/openssl.c

+		RETURN_FALSE;
+	}
+	EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
+	if (!ctx) RETURN_FALSE;


CS:

if (!ctx) { RETURN_FALSE; }

bukka · 2018-04-03T17:42:03Z

ext/openssl/openssl.c

+	EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
+	if (!ctx) RETURN_FALSE;
+	if (EVP_PKEY_derive_init(ctx) > 0
+	 && EVP_PKEY_derive_set_peer(ctx, peer_key) > 0


NIT CS: usually tab should used for indent of && on the next line

if (EVP_PKEY_derive_init(ctx) > 0 && EVP_PKEY_derive_set_peer(ctx, peer_key) > 0

@bukka Thanks for the feedback!

As for padding for a wrapped line - wanted to make it distinct from the padding on the following block,
changed to tabs now as it's consistent with other places in the code

bukka · 2018-04-03T17:46:10Z

It looks good! Just added couple of comments about minor things..

About the name I would really prefer openssl_pkey_derive as openssl_derive is very generic. I know there are some exceptions but it is good if it maps on OpenSSL name which is with PKEY prefix. Also I think it's pkey related as it only accepts pkeys... :)

commercebyte · 2018-04-03T21:04:54Z

@bukka Looks like everything's good,

Note - The function does not accept two private keys anymore, because of the way php_openssl_evp_from_zval() works.

It should not be a problem because deriving a secret from 2 private keys is not a normal use case, one of the private keys can be explicitly converted to public

bukka · 2018-04-04T17:10:30Z

ext/openssl/openssl.c

+	if (keylen < 0) {
+		php_error_docref(NULL, E_WARNING, "keylen < 0, assuming NULL");
+	}
+	if ((pkey = php_openssl_evp_from_zval(priv_key,0,"",0,0,NULL)) == NULL


NIT - CS: space after comma...

Same for following line ;)

bukka · 2018-04-04T17:17:13Z

All looks good (just one small NIT)!

As it's a new function, it would be good to message to internals mailing list. I don't think we need an RFC for this unless there are some objections. Are you ok to that (just a short description of the function and link to the PR should be enough)?

bukka · 2018-04-13T19:55:47Z

@commercebyte Just wanted to check if you are ok to message internals or if you want me to do it? I'm happy to do that if you want.

commercebyte · 2018-04-13T21:15:25Z

@bukka Sorry but it doesn't look like i have a post permission for http://news.php.net/php.internals
If you can help me to post or to get the right permissions i'll really appreciate.
Thank You!

bukka · 2018-04-20T20:01:08Z

@commercebyte I think you would need to be subscribed to be able to post but not really sure. Anyway I just emailed the mailing lists which should fine:

http://news.php.net/php.internals/102068

If no one objects and I don't forget (in that case please ping me :) ), I will merge it in the next 2 weeks.

Thanks

commercebyte · 2018-04-21T01:09:32Z

Thank You @bukka !

The description of the function might've been lost in the thread conversation, let me summarize once again -

string openssl_pkey_derive($peer_pub_key, $priv_key, int $keylen=NULL)

Derives a negotiated secret- DH, ECDH, or any future algorithm supported by EVP_PKEY_derive()

$peer_pub_key: peer's public key, string or resource (currently supported: DH or EC)

$priv_key: owner's private key, string or resource (currently supported: DH or EC, with same domain parameters as $peer_pub_key)

$keylen: requested length of the derived secret, NULL or 0 to use the default for the algorithm

Returns: derived secret string, or false on error

dol · 2018-04-23T10:48:12Z

Nice work. Looking forward for this capability.
I only see one thing that can be improved:

Use official test vectors to test your code. https://csrc.nist.gov/csrc/media/projects/cryptographic-algorithm-validation-program/documents/components/ecccdhtestvectors.zip for ECDH. Example: https://3v4l.org/DsrpG

And for DH: https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/key-management#Testing

php-pulls · 2018-05-09T19:01:07Z

Comment on behalf of bukka at php.net:

Merged via 897133f

bukka · 2018-05-09T19:08:59Z

@commercebyte I have just tested and merged it. Just one small thing with arginfo that I fixed. Otherwise all seems good! Thanks for you work!

@dol That would be really cool to add! I merged it with the supplied test as I don't want to have it as a requirement for the PR but if you or someone has time to create a PR, will be more than happy to merge it ;)

dol · 2018-05-10T09:59:37Z

ext/openssl/tests/ecc.phpt

@@ -1,7 +1,7 @@
 --TEST--
 openssl_*() with OPENSSL_KEYTYPE_EC
 --SKIPIF--
-<?php if (!extension_loaded("openssl") && !defined("OPENSSL_KEYTYPE_EC")) print "skip"; ?>
+<?php if (!extension_loaded("openssl") || !defined("OPENSSL_KEYTYPE_EC")) print "skip"; ?>


Thanks for fixing this. Was introduced 2 years ago. 9688138#diff-a2ffa4ffa39980a4fe2fb9a7cb455e6eR4 ;-)

dol · 2018-05-10T10:01:12Z

@bukka Not sure I find time. But I add it to my todo list.

ECDH support for openssl_dh_compute_key()

7861bad

string openssl_dh_compute_key(resource ec_pub_key,resource ec_priv_key) [new] string openssl_dh_compute_key(resource dh_pub_key,resource dh_priv_key) [new] string openssl_dh_compute_key(string dh_pub,resource dh_priv_key) [original]

windows fix

e925b43

krakjoe added Feature Waiting on Review labels Mar 27, 2018

vesvault-jz added 3 commits March 31, 2018 11:55

openssl_derive(resource peer_pub_key, resource priv_key, int keylen=N…

f2cfd3d

…ULL)

Merge branch 'master' into openssl_dh_compute_key-ecdh

b170b05

openssl_dh_compute_key reverted to the original

3feba65

commercebyte changed the title ~~ECDH support for openssl_dh_compute_key()~~ ECDH and DH secret derivation: openssl_derive() Mar 31, 2018

vesvault-jz added 2 commits March 31, 2018 16:18

fixed a wrong skip condition in ecc.phpt, was copied into the ecdh te…

bea922d

…st as well

an empty commit to restart appveyor, seems like it failed for no good…

46f9762

… reason -

bukka reviewed Apr 3, 2018

View reviewed changes

renamed to openssl_pkey_derive() -

e011b83

commercebyte force-pushed the master branch from b3742fa to e011b83 Compare April 3, 2018 19:37

commercebyte changed the title ~~ECDH and DH secret derivation: openssl_derive()~~ ECDH and DH secret derivation: openssl_pkey_derive() Apr 3, 2018

bukka reviewed Apr 4, 2018

View reviewed changes

NIT CS

cd853e6

php-pulls closed this May 9, 2018

dol reviewed May 10, 2018

View reviewed changes

dvaeversted mentioned this pull request Feb 5, 2021

Added missing documentation for openssl_pkey_derive php/doc-en#437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECDH and DH secret derivation: openssl_pkey_derive() #3197

ECDH and DH secret derivation: openssl_pkey_derive() #3197

commercebyte commented Mar 26, 2018

cmb69 commented Mar 27, 2018

nikic commented Mar 27, 2018

commercebyte commented Mar 27, 2018

bukka commented Mar 30, 2018

bukka commented Mar 30, 2018

commercebyte commented Mar 31, 2018

bukka Apr 3, 2018

bukka Apr 3, 2018

bukka Apr 3, 2018

bukka Apr 3, 2018

commercebyte Apr 3, 2018

bukka commented Apr 3, 2018

commercebyte commented Apr 3, 2018

bukka Apr 4, 2018

bukka Apr 4, 2018

bukka commented Apr 4, 2018

bukka commented Apr 13, 2018

commercebyte commented Apr 13, 2018

bukka commented Apr 20, 2018

commercebyte commented Apr 21, 2018 •

edited

dol commented Apr 23, 2018

php-pulls commented May 9, 2018

bukka commented May 9, 2018

dol May 10, 2018

dol commented May 10, 2018

ECDH and DH secret derivation: openssl_pkey_derive() #3197

ECDH and DH secret derivation: openssl_pkey_derive() #3197

Conversation

commercebyte commented Mar 26, 2018

cmb69 commented Mar 27, 2018

nikic commented Mar 27, 2018

commercebyte commented Mar 27, 2018

bukka commented Mar 30, 2018

bukka commented Mar 30, 2018

commercebyte commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Apr 3, 2018

commercebyte commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka commented Apr 4, 2018

bukka commented Apr 13, 2018

commercebyte commented Apr 13, 2018

bukka commented Apr 20, 2018

commercebyte commented Apr 21, 2018 • edited

dol commented Apr 23, 2018

php-pulls commented May 9, 2018

bukka commented May 9, 2018

Choose a reason for hiding this comment

dol commented May 10, 2018

commercebyte commented Apr 21, 2018 •

edited