[PATCH] store length of ESP encryption and HMAC keys so that they can be manipulated separately for both Juniper and GP

Daniel Lenski dlenski at gmail.com
Wed Dec 28 09:38:50 PST 2016


David Woodhouse wrote:
> Daniel Lenski wrote:
>> -       unsigned char secrets[0x40];
>> +       unsigned char secrets[0x40]; /* Encryption key bytes, then HMAC key bytes */
>
> You're allowed to object to that horridness and split it into two
> separate fields for the encryption and HMAC keys, instead of just
> documenting it.
>
> In fact, one might argue that would be the better approach...

Signed-off-by: Daniel Lenski <dlenski at gmail.com>
---
 esp.c                  | 13 ++++---------
 gnutls-esp.c           |  4 ++--
 gpst.c                 | 28 +++++++++++++---------------
 oncp.c                 | 42 ++++++++++++++++++++++++++++++------------
 openconnect-internal.h |  5 ++++-
 openssl-esp.c          |  2 +-
 6 files changed, 54 insertions(+), 40 deletions(-)

diff --git a/esp.c b/esp.c
index f6b8f27..186858a 100644
--- a/esp.c
+++ b/esp.c
@@ -34,16 +34,13 @@ int print_esp_keys(struct openconnect_info *vpninfo, const char *name, struct es
 	int i;
 	const char *enctype, *mactype;
 	char enckey[256], mackey[256];
-	int enclen, maclen;
 
 	switch(vpninfo->esp_enc) {
 	case ENC_AES_128_CBC:
 		enctype = "AES-128-CBC (RFC3602)";
-		enclen = 16;
 		break;
 	case ENC_AES_256_CBC:
 		enctype = "AES-256-CBC (RFC3602)";
-		enclen = 32;
 		break;
 	default:
 		return -EINVAL;
@@ -51,20 +48,18 @@ int print_esp_keys(struct openconnect_info *vpninfo, const char *name, struct es
 	switch(vpninfo->esp_hmac) {
 	case HMAC_MD5:
 		mactype = "HMAC-MD5-96 (RFC2403)";
-		maclen = 16;
 		break;
 	case HMAC_SHA1:
 		mactype = "HMAC-SHA-1-96 (RFC2404)";
-		maclen = 20;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	for (i = 0; i < enclen; i++)
-		sprintf(enckey + (2 * i), "%02x", esp->secrets[i]);
-	for (i = 0; i < maclen; i++)
-		sprintf(mackey + (2 * i), "%02x", esp->secrets[enclen + i]);
+	for (i = 0; i < vpninfo->enc_key_len; i++)
+		sprintf(enckey + (2 * i), "%02x", esp->enc_key[i]);
+	for (i = 0; i < vpninfo->hmac_key_len; i++)
+		sprintf(mackey + (2 * i), "%02x", esp->hmac_key[i]);
 
 	vpn_progress(vpninfo, PRG_TRACE,
 		     _("Parameters for %s ESP: SPI 0x%08x\n"),
diff --git a/gnutls-esp.c b/gnutls-esp.c
index 14ea1da..f3ec72c 100644
--- a/gnutls-esp.c
+++ b/gnutls-esp.c
@@ -48,7 +48,7 @@ static int init_esp_ciphers(struct openconnect_info *vpninfo, struct esp *esp,
 	destroy_esp_ciphers(esp);
 
 	enc_key.size = gnutls_cipher_get_key_size(encalg);
-	enc_key.data = esp->secrets;
+	enc_key.data = esp->enc_key;
 
 	err = gnutls_cipher_init(&esp->cipher, encalg, &enc_key, NULL);
 	if (err) {
@@ -59,7 +59,7 @@ static int init_esp_ciphers(struct openconnect_info *vpninfo, struct esp *esp,
 	}
 
 	err = gnutls_hmac_init(&esp->hmac, macalg,
-			       esp->secrets + enc_key.size,
+			       esp->hmac_key,
 			       gnutls_hmac_get_len(macalg));
 	if (err) {
 		vpn_progress(vpninfo, PRG_ERR,
diff --git a/gpst.c b/gpst.c
index 3346194..f89e3d3 100644
--- a/gpst.c
+++ b/gpst.c
@@ -211,18 +211,18 @@ static int calculate_mtu(struct openconnect_info *vpninfo)
 static int set_esp_algo(struct openconnect_info *vpninfo, const char *s, int hmac)
 {
 	if (hmac) {
-		if (!strcmp(s, "sha1"))		{ vpninfo->esp_hmac = HMAC_SHA1; return 20; }
-		if (!strcmp(s, "md5"))		{ vpninfo->esp_hmac = HMAC_MD5; return 16; }
+		if (!strcmp(s, "sha1"))		{ vpninfo->esp_hmac = HMAC_SHA1; vpninfo->hmac_key_len = 20; return 0; }
+		if (!strcmp(s, "md5"))		{ vpninfo->esp_hmac = HMAC_MD5;  vpninfo->hmac_key_len = 16; return 0; }
 	} else {
 		if (!strcmp(s, "aes128") || !strcmp(s, "aes-128-cbc"))
-		                                { vpninfo->esp_enc = ENC_AES_128_CBC; return 16; }
-		if (!strcmp(s, "aes-256-cbc"))	{ vpninfo->esp_enc = ENC_AES_256_CBC; return 32; }
+		                                { vpninfo->esp_enc = ENC_AES_128_CBC; vpninfo->enc_key_len = 16; return 0; }
+		if (!strcmp(s, "aes-256-cbc"))	{ vpninfo->esp_enc = ENC_AES_256_CBC; vpninfo->enc_key_len = 32; return 0; }
 	}
 	vpn_progress(vpninfo, PRG_ERR, _("Unknown ESP %s algorithm: %s"), hmac ? "MAC" : "encryption", s);
 	return -ENOENT;
 }
 
-static int get_key_bits(xmlNode *xml_node, unsigned char *dest, int keybytes)
+static int get_key_bits(xmlNode *xml_node, unsigned char *dest)
 {
 	int bits = -1;
 	xmlNode *child;
@@ -233,12 +233,12 @@ static int get_key_bits(xmlNode *xml_node, unsigned char *dest, int keybytes)
 			bits = atoi(s);
 			free((void *)s);
 		} else if (xmlnode_get_text(child, "val", &s) == 0) {
-			for (p=s; *p && *(p+1) && p<s+(keybytes<<1); p+=2)
+			for (p=s; *p && *(p+1) && (bits-=8)>=0; p+=2)
 				*dest++ = unhex(p);
 			free((void *)s);
 		}
 	}
-	return (bits == (keybytes<<3)) ? 0 : -EINVAL;
+	return (bits == 0) ? 0 : -EINVAL;
 }
 
 /* Return value:
@@ -288,19 +288,17 @@ static int gpst_parse_config_xml(struct openconnect_info *vpninfo, xmlNode *xml_
 #ifdef HAVE_ESP
 			/* setup_esp_keys() will swap the active incoming key-set */
 			int c = (vpninfo->current_esp_in ^ 1);
-			int enclen=0, maclen=0;
 			for (member = xml_node->children; member; member=member->next) {
 				s = NULL;
 				if (!xmlnode_get_text(member, "udp-port", &s))		udp_sockaddr(vpninfo, atoi(s));
-				else if (!xmlnode_get_text(member, "enc-algo", &s)) 	enclen = set_esp_algo(vpninfo, s, 0);
-				else if (!xmlnode_get_text(member, "hmac-algo", &s))	maclen = set_esp_algo(vpninfo, s, 1);
+				else if (!xmlnode_get_text(member, "enc-algo", &s)) 	set_esp_algo(vpninfo, s, 0);
+				else if (!xmlnode_get_text(member, "hmac-algo", &s))	set_esp_algo(vpninfo, s, 1);
 				else if (!xmlnode_get_text(member, "c2s-spi", &s))	vpninfo->esp_out.spi = htonl(strtoul(s, NULL, 16));
 				else if (!xmlnode_get_text(member, "s2c-spi", &s))	vpninfo->esp_in[c].spi = htonl(strtoul(s, NULL, 16));
-				/* FIXME: this won't work if ekey or akey tags appears before algo tags */
-				else if (xmlnode_is_named(member, "ekey-c2s"))		get_key_bits(member, vpninfo->esp_out.secrets, enclen);
-				else if (xmlnode_is_named(member, "ekey-s2c"))		get_key_bits(member, vpninfo->esp_in[c].secrets, enclen);
-				else if (xmlnode_is_named(member, "akey-c2s"))		get_key_bits(member, vpninfo->esp_out.secrets+enclen, maclen);
-				else if (xmlnode_is_named(member, "akey-s2c"))		get_key_bits(member, vpninfo->esp_in[c].secrets+enclen, maclen);
+				else if (xmlnode_is_named(member, "ekey-c2s"))		get_key_bits(member, vpninfo->esp_out.enc_key);
+				else if (xmlnode_is_named(member, "ekey-s2c"))		get_key_bits(member, vpninfo->esp_in[c].enc_key);
+				else if (xmlnode_is_named(member, "akey-c2s"))		get_key_bits(member, vpninfo->esp_out.hmac_key);
+				else if (xmlnode_is_named(member, "akey-s2c"))		get_key_bits(member, vpninfo->esp_in[c].hmac_key);
 				free((void *)s);
 			}
 			if (vpninfo->dtls_state != DTLS_DISABLED) {
diff --git a/oncp.c b/oncp.c
index e6ff6c3..f5bcc75 100644
--- a/oncp.c
+++ b/oncp.c
@@ -286,11 +286,13 @@ static int process_attr(struct openconnect_info *vpninfo, int group, int attr,
 
 		if (attrlen != 1)
 			goto badlen;
-		if (data[0] == ENC_AES_128_CBC)
+		if (data[0] == ENC_AES_128_CBC) {
 			enctype = "AES-128";
-		else if (data[0] == ENC_AES_256_CBC)
+			vpninfo->enc_key_len = 16;
+		} else if (data[0] == ENC_AES_256_CBC) {
 			enctype = "AES-256";
-		else
+			vpninfo->enc_key_len = 32;
+		} else
 			enctype = "unknown";
 		vpn_progress(vpninfo, PRG_DEBUG, _("ESP encryption: 0x%02x (%s)\n"),
 			      data[0], enctype);
@@ -303,11 +305,13 @@ static int process_attr(struct openconnect_info *vpninfo, int group, int attr,
 
 		if (attrlen != 1)
 			goto badlen;
-		if (data[0] == HMAC_MD5)
+		if (data[0] == HMAC_MD5) {
 			mactype = "MD5";
-		else if (data[0] == HMAC_SHA1)
+			vpninfo->hmac_key_len = 16;
+		} else if (data[0] == HMAC_SHA1) {
 			mactype = "SHA1";
-		else
+			vpninfo->hmac_key_len = 20;
+		} else
 			mactype = "unknown";
 		vpn_progress(vpninfo, PRG_DEBUG, _("ESP HMAC: 0x%02x (%s)\n"),
 			      data[0], mactype);
@@ -373,7 +377,8 @@ static int process_attr(struct openconnect_info *vpninfo, int group, int attr,
 	case GRP_ATTR(7, 2):
 		if (attrlen != 0x40)
 			goto badlen;
-		memcpy(vpninfo->esp_out.secrets, data, 0x40);
+		/* data contains enc_key and hmac_key concatenated */
+		memcpy(vpninfo->esp_out.enc_key, data, 0x40);
 		vpn_progress(vpninfo, PRG_DEBUG, _("%d bytes of ESP secrets\n"),
 			     attrlen);
 		break;
@@ -474,6 +479,7 @@ static int parse_conf_pkt(struct openconnect_info *vpninfo, unsigned char *bytes
 {
 	int kmplen, kmpend, grouplen, groupend, group, attr, attrlen;
 	int ofs = 0;
+	int split_enc_hmac_keys = 0;
 
 	kmplen = load_be16(bytes + ofs + 18);
 	kmpend = ofs + kmplen;
@@ -517,9 +523,17 @@ static int parse_conf_pkt(struct openconnect_info *vpninfo, unsigned char *bytes
 				goto eparse;
 			if (process_attr(vpninfo, group, attr, bytes + ofs, attrlen))
 				goto eparse;
+			if (GRP_ATTR(group, attr)==GRP_ATTR(7, 2))
+				split_enc_hmac_keys = 1;
 			ofs += attrlen;
 		}
 	}
+
+	/* The encryption and HMAC keys are sent concatenated together in a block of 0x40 bytes;
+	   we can't split them apart until we know how long the encryption key is. */
+	if (split_enc_hmac_keys)
+		memcpy(vpninfo->esp_out.hmac_key, vpninfo->esp_out.enc_key + vpninfo->enc_key_len, vpninfo->hmac_key_len);
+
 	return 0;
 }
 
@@ -531,7 +545,8 @@ static int oncp_setup_esp_keys(struct openconnect_info *vpninfo)
 
 #if defined(OPENCONNECT_GNUTLS)
 	if ((ret = gnutls_rnd(GNUTLS_RND_NONCE, &esp_in->spi, sizeof(esp_in->spi))) ||
-	    (ret = gnutls_rnd(GNUTLS_RND_RANDOM, &esp_in->secrets, sizeof(esp_in->secrets)))) {
+	    (ret = gnutls_rnd(GNUTLS_RND_RANDOM, &esp_in->enc_key, vpninfo->enc_key_len)) ||
+	    (ret = gnutls_rnd(GNUTLS_RND_RANDOM, &esp_in->hmac_key, vpninfo->hmac_key_len)) ) {
 		vpn_progress(vpninfo, PRG_ERR,
 			     _("Failed to generate random keys for ESP: %s\n"),
 			     gnutls_strerror(ret));
@@ -539,7 +554,8 @@ static int oncp_setup_esp_keys(struct openconnect_info *vpninfo)
 	}
 #elif defined(OPENCONNECT_OPENSSL)
 	if (!RAND_bytes((void *)&esp_in->spi, sizeof(esp_in->spi)) ||
-	    !RAND_bytes((void *)&esp_in->secrets, sizeof(esp_in->secrets))) {
+	    !RAND_bytes((void *)&esp_in->enc_key, vpninfo->enc_key_len)) ||
+	    !RAND_bytes((void *)&esp_in->hmac_key, vpninfo->hmac_key_len)) ) {
 		vpn_progress(vpninfo, PRG_ERR,
 			     _("Failed to generate random keys for ESP:\n"));
 		openconnect_report_ssl_errors(vpninfo);
@@ -797,7 +813,8 @@ int oncp_connect(struct openconnect_info *vpninfo)
 		buf_append_bytes(reqbuf, esp_kmp_hdr, sizeof(esp_kmp_hdr));
 		buf_append_bytes(reqbuf, &esp->spi, sizeof(esp->spi));
 		buf_append_bytes(reqbuf, esp_kmp_part2, sizeof(esp_kmp_part2));
-		buf_append_bytes(reqbuf, &esp->secrets, sizeof(esp->secrets));
+		buf_append_bytes(reqbuf, &esp->enc_key, vpninfo->enc_key_len);
+		buf_append_bytes(reqbuf, &esp->hmac_key, 0x40 - vpninfo->enc_key_len);
 		if (buf_error(reqbuf)) {
 			vpn_progress(vpninfo, PRG_ERR,
 				     _("Error negotiating ESP keys\n"));
@@ -851,8 +868,9 @@ static int oncp_receive_espkeys(struct openconnect_info *vpninfo, int len)
 		p += sizeof(esp->spi);
 		memcpy(p, esp_kmp_part2, sizeof(esp_kmp_part2));
 		p += sizeof(esp_kmp_part2);
-		memcpy(p, esp->secrets, sizeof(esp->secrets));
-		p += sizeof(esp->secrets);
+		memcpy(p, esp->enc_key, vpninfo->enc_key_len);
+		memcpy(p+vpninfo->enc_key_len, esp->hmac_key, 0x40 - vpninfo->enc_key_len);
+		p += 0x40;
 		vpninfo->cstp_pkt->len = p - vpninfo->cstp_pkt->data;
 		store_le16(vpninfo->cstp_pkt->oncp.rec,
 			   (p - vpninfo->cstp_pkt->oncp.kmp));
diff --git a/openconnect-internal.h b/openconnect-internal.h
index c9f7b08..8deccca 100644
--- a/openconnect-internal.h
+++ b/openconnect-internal.h
@@ -348,7 +348,8 @@ struct esp {
 	uint64_t seq_backlog;
 	uint64_t seq;
 	uint32_t spi; /* Stored network-endian */
-	unsigned char secrets[0x40]; /* Encryption key bytes, then HMAC key bytes */
+	unsigned char enc_key[0x40]; /* Encryption key */
+	unsigned char hmac_key[0x40]; /* HMAC key */
 };
 
 struct openconnect_info {
@@ -372,6 +373,8 @@ struct openconnect_info {
 	int old_esp_maxseq;
 	struct esp esp_in[2];
 	struct esp esp_out;
+	int enc_key_len;
+	int hmac_key_len;
 
 	int tncc_fd; /* For Juniper TNCC */
 	const char *csd_xmltag;
diff --git a/openssl-esp.c b/openssl-esp.c
index 106a1f9..12dd761 100644
--- a/openssl-esp.c
+++ b/openssl-esp.c
@@ -99,7 +99,7 @@ static int init_esp_ciphers(struct openconnect_info *vpninfo, struct esp *esp,
 		destroy_esp_ciphers(esp);
 		return -ENOMEM;
 	}
-	if (!HMAC_Init_ex(esp->hmac, esp->secrets + EVP_CIPHER_key_length(encalg),
+	if (!HMAC_Init_ex(esp->hmac, esp->hmac_key,
 			  EVP_MD_size(macalg), macalg, NULL)) {
 		vpn_progress(vpninfo, PRG_ERR,
 			     _("Failed to initialize ESP HMAC\n"));
-- 
2.7.4




More information about the openconnect-devel mailing list