Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
GitLab
Merge request !1395 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/ecdh.c:

32
+  gnutls_ecc_curve_t curve_pub = GNUTLS_ECC_CURVE_INVALID, curve_priv = GNUTLS_ECC_CURVE_INVALID;
33
+  unsigned int bits_pub = 0, bits_priv = 0;
34
+  gnutls_datum_t priv_x = {NULL, 0}, priv_y = {NULL, 0}, priv_k = {NULL, 0}, pub_x = {NULL, 0}, pub_y = {NULL, 0};

Are those initializers really needed? Also, unlike nettle, we do use Linux kernel coding style with hard tabs :-)

Daiki Ueno started a new discussion on lib/ecdh.c:

76
+  gnutls_free(priv_k.data);
77
+  gnutls_free(pub_x.data);
78
+  gnutls_free(pub_y.data);

I suggest clearing the values with zeroize_key or gnutls_memset (at least for the private ones).

Daiki Ueno started a new discussion on lib/ecdh.c:

23
+/* Helper functions for ECC handling 
24
+ * based on public domain code by Tom St. Dennis.
25
+ */

Is this comment relevant?

Daiki Ueno started a new discussion on lib/ecdh.c:

28
+#include "errors.h"
29
+
30
+int gnutls_ecdh_compute_key(gnutls_privkey_t privkey, gnutls_pubkey_t pubkey, gnutls_datum_t *Z)

Would be nice to have a gtk-doc comment, explaining the usage of this function.

Daiki Ueno started a new discussion on lib/ecdh.c:

38
+  Z->size = 0;
39
+  
40
+  if (gnutls_privkey_get_pk_algorithm(privkey, &bits_priv) != GNUTLS_PK_ECDSA)

For FIPS, we deliberately don't support Curve25519/Curve448 (because they are not approved in FIPS140-2), but it might be worth adding support for them in the generic API.

Daiki Ueno started a new discussion on tests/ecdh-compute.c:

14 14
  * WITHOUT ANY WARRANTY; without even the implied warranty of
15
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
15
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the GNU

I suppose this whitespace change is not intended?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on lib/ecdh.c:

19
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>
20
+ *
21
+ */
22
+
23
+/* Helper functions for ECC handling 
24
+ * based on public domain code by Tom St. Dennis.
25
+ */
26
+#include "gnutls_int.h"
27
+#include <ecdh.h>
28
+#include "errors.h"
29
+
30
+int gnutls_ecdh_compute_key(gnutls_privkey_t privkey, gnutls_pubkey_t pubkey, gnutls_datum_t *Z)
31
+{
32
+  gnutls_ecc_curve_t curve_pub = GNUTLS_ECC_CURVE_INVALID, curve_priv = GNUTLS_ECC_CURVE_INVALID;
33
+  unsigned int bits_pub = 0, bits_priv = 0;
34
+  gnutls_datum_t priv_x = {NULL, 0}, priv_y = {NULL, 0}, priv_k = {NULL, 0}, pub_x = {NULL, 0}, pub_y = {NULL, 0};

On lines 52 and 58, I use gnutls_privkey_export_ecc_raw and gnutls_pubkey_export_ecc_raw to fill those gnutls_datum_t variables, if one fails, then there's a goto cleanup with gnutls_free for them.

I assume that if one of the *_export_ecc_raw fails and the gnutls_datum_t data are undefined, gnutls_free may segfault.

I can split cleanup goto step into 2 steps instead if you don't want gnutls_datum_t values to be initialized with {NULL, 0}?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on lib/ecdh.c:

10
+ * as published by the Free Software Foundation; either version 2.1 of
11
+ * the License, or (at your option) any later version.
12
+ *
13
+ * This library is distributed in the hope that it will be useful, but
14
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
15
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
16
+ * Lesser General Public License for more details.
17
+ *
18
+ * You should have received a copy of the GNU Lesser General Public License
19
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>
20
+ *
21
+ */
22
+
23
+/* Helper functions for ECC handling 
24
+ * based on public domain code by Tom St. Dennis.
25
+ */

nope, probably a wrong ^C^V :-)


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on lib/ecdh.c:

25
+ */
26
+#include "gnutls_int.h"
27
+#include <ecdh.h>
28
+#include "errors.h"
29
+
30
+int gnutls_ecdh_compute_key(gnutls_privkey_t privkey, gnutls_pubkey_t pubkey, gnutls_datum_t *Z)
31
+{
32
+  gnutls_ecc_curve_t curve_pub = GNUTLS_ECC_CURVE_INVALID, curve_priv = GNUTLS_ECC_CURVE_INVALID;
33
+  unsigned int bits_pub = 0, bits_priv = 0;
34
+  gnutls_datum_t priv_x = {NULL, 0}, priv_y = {NULL, 0}, priv_k = {NULL, 0}, pub_x = {NULL, 0}, pub_y = {NULL, 0};
35
+  int ret = GNUTLS_E_SUCCESS, res;
36
+  
37
+  Z->data = NULL;
38
+  Z->size = 0;
39
+  
40
+  if (gnutls_privkey_get_pk_algorithm(privkey, &bits_priv) != GNUTLS_PK_ECDSA)

Curve25519/Curve448 don't have a 'y' coordinate, so _gnutls_ecdh_compute_key wouldn't work.

I can use _gnutls_dh_compute_key for Curve25519/Curve448 if I assume it's the right way.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on tests/ecdh-compute.c:

12 12
  *
13 13
  * GnuTLS is distributed in the hope that it will be useful, but
14 14
  * WITHOUT ANY WARRANTY; without even the implied warranty of
15
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
15
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the GNU

indeed, thanks


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on lib/ecdh.c:

63
+    ret = res;
64
+    goto cleanup;
65
+  }
66
+
67
+  if ((res = _gnutls_ecdh_compute_key(curve_priv, &priv_x, &priv_y, &priv_k, &pub_x, &pub_y, Z)) != GNUTLS_E_SUCCESS)
68
+  {
69
+    ret = res;
70
+    goto cleanup;
71
+  }
72
+
73
+cleanup:
74
+  gnutls_free(priv_x.data);
75
+  gnutls_free(priv_y.data);
76
+  gnutls_free(priv_k.data);
77
+  gnutls_free(pub_x.data);
78
+  gnutls_free(pub_y.data);

Done


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Nicolas Mora commented on a discussion on lib/ecdh.c:

15
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
16
+ * Lesser General Public License for more details.
17
+ *
18
+ * You should have received a copy of the GNU Lesser General Public License
19
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>
20
+ *
21
+ */
22
+
23
+/* Helper functions for ECC handling 
24
+ * based on public domain code by Tom St. Dennis.
25
+ */
26
+#include "gnutls_int.h"
27
+#include <ecdh.h>
28
+#include "errors.h"
29
+
30
+int gnutls_ecdh_compute_key(gnutls_privkey_t privkey, gnutls_pubkey_t pubkey, gnutls_datum_t *Z)

gtk-doc comment added


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Add ecdh compute function gnutls_ecdh_compute_key (!1395)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Andreas Metzler commented on a discussion on lib/ecdh.c:

15
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
16
+ * Lesser General Public License for more details.
17
+ *
18
+ * You should have received a copy of the GNU Lesser General Public License
19
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>
20
+ *
21
+ */
22
+
23
+/* Helper functions for ECC handling 
24
+ * based on public domain code by Tom St. Dennis.
25
+ */
26
+#include "gnutls_int.h"
27
+#include <ecdh.h>
28
+#include "errors.h"
29
+
30
+int gnutls_ecdh_compute_key(gnutls_privkey_t privkey, gnutls_pubkey_t pubkey, gnutls_datum_t *Z)

The comment not very helpful it just avoids a warning about undocumented functions. The comment should the decribe inputs, outputs and and the actual point of the function. (When to call?)


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel