diff mbox

Correct the security weak construction of client_random and server_random in Client and Server Hellos.

Message ID CAGnO3doqKzZAs8Y=kYOqKqCXs3FQvx+zC2BhzEOratVHvR-EOw@mail.gmail.com
State Changes Requested
Headers show

Commit Message

Nick Lowe Feb. 10, 2016, 2:39 p.m. UTC 
Correct the security weak construction of client_random and
server_random in Client and Server Hellos. random_get_bytes(...) already
mixes in the current date and time via its entropy pool.

Signed-off-by: Nick Lowe <nick.lowe@lugatech.com>
---
 src/tls/tlsv1_client_write.c | 5 +----
 src/tls/tlsv1_server_write.c | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

Comments

Jouni Malinen Feb. 20, 2016, 5:17 p.m. UTC | #1 
On Wed, Feb 10, 2016 at 02:39:21PM +0000, Nick Lowe wrote:
> Correct the security weak construction of client_random and
> server_random in Client and Server Hellos. random_get_bytes(...) already
> mixes in the current date and time via its entropy pool.

Calling a 32-byte field with 28 bytes (224 bits!) of strong random data
a weak construction is pushing definition of "weak" pretty far.. This is
the way TLS has been defined at least up to and including TLS v1.2. I
know that there is an attempt to deprecate gmt_unix_time and remove it
at least from TLS v1.3. However, it does not look like
draft-mathewson-no-gmtunixtime-00 has yet been published as an RFC.
Should that happen, I'd be fine with this type of patch with the commit
message updated to point to that RFC as the reason instead of claims of
this being weak.
Nick Lowe Feb. 20, 2016, 5:22 p.m. UTC | #2 
Agree. I actually meant weaker than necessary, poorer would have been
a better choice of word.

Do you want me to reword and send again?

Nick
diff mbox

Patch

From 4562289b7bca77f7e2a9646fe305b1ce83593047 Mon Sep 17 00:00:00 2001
From: Nick Lowe <nick.lowe@lugatech.com>
Date: Wed, 10 Feb 2016 14:33:13 +0000
Subject: [PATCH] Correct the security weak construction of client_random and
 server_random in Client and Server Hellos. random_get_bytes(...) already
 mixes in the current date and time via its entropy pool.

Signed-off-by: Nick Lowe <nick.lowe@lugatech.com>
---
 src/tls/tlsv1_client_write.c | 5 +----
 src/tls/tlsv1_server_write.c | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/tls/tlsv1_client_write.c b/src/tls/tlsv1_client_write.c
index 04d895e..ae76a19 100644
--- a/src/tls/tlsv1_client_write.c
+++ b/src/tls/tlsv1_client_write.c
@@ -45,7 +45,6 @@  static size_t tls_client_cert_chain_der_len(struct tlsv1_client *conn)
 u8 * tls_send_client_hello(struct tlsv1_client *conn, size_t *out_len)
 {
 	u8 *hello, *end, *pos, *hs_length, *hs_start, *rhdr;
-	struct os_time now;
 	size_t len, i;
 	u8 *ext_start;
 	u16 tls_version = TLS_VERSION;
@@ -71,9 +70,7 @@  u8 * tls_send_client_hello(struct tlsv1_client *conn, size_t *out_len)
 		   tls_version_str(tls_version));
 	*out_len = 0;
 
-	os_get_time(&now);
-	WPA_PUT_BE32(conn->client_random, now.sec);
-	if (random_get_bytes(conn->client_random + 4, TLS_RANDOM_LEN - 4)) {
+	if (random_get_bytes(conn->client_random, TLS_RANDOM_LEN)) {
 		wpa_printf(MSG_ERROR, "TLSv1: Could not generate "
 			   "client_random");
 		return NULL;
diff --git a/src/tls/tlsv1_server_write.c b/src/tls/tlsv1_server_write.c
index bdc6c11..584462d 100644
--- a/src/tls/tlsv1_server_write.c
+++ b/src/tls/tlsv1_server_write.c
@@ -43,7 +43,6 @@  static int tls_write_server_hello(struct tlsv1_server *conn,
 				  u8 **msgpos, u8 *end)
 {
 	u8 *pos, *rhdr, *hs_start, *hs_length, *ext_start;
-	struct os_time now;
 	size_t rlen;
 
 	pos = *msgpos;
@@ -52,9 +51,7 @@  static int tls_write_server_hello(struct tlsv1_server *conn,
 	rhdr = pos;
 	pos += TLS_RECORD_HEADER_LEN;
 
-	os_get_time(&now);
-	WPA_PUT_BE32(conn->server_random, now.sec);
-	if (random_get_bytes(conn->server_random + 4, TLS_RANDOM_LEN - 4)) {
+	if (random_get_bytes(conn->server_random, TLS_RANDOM_LEN)) {
 		wpa_printf(MSG_ERROR, "TLSv1: Could not generate "
 			   "server_random");
 		return -1;
-- 
2.5.0