[PATCH 1/5] tls: Fix tls12_prf buffer size

[PATCH 1/5] tls: Fix tls12_prf buffer size

[Andrew Zaborowski]

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

Enlarge the A(i) buffer to be able to hold the biggest possible hash
result (64) + the label + the biggest possible seed (64).  As reported
by Will Dietz the buffer size issue could cause crashes with the bigger
hashes when built under clang.

Reported-by: Will Dietz <w@wdtz.org>
---
 ell/tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/tls.c b/ell/tls.c
index 0e06c27..5efd928 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -92,7 +92,7 @@ bool tls12_prf(enum l_checksum_type type,
 {
 	struct l_checksum *hmac = l_checksum_new_hmac(type, secret, secret_len);
 	size_t a_len, chunk_len, prfseed_len = strlen(label) + seed_len;
-	uint8_t a[128], prfseed[prfseed_len];
+	uint8_t a[64 + prfseed_len], prfseed[prfseed_len];
 
 	if (!hmac)
 		return false;
-- 
2.20.1

[PATCH 2/5] tls: Update the docs for l_tls_set_cacert and _auth_data

[Andrew Zaborowski]

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

Explicitly state when these functions take ownership of the parameters
and make the semantics the same for both functions.  Drop a no-longer
applicable TODO comment.  (this is independent of the comments needing
to be moved to tls.c in doxygen format)
---
 ell/tls.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/ell/tls.h b/ell/tls.h
index 0ca6def..b67492b 100644
--- a/ell/tls.h
+++ b/ell/tls.h
@@ -98,19 +98,21 @@ void l_tls_write(struct l_tls *tls, const uint8_t *data, size_t len);
 /* Submit TLS payload from underlying transport to be decrypted */
 void l_tls_handle_rx(struct l_tls *tls, const uint8_t *data, size_t len);
 
-/* If peer is to be authenticated, supply the CA certificates */
+/*
+ * If peer is to be authenticated, supply the CA certificates.  On success
+ * the l_tls object takes ownership of the queue and the individual l_cert
+ * objects and they should not be freed by the caller afterwards.
+ */
 bool l_tls_set_cacert(struct l_tls *tls, struct l_queue *ca_certs);
 
 /*
- * If we are to be authenticated, supply our certificate and private key. On the
- * client this is optional.
- * TODO: allow NULL private key if certificate file contains the key.
+ * If we are to be authenticated, supply our certificate and private key.
+ * On the client this is optional.  On success, the l_tls object takes
+ * ownership of the certchain and the key objects and they should not be
+ * freed by the caller afterwards.
  * TODO: it may also be useful for the caller to be able to supply one
  * certificate of each type so they can be used depending on which is compatible
  * with the negotiated parameters.
- *
- * Note: Providing certchain and priv_key will move memory ownership into the
- *       tls object. These objects should not be freed by the caller.
  */
 bool l_tls_set_auth_data(struct l_tls *tls,
 				struct l_certchain *certchain,
-- 
2.20.1

[PATCH 3/5] tls: Don't free cert on l_tls_set_auth_data failure

[Andrew Zaborowski]

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

Make sure we don't free the key/cert on error return from this
function to be consistent with l_tls_set_cacert and their new
comments.
---
 ell/tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/tls.c b/ell/tls.c
index 5efd928..52e7924 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2847,7 +2847,7 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls,
 					L_CHECKSUM_NONE, &tls->priv_key_size,
 					&is_public) || is_public) {
 			TLS_DEBUG("Not a private key or l_key_get_info failed");
-			l_key_free(tls->priv_key);
+			tls->cert = NULL;
 			tls->priv_key = NULL;
 			tls->priv_key_size = 0;
 			return false;
-- 
2.20.1

[PATCH 4/5] examples: Only access argv up to argc

[Andrew Zaborowski]

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

There was an assumption that argv elements beyond argc were NULL.  This
may be true under some conditions but I guess it's better to not assume
that in example code.
---
 examples/https-client-test.c | 17 +++++++++++------
 examples/https-server-test.c |  6 ++++--
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/examples/https-client-test.c b/examples/https-client-test.c
index 281bdb8..11dce88 100644
--- a/examples/https-client-test.c
+++ b/examples/https-client-test.c
@@ -134,9 +134,9 @@ int main(int argc, char *argv[])
 	struct sockaddr_in addr;
 	int fd;
 	bool auth_ok;
-	struct l_certchain *cert;
-	struct l_key *priv_key;
-	struct l_queue *ca_cert;
+	struct l_certchain *cert = NULL;
+	struct l_key *priv_key = NULL;
+	struct l_queue *ca_cert = NULL;
 
 	if (argc != 2 && argc != 3 && argc != 6) {
 		printf("Usage: %s <https-host-name> [<ca-cert-path> "
@@ -192,9 +192,14 @@ int main(int argc, char *argv[])
 	if (getenv("TLS_DEBUG"))
 		l_tls_set_debug(tls, https_tls_debug_cb, NULL, NULL);
 
-	ca_cert = l_pem_load_certificate_list(argv[2]);
-	cert = l_pem_load_certificate_chain(argv[3]);
-	priv_key = l_pem_load_private_key(argv[4], argv[5], NULL);
+	if (argc >= 3)
+		ca_cert = l_pem_load_certificate_list(argv[2]);
+
+	if (argc >= 4)
+		cert = l_pem_load_certificate_chain(argv[3]);
+
+	if (argc >= 6)
+		priv_key = l_pem_load_private_key(argv[4], argv[5], NULL);
 
 	auth_ok = (argc <= 2 || l_tls_set_cacert(tls, ca_cert)) &&
 		(argc <= 5 ||
diff --git a/examples/https-server-test.c b/examples/https-server-test.c
index 93c0bcf..813650a 100644
--- a/examples/https-server-test.c
+++ b/examples/https-server-test.c
@@ -122,7 +122,7 @@ int main(int argc, char *argv[])
 	bool auth_ok;
 	struct l_certchain *cert;
 	struct l_key *priv_key;
-	struct l_queue *ca_cert;
+	struct l_queue *ca_cert = NULL;
 
 	if (argc != 4 && argc != 5) {
 		printf("Usage: %s <server-cert-path> <server-key-path> "
@@ -179,7 +179,9 @@ int main(int argc, char *argv[])
 
 	cert = l_pem_load_certificate_chain(argv[1]);
 	priv_key = l_pem_load_private_key(argv[2], argv[3], NULL);
-	ca_cert = l_pem_load_certificate_list(argv[4]);
+
+	if (argc >= 5)
+		ca_cert = l_pem_load_certificate_list(argv[4]);
 
 	auth_ok = l_tls_set_auth_data(tls, cert, priv_key) &&
 		(argc <= 4 || l_tls_set_cacert(tls, ca_cert)) &&
-- 
2.20.1

[PATCH 5/5] examples: Free tls certificate data on error

[Andrew Zaborowski]

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

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

diff --git a/examples/https-client-test.c b/examples/https-client-test.c
index 11dce88..eaa7900 100644
--- a/examples/https-client-test.c
+++ b/examples/https-client-test.c
@@ -208,8 +208,12 @@ int main(int argc, char *argv[])
 
 	if (tls && auth_ok)
 		l_main_run();
-	else
+	else {
 		printf("TLS setup failed\n");
+		l_queue_destroy(ca_cert, (l_queue_destroy_func_t) l_cert_free);
+		l_certchain_free(cert);
+		l_key_free(priv_key);
+	}
 
 	l_io_destroy(io);
 	l_tls_free(tls);
diff --git a/examples/https-server-test.c b/examples/https-server-test.c
index 813650a..2512601 100644
--- a/examples/https-server-test.c
+++ b/examples/https-server-test.c
@@ -189,8 +189,12 @@ int main(int argc, char *argv[])
 
 	if (tls && auth_ok)
 		l_main_run();
-	else
+	else {
 		printf("TLS setup failed\n");
+		l_queue_destroy(ca_cert, (l_queue_destroy_func_t) l_cert_free);
+		l_certchain_free(cert);
+		l_key_free(priv_key);
+	}
 
 	l_io_destroy(io);
 	l_tls_free(tls);
-- 
2.20.1

Re: [PATCH 1/5] tls: Fix tls12_prf buffer size

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 7:24 PM, Andrew Zaborowski wrote:
> Enlarge the A(i) buffer to be able to hold the biggest possible hash
> result (64) + the label + the biggest possible seed (64).  As reported
> by Will Dietz the buffer size issue could cause crashes with the bigger
> hashes when built under clang.
> 
> Reported-by: Will Dietz <w@wdtz.org>
> ---
>   ell/tls.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

All applied, thanks.

Regards,
-Denis