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