[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
Sun May 14 16:33:29 PDT 2017
David,
If you go on a merging spree… I just wanted to remind you that this
patch is hanging out as well. It splits apart the ESP encryption and
HMAC keys so that they aren't concatenated in a Juniper-specific way.
I've tested it with both GlobalProtect and Juniper ESP VPNs and it
works fine. It hasn't been merged into my GlobalProtect branch,
however, because it isn't required for GP functionality… so if you
don't want to merge it in this form then it's not necessary.
-Dan
On Wed, Dec 28, 2016 at 9:38 AM, Daniel Lenski <dlenski at gmail.com> wrote:
> 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