This patch is a bit? redundant. The mapping between encryption
algorithms and encryption key length is not only stored in the vpninfo
structure, but it's also re-computed in several different places.
I would like to use the gnutls/openssl functions to determine key
length everywhere, but this would mean (a) storing backend-specific
algorithm identifiers and (b) adding a lot more #ifdef GNUTLS/OPENSSL.
Any thoughts?
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
>
