[PATCH 1/7] ecdh: Erase local copy of shared-secret after use

[PATCH 1/7] ecdh: Erase local copy of shared-secret after use

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

---
 ell/ecdh.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ell/ecdh.c b/ell/ecdh.c
index 0b8c347..2c162e9 100644
--- a/ell/ecdh.c
+++ b/ell/ecdh.c
@@ -102,6 +102,8 @@ LIB_EXPORT bool l_ecdh_generate_shared_secret(const struct l_ecc_curve *curve,
 
 	*secret = _ecc_constant_new(curve, product->x, curve->ndigits * 8);
 
+	memset(product->x, 0, curve->ndigits * 8);
+	memset(product->y, 0, curve->ndigits * 8);
 	l_ecc_point_free(product);
 	l_ecc_scalar_free(z);
 
-- 
2.19.1

[PATCH 2/7] ecc: Erase freed values in l_ecc_scalar_free

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

Both the private key and the final shared secret in ECDH happen to be
scalars (the public key is a point).  We could have a separate function
to free sensitive scalars but it's probably ok to erase all scalars
perfomance wise.
---
 ell/ecc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ell/ecc.c b/ell/ecc.c
index 6b93e54..a59e8e4 100644
--- a/ell/ecc.c
+++ b/ell/ecc.c
@@ -427,5 +427,9 @@ LIB_EXPORT ssize_t l_ecc_scalar_get_data(const struct l_ecc_scalar *c,
 
 LIB_EXPORT void l_ecc_scalar_free(struct l_ecc_scalar *c)
 {
+	if (unlikely(!c))
+		return;
+
+	memset(c->c, 0, c->curve->ndigits * 8);
 	l_free(c);
 }
-- 
2.19.1

[PATCH 3/7] ecdh: Drop the curve parameter to l_ecdh_generate_shared_secret

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

We already have a pointer to the curve in the public and private keys so
we can skip this parameter.
---
 ell/ecdh.c | 6 ++----
 ell/ecdh.h | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/ell/ecdh.c b/ell/ecdh.c
index 2c162e9..5ecbd44 100644
--- a/ell/ecdh.c
+++ b/ell/ecdh.c
@@ -83,17 +83,15 @@ LIB_EXPORT bool l_ecdh_generate_key_pair(const struct l_ecc_curve *curve,
 	return true;
 }
 
-LIB_EXPORT bool l_ecdh_generate_shared_secret(const struct l_ecc_curve *curve,
+LIB_EXPORT bool l_ecdh_generate_shared_secret(
 				const struct l_ecc_scalar *private_key,
 				const struct l_ecc_point *other_public,
 				struct l_ecc_scalar **secret)
 {
+	const struct l_ecc_curve *curve = private_key->curve;
 	struct l_ecc_scalar *z;
 	struct l_ecc_point *product;
 
-	if (unlikely(!curve))
-		return false;
-
 	z = l_ecc_scalar_new_random(curve);
 
 	product = l_ecc_point_new(curve);
diff --git a/ell/ecdh.h b/ell/ecdh.h
index 4b14535..34cbc6b 100644
--- a/ell/ecdh.h
+++ b/ell/ecdh.h
@@ -42,8 +42,7 @@ bool l_ecdh_generate_key_pair(const struct l_ecc_curve *curve,
  * Generate a shared secret from a private/public key. secret is an out
  * parameters and must be freed.
  */
-bool l_ecdh_generate_shared_secret(const struct l_ecc_curve *curve,
-				const struct l_ecc_scalar *private_key,
+bool l_ecdh_generate_shared_secret(const struct l_ecc_scalar *private_key,
 				const struct l_ecc_point *other_public,
 				struct l_ecc_scalar **secret);
 
-- 
2.19.1

[PATCH 4/7] ecc: Relax length check in l_ecc_point_from_data

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

l_ecc_point_from_data handles four point formats and for the first three
the parser allows the len parameter to be longer than the ECC data while
for L_ECC_POINT_TYPE_FULL it checks that len is exactly the size of the
point data, relax this check to allow higher len values.
---
 ell/ecc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/ecc.c b/ell/ecc.c
index a59e8e4..6d9ab42 100644
--- a/ell/ecc.c
+++ b/ell/ecc.c
@@ -314,7 +314,7 @@ LIB_EXPORT struct l_ecc_point *l_ecc_point_from_data(
 
 		break;
 	case L_ECC_POINT_TYPE_FULL:
-		if (len != bytes * 2)
+		if (len < bytes * 2)
 			goto failed;
 
 		_ecc_be2native(p->y, (void *) data + bytes, curve->ndigits);
-- 
2.19.1

[PATCH 5/7] unit: Update l_ecdh_generate_shared_secret parameters

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

---
 unit/test-ecdh.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/unit/test-ecdh.c b/unit/test-ecdh.c
index cc0f3a8..d340f64 100644
--- a/unit/test-ecdh.c
+++ b/unit/test-ecdh.c
@@ -73,8 +73,8 @@ static void test_basic(const void *data)
 	assert(l_ecdh_generate_key_pair(curve, &private1, &public1));
 	assert(l_ecdh_generate_key_pair(curve, &private2, &public2));
 
-	assert(l_ecdh_generate_shared_secret(curve, private1, public2, &secret1));
-	assert(l_ecdh_generate_shared_secret(curve, private2, public1, &secret2));
+	assert(l_ecdh_generate_shared_secret(private1, public2, &secret1));
+	assert(l_ecdh_generate_shared_secret(private2, public1, &secret2));
 
 	assert(!memcmp(secret1->c, secret2->c, 32));
 
@@ -130,10 +130,8 @@ static void test_vector_p256(const void *data)
 
 	use_real_getrandom = false;
 
-	assert(l_ecdh_generate_shared_secret(curve, a_secret, b_public,
-						&a_shared));
-	assert(l_ecdh_generate_shared_secret(curve, b_secret, a_public,
-						&b_shared));
+	assert(l_ecdh_generate_shared_secret(a_secret, b_public, &a_shared));
+	assert(l_ecdh_generate_shared_secret(b_secret, a_public, &b_shared));
 
 	assert(!memcmp(a_shared->c, ss_buf, 32));
 	assert(!memcmp(b_shared->c, ss_buf, 32));
@@ -195,10 +193,8 @@ static void test_vector_p384(const void *data)
 
 	use_real_getrandom = false;
 
-	assert(l_ecdh_generate_shared_secret(curve, a_secret, b_public,
-						&a_shared));
-	assert(l_ecdh_generate_shared_secret(curve, b_secret, a_public,
-						&b_shared));
+	assert(l_ecdh_generate_shared_secret(a_secret, b_public, &a_shared));
+	assert(l_ecdh_generate_shared_secret(b_secret, a_public, &b_shared));
 
 	assert(!memcmp(a_shared->c, ss_buf, 48));
 	assert(!memcmp(b_shared->c, ss_buf, 48));
-- 
2.19.1

[PATCH 6/7] examples: Call l_tls_start in https examples

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

---
 examples/https-client-test.c | 3 ++-
 examples/https-server-test.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/examples/https-client-test.c b/examples/https-client-test.c
index ccf168e..614a1ce 100644
--- a/examples/https-client-test.c
+++ b/examples/https-client-test.c
@@ -182,7 +182,8 @@ int main(int argc, char *argv[])
 
 	auth_ok = (argc <= 2 || l_tls_set_cacert(tls, argv[2])) &&
 		(argc <= 5 ||
-		 l_tls_set_auth_data(tls, argv[3], argv[4], argv[5]));
+		 l_tls_set_auth_data(tls, argv[3], argv[4], argv[5])) &&
+		l_tls_start(tls);
 
 	if (tls && auth_ok)
 		l_main_run();
diff --git a/examples/https-server-test.c b/examples/https-server-test.c
index 9940a0b..8f7f8ca 100644
--- a/examples/https-server-test.c
+++ b/examples/https-server-test.c
@@ -175,7 +175,8 @@ int main(int argc, char *argv[])
 		l_tls_set_debug(tls, https_tls_debug_cb, NULL, NULL);
 
 	auth_ok = l_tls_set_auth_data(tls, argv[1], argv[2], argv[3]) &&
-		(argc <= 4 || l_tls_set_cacert(tls, argv[4]));
+		(argc <= 4 || l_tls_set_cacert(tls, argv[4])) &&
+		l_tls_start(tls);
 
 	if (tls && auth_ok)
 		l_main_run();
-- 
2.19.1

[PATCH 7/7] examples: Further improve https example error messages

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

---
 examples/https-client-test.c | 13 +++++++++++--
 examples/https-server-test.c |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/examples/https-client-test.c b/examples/https-client-test.c
index 614a1ce..9a765c6 100644
--- a/examples/https-client-test.c
+++ b/examples/https-client-test.c
@@ -38,9 +38,12 @@
 static struct l_io *io;
 static struct l_tls *tls;
 static const char *hostname;
+static bool ready;
 
 static void https_io_disconnect(struct l_io *io, void *user_data)
 {
+	if (!ready)
+		printf("socket disconnected\n");
 	l_main_quit();
 }
 
@@ -50,9 +53,11 @@ static bool https_io_read(struct l_io *io, void *user_data)
 	int l;
 
 	l = read(l_io_get_fd(io), buf, sizeof(buf));
-	if (l == 0)
+	if (l == 0) {
+		if (!ready)
+			printf("socket EOF\n");
 		l_main_quit();
-	else if (l > 0)
+	} else if (l > 0)
 		l_tls_handle_rx(tls, buf, l);
 
 	return true;
@@ -73,6 +78,7 @@ static void https_new_data(const uint8_t *data, size_t len, void *user_data)
 	while (len) {
 		r = write(1, data, len);
 		if (r < 0) {
+			printf("socket EOF\n");
 			l_main_quit();
 			break;
 		}
@@ -88,6 +94,7 @@ static void https_tls_write(const uint8_t *data, size_t len, void *user_data)
 	while (len) {
 		r = send(l_io_get_fd(io), data, len, MSG_NOSIGNAL);
 		if (r < 0) {
+			printf("socket send: %s\n", strerror(errno));
 			l_main_quit();
 			break;
 		}
@@ -101,6 +108,8 @@ static void https_tls_ready(const char *peer_identity, void *user_data)
 	uint8_t buf[2048];
 	int l;
 
+	ready = true;
+
 	if (peer_identity)
 		printf("Server authenticated as %s\n", peer_identity);
 	else
diff --git a/examples/https-server-test.c b/examples/https-server-test.c
index 8f7f8ca..4f706ca 100644
--- a/examples/https-server-test.c
+++ b/examples/https-server-test.c
@@ -93,7 +93,7 @@ static void https_tls_write(const uint8_t *data, size_t len, void *user_data)
 	while (len) {
 		r = send(l_io_get_fd(io), data, len, MSG_NOSIGNAL);
 		if (r < 0) {
-			printf("send error\n");
+			printf("send: %s\n", strerror(errno));
 			l_main_quit();
 			break;
 		}
-- 
2.19.1

Re: [PATCH 1/7] ecdh: Erase local copy of shared-secret after use

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

Hi Andrew,

On 12/21/2018 04:03 PM, Andrew Zaborowski wrote:
> ---
>   ell/ecdh.c | 2 ++
>   1 file changed, 2 insertions(+)
> 

All 7 applied, thanks.

Regards,
-Denis