Shame on me!!! I tested that this built and worked correctly only with GnuTLS, not OpenSSL. David, Looks like you already caught and patched it yourself: http://git.infradead.org/users/dwmw2/openconnect.git/blobdiff/b2b1dd0702239d415943cea6f821a345e0d50b63..d8b283f168c8e9574a821eef077b40984fae1026:/openssl-esp.c A user of openconnect-gp ran into this as well. Sorry about that. :-( -Dan On Sun, May 14, 2017 at 9:22 PM, 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 | 7 ++++--- > oncp.c | 37 +++++++++++++++++++++++++++---------- > openconnect-internal.h | 11 ++++++++++- > openssl-esp.c | 5 +++-- > 5 files changed, 48 insertions(+), 25 deletions(-) > > diff --git a/esp.c b/esp.c > index 44c9407..30ec442 100644 > --- a/esp.c > +++ b/esp.c > @@ -32,16 +32,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 0x02: > enctype = "AES-128-CBC (RFC3602)"; > - enclen = 16; > break; > case 0x05: > enctype = "AES-256-CBC (RFC3602)"; > - enclen = 32; > break; > default: > return -EINVAL; > @@ -49,20 +46,18 @@ int print_esp_keys(struct openconnect_info *vpninfo, const char *name, struct es > switch(vpninfo->esp_hmac) { > case 0x01: > mactype = "HMAC-MD5-96 (RFC2403)"; > - maclen = 16; > break; > case 0x02: > 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 1ad4e60..f3fd499 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, > @@ -111,7 +111,8 @@ int setup_esp_keys(struct openconnect_info *vpninfo) > esp_in = &vpninfo->esp_in[vpninfo->current_esp_in]; > > 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)); > diff --git a/oncp.c b/oncp.c > index 3c7cfa1..45c2f15 100644 > --- a/oncp.c > +++ b/oncp.c > @@ -302,11 +302,13 @@ static int process_attr(struct openconnect_info *vpninfo, int group, int attr, > > if (attrlen != 1) > goto badlen; > - if (data[0] == 0x02) > + if (data[0] == ENC_AES_128_CBC) { > enctype = "AES-128"; > - else if (data[0] == 0x05) > + 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); > @@ -319,11 +321,13 @@ static int process_attr(struct openconnect_info *vpninfo, int group, int attr, > > if (attrlen != 1) > goto badlen; > - if (data[0] == 0x01) > + if (data[0] == HMAC_MD5) { > mactype = "MD5"; > - else if (data[0] == 0x02) > + 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); > @@ -389,7 +393,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; > @@ -490,6 +495,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; > @@ -533,12 +539,21 @@ 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; > } > > + > int oncp_connect(struct openconnect_info *vpninfo) > { > int ret, len, kmp, kmplen, group; > @@ -786,7 +801,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")); > @@ -840,8 +856,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 a24a9e4..a54761f 100644 > --- a/openconnect-internal.h > +++ b/openconnect-internal.h > @@ -335,7 +335,8 @@ struct esp { > uint64_t seq_backlog; > uint64_t seq; > uint32_t spi; /* Stored network-endian */ > - unsigned char secrets[0x40]; > + unsigned char enc_key[0x40]; /* Encryption key */ > + unsigned char hmac_key[0x40]; /* HMAC key */ > }; > > struct openconnect_info { > @@ -359,6 +360,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; > @@ -684,6 +687,12 @@ struct openconnect_info { > #define AC_PKT_COMPRESSED 8 /* Compressed data */ > #define AC_PKT_TERM_SERVER 9 /* Server kick */ > > +/* Encryption and HMAC algorithms (matching Juniper's binary encoding) */ > +#define ENC_AES_128_CBC 2 > +#define ENC_AES_256_CBC 5 > +#define HMAC_MD5 1 > +#define HMAC_SHA1 2 > + > #define vpn_progress(_v, lvl, ...) do { \ > if ((_v)->verbose >= (lvl)) \ > (_v)->progress((_v)->cbdata, lvl, __VA_ARGS__); \ > diff --git a/openssl-esp.c b/openssl-esp.c > index e20bde0..faba1ff 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")); > @@ -151,7 +151,8 @@ int setup_esp_keys(struct openconnect_info *vpninfo) > esp_in = &vpninfo->esp_in[vpninfo->current_esp_in]; > > 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); > -- > 2.7.4 >
