-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 commit 7d0ca355a5c7f8337130d4b0b3e7686f2fa4d4c2 Author: Paul Wouters Date: Thu Apr 25 12:44:55 2013 -0400 * security: atodn() / atoid() buffer overflow lib/libswan/x509dn.c:atodn() does not perform any length checking whatsoever on the output buffer. Affected: - Libreswan 3.0 and 3.1 (3.2 disabled the oe= option) - Openswan versions up to and including 2.6.38 - Possibly certain strongswan 3.x/4.x versions This overflow is exposed (pre-authentication) only in opportunistic encryption mode. When it is called via receiving a certificate via IKEv1 or IKEv2, and when it is loaded from disk, the buffers passed to atodn() are big enough. This means this vulnerability can only be triggered when: - Opportunistic Encryption is enabled (oe=yes) - The attacker is local in the same network and adds a malicious reverse DNS record to the client's IP, or - The attacker can trigger an OE DNS lookup to a client fully configured with OE and their own key. Libreswan and openswan versions do not enable Opportunistic Encryption per default. Most distributions like RHEL, Fedora, Debian and Ubuntu also do not enable OE per default. This patch addresses the vulnerability in atodn() and further limits the atoid() call not to traverse into the ASN1 case when triggered by non-cert cases such as opportunistic encryption. Vulnerability discoverd by Florian Weimer of the Red Hat Product Security Team. Patch by D. Hugh Redelmeier and Paul Wouters diff --git a/include/asn1.h b/include/asn1.h index d69ebf9..b812488 100644 - --- a/include/asn1.h +++ b/include/asn1.h @@ -84,8 +84,10 @@ typedef enum { #define ASN1_BODY 0x20 #define ASN1_RAW 0x40 - -#define ASN1_INVALID_LENGTH 0xffffffff +#define ASN1_INVALID_LENGTH (~(size_t) 0) /* largest size_t */ +#define ASN1_MAX_LEN (1U << (8*3)) /* don't handle objects with length greater than this */ +#define ASN1_MAX_LEN_LEN 4 /* no coded length takes more than 4 bytes. */ /* definition of an ASN.1 object */ diff --git a/include/id.h b/include/id.h index d1825b4..b440a11 100644 - --- a/include/id.h +++ b/include/id.h @@ -47,7 +47,7 @@ extern const struct id *resolve_myid(const struct id *id); extern void set_myFQDN(void); extern void free_myFQDN(void); - -extern err_t atoid(char *src, struct id *id, bool myid_ok); +extern err_t atoid(char *src, struct id *id, bool myid_ok, bool oe_only); extern void iptoid(const ip_address *ip, struct id *id); extern unsigned char* temporary_cyclic_buffer(void); extern int idtoa(const struct id *id, char *dst, size_t dstlen); diff --git a/lib/libswan/id.c b/lib/libswan/id.c index 4442971..31ca7e5 100644 - --- a/lib/libswan/id.c +++ b/lib/libswan/id.c @@ -58,27 +58,29 @@ temporary_cyclic_buffer(void) /* Convert textual form of id into a (temporary) struct id. * Note that if the id is to be kept, unshare_id_content will be necessary. + * This function should be split into parts so the boolean arguments can be + * removed -- Paul */ err_t - -atoid(char *src, struct id *id, bool myid_ok) +atoid(char *src, struct id *id, bool myid_ok, bool oe_only) { err_t ugh = NULL; *id = empty_id; - - if (myid_ok && streq("%myid", src)) + if (!oe_only && myid_ok && streq("%myid", src)) { id->kind = ID_MYID; } - - else if (streq("%fromcert", src)) + else if (!oe_only && streq("%fromcert", src)) { id->kind = ID_FROMCERT; } - - else if (streq("%none", src)) + else if (!oe_only && streq("%none", src)) { id->kind = ID_NONE; } - - else if (strchr(src, '=') != NULL) + else if (!oe_only && strchr(src, '=') != NULL) { /* we interpret this as an ASCII X.501 ID_DER_ASN1_DN */ id->kind = ID_DER_ASN1_DN; @@ -112,7 +114,7 @@ atoid(char *src, struct id *id, bool myid_ok) { if (*src == '@') { - - if (*(src+1) == '#') + if (!oe_only && *(src+1) == '#') { /* if there is a second specifier (#) on the line * we interprete this as ID_KEY_ID @@ -123,7 +125,7 @@ atoid(char *src, struct id *id, bool myid_ok) ugh = ttodata(src+2, 0, 16, (char *)id->name.ptr , strlen(src), &id->name.len); } - - else if (*(src+1) == '~') + else if (!oe_only && *(src+1) == '~') { /* if there is a second specifier (~) on the line * we interprete this as a binary ID_DER_ASN1_DN @@ -134,7 +136,7 @@ atoid(char *src, struct id *id, bool myid_ok) ugh = ttodata(src+2, 0, 16, (char *)id->name.ptr , strlen(src), &id->name.len); } - - else if (*(src+1) == '[') + else if (!oe_only && *(src+1) == '[') { /* if there is a second specifier ([) on the line * we interprete this as a text ID_KEY_ID, and we remove diff --git a/lib/libswan/secrets.c b/lib/libswan/secrets.c index 6e9466b..8ff80e0 100644 - --- a/lib/libswan/secrets.c +++ b/lib/libswan/secrets.c @@ -1223,7 +1223,7 @@ lsw_process_secret_records(struct secret **psecrets, int verbose, } else { - - ugh = atoid(flp->tok, &id, FALSE); + ugh = atoid(flp->tok, &id, FALSE, FALSE); } if (ugh != NULL) diff --git a/lib/libswan/x509dn.c b/lib/libswan/x509dn.c index 61407e5..7731856 100644 - --- a/lib/libswan/x509dn.c +++ b/lib/libswan/x509dn.c @@ -472,7 +472,7 @@ static const x501rdn_t x501rdns[] = { {"TCGID" , {oid_TCGID, 12}, ASN1_PRINTABLESTRING} }; - -#define X501_RDN_ROOF 24 +#define X501_RDN_ROOF elemsof(x501rdns) /* Maximum length of ASN.1 distinquished name */ #define ASN1_BUF_LEN 512 @@ -775,11 +775,11 @@ atodn(char *src, chunk_t *dn) UNKNOWN_OID = 4 } state_t; - - u_char oid_len_buf[3]; - - u_char name_len_buf[3]; - - u_char rdn_seq_len_buf[3]; - - u_char rdn_set_len_buf[3]; - - u_char dn_seq_len_buf[3]; + u_char oid_len_buf[ASN1_MAX_LEN_LEN]; + u_char name_len_buf[ASN1_MAX_LEN_LEN]; + u_char rdn_seq_len_buf[ASN1_MAX_LEN_LEN]; + u_char rdn_set_len_buf[ASN1_MAX_LEN_LEN]; + u_char dn_seq_len_buf[ASN1_MAX_LEN_LEN]; chunk_t asn1_oid_len = { oid_len_buf, 0 }; chunk_t asn1_name_len = { name_len_buf, 0 }; @@ -797,7 +797,7 @@ atodn(char *src, chunk_t *dn) err_t ugh = NULL; - - u_char *dn_ptr = dn->ptr + 4; + u_char *dn_ptr = dn->ptr + 1 + ASN1_MAX_LEN_LEN; /* leave room for prefix */ state_t state = SEARCH_OID; @@ -885,25 +885,37 @@ atodn(char *src, chunk_t *dn) code_asn1_length(rdn_set_len, &asn1_rdn_set_len); /* encode the relative distinguished name */ - - *dn_ptr++ = ASN1_SET; - - chunkcpy(dn_ptr, asn1_rdn_set_len); - - *dn_ptr++ = ASN1_SEQUENCE; - - chunkcpy(dn_ptr, asn1_rdn_seq_len); - - *dn_ptr++ = ASN1_OID; - - chunkcpy(dn_ptr, asn1_oid_len); - - chunkcpy(dn_ptr, x501rdns[pos].oid); - - /* encode the ASN.1 character string type of the name */ - - *dn_ptr++ = (x501rdns[pos].type == ASN1_PRINTABLESTRING - - && !is_printablestring(name))? ASN1_T61STRING : x501rdns[pos].type; - - chunkcpy(dn_ptr, asn1_name_len); - - chunkcpy(dn_ptr, name); - - - - /* accumulate the length of the distinguished name sequence */ - - dn_seq_len += 1 + asn1_rdn_set_len.len + rdn_set_len; - - - - /* reset name and change state */ - - name = empty_chunk; - - state = SEARCH_OID; + if (IDTOA_BUF < dn_ptr - dn->ptr + + 1 + asn1_rdn_set_len.len /* set */ + + 1 + asn1_rdn_seq_len.len /* sequence */ + + 1 + asn1_oid_len.len + x501rdns[pos].oid.len /* oid len, oid */ + + 1 + asn1_name_len.len + name.len /* type name */ + ) { + /* no room! */ + ugh = "DN is too big"; + state = UNKNOWN_OID; + /* I think that it is safe to continue (but perhaps pointless) */ + } else { + *dn_ptr++ = ASN1_SET; + chunkcpy(dn_ptr, asn1_rdn_set_len); + *dn_ptr++ = ASN1_SEQUENCE; + chunkcpy(dn_ptr, asn1_rdn_seq_len); + *dn_ptr++ = ASN1_OID; + chunkcpy(dn_ptr, asn1_oid_len); + chunkcpy(dn_ptr, x501rdns[pos].oid); + /* encode the ASN.1 character string type of the name */ + *dn_ptr++ = (x501rdns[pos].type == ASN1_PRINTABLESTRING + && !is_printablestring(name))? ASN1_T61STRING : x501rdns[pos].type; + chunkcpy(dn_ptr, asn1_name_len); + chunkcpy(dn_ptr, name); + + /* accumulate the length of the distinguished name sequence */ + dn_seq_len += 1 + asn1_rdn_set_len.len + rdn_set_len; + + /* reset name and change state */ + name = empty_chunk; + state = SEARCH_OID; + } } break; case UNKNOWN_OID: @@ -911,9 +923,9 @@ atodn(char *src, chunk_t *dn) } } while (*src++ != '\0'); - - /* complete the distinguished name sequence*/ - - code_asn1_length(dn_seq_len, &asn1_dn_seq_len); - - dn->ptr += 3 - asn1_dn_seq_len.len; + /* complete the distinguished name sequence: prefix it with ASN1_SEQUENCE and length */ + code_asn1_length((size_t)dn_seq_len, &asn1_dn_seq_len); + dn->ptr += ASN1_MAX_LEN_LEN + 1 - 1 - asn1_dn_seq_len.len; dn->len = 1 + asn1_dn_seq_len.len + dn_seq_len; dn_ptr = dn->ptr; *dn_ptr++ = ASN1_SEQUENCE; diff --git a/programs/pluto/connections.c b/programs/pluto/connections.c index e8d326b..f08521b 100644 - --- a/programs/pluto/connections.c +++ b/programs/pluto/connections.c @@ -911,7 +911,7 @@ extract_end(struct end *dst, const struct whack_end *src, const char *which) } else { - - err_t ugh = atoid(src->id, &dst->id, TRUE); + err_t ugh = atoid(src->id, &dst->id, TRUE, FALSE); if (ugh != NULL) { diff --git a/programs/pluto/dnskey.c b/programs/pluto/dnskey.c index 5525d12..78f1d0a 100644 - --- a/programs/pluto/dnskey.c +++ b/programs/pluto/dnskey.c @@ -277,8 +277,12 @@ decode_iii(char **pp, struct id *gw_id) if (*p == '@') { /* gateway specification in this record is @FQDN */ - - err_t ugh = atoid(p, gw_id, FALSE); + if(strspn(p,' ') >= IDTOA_BUF) { + return builddiag("malformed FQDN in TXT " our_TXT_attr_string ": ID too large for IDTOA_BUF"); + } + + err_t ugh = atoid(p, gw_id, FALSE, TRUE); /* only run OE related parts of atoid() */ if (ugh != NULL) return builddiag("malformed FQDN in TXT " our_TXT_attr_string ": %s" , ugh); diff --git a/programs/pluto/myid.c b/programs/pluto/myid.c index bdd0e12..2e92f25 100644 - --- a/programs/pluto/myid.c +++ b/programs/pluto/myid.c @@ -103,7 +103,7 @@ set_myid(enum myid_state s, char *idstr) if (idstr != NULL) { struct id id; - - err_t ugh = atoid(idstr, &id, FALSE); + err_t ugh = atoid(idstr, &id, FALSE, FALSE); if (ugh != NULL) { diff --git a/programs/pluto/rcv_whack.c b/programs/pluto/rcv_whack.c index 1725357..7d5072c 100644 - --- a/programs/pluto/rcv_whack.c +++ b/programs/pluto/rcv_whack.c @@ -259,7 +259,7 @@ static void key_add_request(const struct whack_message *msg) { struct id keyid; - - err_t ugh = atoid(msg->keyid, &keyid, FALSE); + err_t ugh = atoid(msg->keyid, &keyid, FALSE, FALSE); if (ugh != NULL) { diff --git a/programs/showhostkey/showhostkey.c b/programs/showhostkey/showhostkey.c index c9fe9cf..bf87080 100644 - --- a/programs/showhostkey/showhostkey.c +++ b/programs/showhostkey/showhostkey.c @@ -203,7 +203,7 @@ struct secret *pick_key(struct secret *host_secrets struct secret *s; err_t e; - - e = atoid(idname, &id, FALSE); + e = atoid(idname, &id, FALSE, FALSE); if(e) { printf("%s: key '%s' is invalid\n", progname, idname); exit(4); -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) iQIcBAEBCAAGBQJRkWmnAAoJEIX/S0OzD8b5EZIP+wb5LyvL4jXGYJzvalkCjWL3 1cZp5H672jGdVvW/G3bJ5unhjpRt9ASxebHR/4LfWZuWG5U4gdPRjcz1YcuNwVnB xOXZ4ELWYRFFblkkHz+GO5rSRwmWhFnyGvDdN5Oh6VBcmegHvaKk6uVLPXZJpVdg 2U1+s+x3EkrcP6IJyTa9pyhZiDWcdYVn3seyHcFCNa3R/Xkwefi3HwA2w8+L18NX NvIMUx2aXj70cBE5VAg+XJWIZ2Rrlf2zHDM96GUUfGIIH1mzpuxYCFbpGqISmOYI AAumQ9I4kQGy0ZkWn41Et3ppJvcRFoMlAz70Ay+nbZ/+eqQH9B3KfplfX2UrsXAn SVvMPypkMfjhUbPG8AWr//6+a0uZxa0PyibNXhhdr+3ocANaZ8ty+ehFmVl0DIBM rc582erQ8s4Bj8v+4vy1TzkR5HXWhwWhCjD0EnU8zGGjZ2u+1BAYgzTUG4Nqo+/Q ziJdc71vy+OqyLXTFMdekUuRl40BXuFHHUv6jWeslgIh2/1Z/A0NZzxs2sMFCkEW anTG32ridJSCqQhSXZ4xW07O5F45csH6qgze2jQdYEizATYsDqeKazEZhmakUsow v5gj85f5VYGWjoYjKr/HbrueEbeGpV3Twf4tZ6XyCxAjJEt6N8XWidSiMeL3gNIm cgXmYH+ak4nDLJGyaYDt =5y9o -----END PGP SIGNATURE-----