SEO

Short version:

PLEASE TRY RUNNING WITH THIS DIFF.

If it stops your current setup working, please let me and the lists know
as soon as possible. If things continue to work, just send an email
direct to me - I would like to get this put in for 4.2.

This diff fixes a long-standing interoperability issue between OpenBSD
isakmpd and Cisco IOS (and possibly others). There is a small
possibility that it may break existing configurations, and it is that
which we would like to rule out.

I'm looking for configurations that are OpenBSD-to-OpenBSD, as well as
OpenBSD-to-non-OpenBSD. Testing of different versions, as well as
patched-to-unpatched, gets extra bonus points.


Long version:

When performing Phase 1 key exchange using RSA signature authentication,
an OpenBSD isakmpd always sends the certificate of its claimed
identity. We assume that the remote end will do the same: we don't
send a CERT_REQUEST message.

This works fine when we're talking to OpenBSD and most other systems,
but Cisco IOS will not send its certificate unless it first receives a
CERT_REQUEST message.

(There's an interoperability test that hit this issue at
http://www.hsc.fr/ressources/ipsec/i.../index.html.en, and a
discussion about CERT_REQUEST payloads at
http://www.sandelman.ca/ipsec/2000/01/msg00118.html, for those who
are interested.)

Now, the CERT_REQUEST message gets sent before we have authenticated
the peer (of course!). So we're giving out information about which
CA(s) we trust to all and sundry, possibly allowing them to target a
weak CA.

So what this diff does it to *only* send a CERT_REQUEST if the peer
first sends us one (i.e. this is for IOS as initiator and OpenBSD as
responder). And all we do is reflect back the CA that came in the
inbound CERT_REQUEST. (Not nice, but it avoids the information leak.
And since we don't know the ID that the initiator will claim at this
point, we either have to send all CAs that we will accept or randomly
pick one. This seems to be the best compromise.)

Further, we only do this if we do actually have at least one CA
certificate in our CA-directory (usually /etc/isakmpd/ca/). And this
is for working with ipsec.conf, rather than keynote.

Anyway, many people have helped me with this diff, including cloder@,
hshoexer@ and mpf@. And Stuart Henderson helped with some of the
testing earlier in the year.

All this for three pairs of messages!

Thanks

Tom


Index: cert.c
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/cert.c,v
retrieving revision 1.31
diff -u -p -r1.31 cert.c
--- cert.c 8 Apr 2005 22:32:09 -0000 1.31
+++ cert.c 31 Jul 2007 19:56:06 -0000
@@ -49,7 +49,8 @@ struct cert_handler cert_handler[] = {
x509_cert_insert, x509_cert_free,
x509_certreq_validate, x509_certreq_decode, x509_free_aca,
x509_cert_obtain, x509_cert_get_key, x509_cert_get_subjects,
- x509_cert_dup, x509_serialize, x509_printable, x509_from_printable
+ x509_cert_dup, x509_serialize, x509_printable, x509_from_printable,
+ x509_ca_count
},
{
ISAKMP_CERTENC_KEYNOTE,
@@ -58,7 +59,7 @@ struct cert_handler cert_handler[] = {
keynote_certreq_validate, keynote_certreq_decode, keynote_free_aca,
keynote_cert_obtain, keynote_cert_get_key, keynote_cert_get_subjects,
keynote_cert_dup, keynote_serialize, keynote_printable,
- keynote_from_printable
+ keynote_from_printable, keynote_ca_count
},
};

@@ -118,18 +119,32 @@ certreq_decode(u_int16_t type, u_int8_t

aca.id = type;
aca.handler = handler;
+ aca.data = aca.raw_ca = NULL;

if (datalen > 0) {
- aca.data = handler->certreq_decode(data, datalen);
- if (!aca.data)
+ int rc;
+
+ rc = handler->certreq_decode(&aca.data, data, datalen);
+ if (!rc)
+ return 0;
+
+ aca.raw_ca = malloc(datalen);
+ if (aca.raw_ca == NULL) {
+ log_error("certreq_decode: malloc (%lu) failed",
+ (unsigned long)datalen);
+ handler->free_aca(aca.data);
return 0;
- } else
- aca.data = 0;
+ }
+
+ memcpy(aca.raw_ca, data, datalen);
+ }
+ aca.raw_ca_len = datalen;

ret = malloc(sizeof aca);
if (!ret) {
log_error("certreq_decode: malloc (%lu) failed",
(unsigned long)sizeof aca);
+ free(aca.raw_ca);
handler->free_aca(aca.data);
return 0;
}
Index: cert.h
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/cert.h,v
retrieving revision 1.14
diff -u -p -r1.14 cert.h
--- cert.h 14 May 2004 08:42:56 -0000 1.14
+++ cert.h 31 Jul 2007 19:56:06 -0000
@@ -52,6 +52,7 @@
* cert_printable - for X509, the hex representation of the serialized form;
* for KeyNote, itself.
* cert_from_printable - the reverse of cert_printable
+ * ca_count - how many CAs we have in our store (for CERT_REQ processing)
*/

struct cert_handler {
@@ -63,7 +64,7 @@ struct cert_handler {
int (*cert_insert)(int, void *);
void (*cert_free)(void *);
int (*certreq_validate)(u_int8_t *, u_int32_t);
- void *(*certreq_decode)(u_int8_t *, u_int32_t);
+ int (*certreq_decode)(void **, u_int8_t *, u_int32_t);
void (*free_aca)(void *);
int (*cert_obtain)(u_int8_t *, size_t, void *, u_int8_t **,
u_int32_t *);
@@ -74,6 +75,7 @@ struct cert_handler {
void (*cert_serialize) (void *, u_int8_t **, u_int32_t *);
char *(*cert_printable) (void *);
void *(*cert_from_printable) (char *);
+ int (*ca_count)(void);
};

/* The acceptable authority of cert request. */
@@ -85,6 +87,10 @@ struct certreq_aca {

/* If data is a null pointer, everything is acceptable. */
void *data;
+
+ /* Copy of raw CA value received */
+ u_int32_t raw_ca_len;
+ void *raw_ca;
};

struct certreq_aca *certreq_decode(u_int16_t, u_int8_t *, u_int32_t);
Index: exchange.c
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/exchange.c,v
retrieving revision 1.130
diff -u -p -r1.130 exchange.c
--- exchange.c 16 Apr 2007 13:01:39 -0000 1.130
+++ exchange.c 31 Jul 2007 19:56:07 -0000
@@ -1593,6 +1593,8 @@ exchange_free_aca_list(struct exchange *

for (aca = TAILQ_FIRST(&exchange->aca_list); aca;
aca = TAILQ_FIRST(&exchange->aca_list)) {
+ if (aca->raw_ca)
+ free(aca->raw_ca);
if (aca->data) {
if (aca->handler)
aca->handler->free_aca(aca->data);
@@ -1601,6 +1603,58 @@ exchange_free_aca_list(struct exchange *
TAILQ_REMOVE(&exchange->aca_list, aca, link);
free(aca);
}
+}
+
+/* Add any CERTREQs we should send. */
+int
+exchange_add_certreqs(struct message *msg)
+{
+ struct exchange *exchange = msg->exchange;
+ struct certreq_aca *aca;
+ u_int8_t *buf;
+
+ /*
+ * Some peers (e.g. Cisco IOS) won't send their cert unless we
+ * specifically ask beforehand with CERTREQ. We reflect any
+ * CERTREQs we receive from the initiator in order to do this.
+ * This avoids leaking information about which CAs we trust,
+ * and works in the most common case where both ends trust the
+ * same CA.
+ */
+ for (aca = TAILQ_FIRST(&exchange->aca_list); aca;
+ aca = TAILQ_NEXT(aca, link)) {
+
+ /* But only do this if we have at least one CA */
+ if (aca->handler != NULL && aca->handler->ca_count() == 0) {
+ LOG_DBG((LOG_EXCHANGE, 10,
+ "exchange_add_certreqs: no CA, so not "
+ "sending a CERTREQ"));
+ continue;
+ }
+
+ if (aca->raw_ca_len) {
+ buf = malloc(ISAKMP_CERTREQ_SZ + aca->raw_ca_len);
+ if (buf == NULL) {
+ log_error("exchange_add_certreqs: "
+ "malloc (%lu) failed",
+ ISAKMP_CERTREQ_SZ +
+ (unsigned long)aca->raw_ca_len);
+ return -1;
+ }
+
+ buf[ISAKMP_CERTREQ_TYPE_OFF] = aca->id;
+ memcpy(buf + ISAKMP_CERTREQ_AUTHORITY_OFF,
+ aca->raw_ca, aca->raw_ca_len);
+
+ if (message_add_payload(msg, ISAKMP_PAYLOAD_CERT_REQ,
+ buf, ISAKMP_CERTREQ_SZ + aca->raw_ca_len, 1)) {
+ free(buf);
+ return -1;
+ }
+ }
+ }
+
+ return 0;
}

/* Obtain certificates from acceptable certification authority. */
Index: exchange.h
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/exchange.h,v
retrieving revision 1.32
diff -u -p -r1.32 exchange.h
--- exchange.h 2 Jul 2006 13:19:00 -0000 1.32
+++ exchange.h 31 Jul 2007 19:56:08 -0000
@@ -224,6 +224,7 @@ struct exchange {
#define EXCHANGE_FLAG_OPENBSD 0x0200 /* Peer is OpenBSD */

extern int exchange_add_certs(struct message *);
+extern int exchange_add_certreqs(struct message *);
extern void exchange_finalize(struct message *);
extern void exchange_free(struct exchange *);
extern void exchange_free_aca_list(struct exchange *);
Index: ike_phase_1.c
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/ike_phase_1.c,v
retrieving revision 1.69
diff -u -p -r1.69 ike_phase_1.c
--- ike_phase_1.c 7 May 2007 18:19:56 -0000 1.69
+++ ike_phase_1.c 31 Jul 2007 19:56:09 -0000
@@ -548,6 +548,11 @@ ike_phase_1_send_KE_NONCE(struct message
/* XXX Log? */
return -1;
}
+ /* Are there any CERTREQs to send? */
+ if (exchange_add_certreqs(msg)) {
+ /* XXX Log? */
+ return -1;
+ }
/* Try to add certificates which are acceptable for the CERTREQs */
if (exchange_add_certs(msg)) {
/* XXX Log? */
Index: policy.c
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/policy.c,v
retrieving revision 1.90
diff -u -p -r1.90 policy.c
--- policy.c 16 Apr 2007 13:01:39 -0000 1.90
+++ policy.c 31 Jul 2007 19:56:11 -0000
@@ -2096,11 +2096,11 @@ keynote_certreq_validate(u_int8_t *data,
}

/* Beats me what we should be doing with this. */
-void *
-keynote_certreq_decode(u_int8_t *data, u_int32_t len)
+int
+keynote_certreq_decode(void **pdata, u_int8_t *data, u_int32_t len)
{
/* XXX */
- return NULL;
+ return 0;
}

void
@@ -2301,4 +2301,11 @@ void *
keynote_from_printable(char *cert)
{
return strdup(cert);
+}
+
+/* Number of CAs we trust (currently this is x509 only) */
+int
+keynote_ca_count(void)
+{
+ return 0;
}
Index: policy.h
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/policy.h,v
retrieving revision 1.16
diff -u -p -r1.16 policy.h
--- policy.h 5 Apr 2005 22:53:50 -0000 1.16
+++ policy.h 31 Jul 2007 19:56:13 -0000
@@ -54,7 +54,7 @@ extern int keynote_cert_validate(vo
extern int keynote_cert_insert(int, void *);
extern void keynote_cert_free(void *);
extern int keynote_certreq_validate(u_int8_t *, u_int32_t);
-extern void *keynote_certreq_decode(u_int8_t *, u_int32_t);
+extern int keynote_certreq_decode(void **, u_int8_t *, u_int32_t);
extern void keynote_free_aca(void *);
extern int keynote_cert_obtain(u_int8_t *, size_t, void *,
u_int8_t **, u_int32_t *);
@@ -65,4 +65,5 @@ extern void *keynote_cert_dup(void *)
extern void keynote_serialize(void *, u_int8_t **, u_int32_t *);
extern char *keynote_printable(void *);
extern void *keynote_from_printable(char *);
+extern int keynote_ca_count(void);
#endif /* _POLICY_H_ */
Index: x509.c
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/x509.c,v
retrieving revision 1.109
diff -u -p -r1.109 x509.c
--- x509.c 16 Apr 2007 13:01:39 -0000 1.109
+++ x509.c 31 Jul 2007 19:56:13 -0000
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509.c,v 1.109 2007/04/16 13:01:39 moritz Exp $ */
+/* $OpenBSD: x509.c,v 1.108 2007/03/03 20:03:03 tom Exp $ */
/* $EOM: x509.c,v 1.54 2001/01/16 18:42:16 ho Exp $ */

/*
@@ -78,6 +78,8 @@ static int x509_hash_enter(X509 *);
static X509_STORE *x509_certs = 0;
static X509_STORE *x509_cas = 0;

+static int n_x509_cas = 0;
+
/* Initial number of bits used as hash. */
#define INITIAL_BUCKET_BITS 6

@@ -571,7 +573,7 @@ x509_hash_enter(X509 *cert)
/* X509 Certificate Handling functions. */

int
-x509_read_from_dir(X509_STORE *ctx, char *name, int hash)
+x509_read_from_dir(X509_STORE *ctx, char *name, int hash, int *pcount)
{
FILE *certfp;
X509 *cert;
@@ -628,6 +630,10 @@ x509_read_from_dir(X509_STORE *ctx, char
"failed for %s", file);
continue;
}
+
+ if (pcount != NULL)
+ (*pcount)++;
+
if (!X509_STORE_add_cert(ctx, cert)) {
/*
* This is actually expected if we have several
@@ -752,7 +758,7 @@ x509_cert_init(void)
log_print("x509_cert_init: creating new X509_STORE failed");
return 0;
}
- if (!x509_read_from_dir(x509_cas, dirname, 0)) {
+ if (!x509_read_from_dir(x509_cas, dirname, 0, &n_x509_cas)) {
log_print("x509_cert_init: x509_read_from_dir failed");
return 0;
}
@@ -771,7 +777,7 @@ x509_cert_init(void)
log_print("x509_cert_init: creating new X509_STORE failed");
return 0;
}
- if (!x509_read_from_dir(x509_certs, dirname, 1)) {
+ if (!x509_read_from_dir(x509_certs, dirname, 1, NULL)) {
log_print("x509_cert_init: x509_read_from_dir failed");
return 0;
}
@@ -949,8 +955,8 @@ x509_certreq_validate(u_int8_t *asn, u_i
}

/* Decode the BER Encoding of a RDNSequence in the CERT_REQ payload. */
-void *
-x509_certreq_decode(u_int8_t *asn, u_int32_t len)
+int
+x509_certreq_decode(void **pdata, u_int8_t *asn, u_int32_t len)
{
#if 0
/* XXX This needs to be done later. */
@@ -993,7 +999,7 @@ x509_certreq_decode(u_int8_t *asn, u_int
fail:
asn_free(&aca);
#endif
- return 0;
+ return 1;
}

void
@@ -1001,11 +1007,13 @@ x509_free_aca(void *blob)
{
struct x509_aca *aca = blob;

- free(aca->name1.type);
- free(aca->name1.val);
+ if (aca != NULL) {
+ free(aca->name1.type);
+ free(aca->name1.val);

- free(aca->name2.type);
- free(aca->name2.val);
+ free(aca->name2.type);
+ free(aca->name2.val);
+ }
}

X509 *
@@ -1338,3 +1346,9 @@ x509_DN_string(u_int8_t *asn1, size_t sz
return strdup(buf);
}

+/* Number of CAs we trust (to decide whether we can send CERT_REQ) */
+int
+x509_ca_count(void)
+{
+ return n_x509_cas;
+}
Index: x509.h
================================================== =================
RCS file: /cvs/src/sbin/isakmpd/x509.h,v
retrieving revision 1.21
diff -u -p -r1.21 x509.h
--- x509.h 23 May 2004 18:17:56 -0000 1.21
+++ x509.h 31 Jul 2007 19:56:13 -0000
@@ -61,7 +61,7 @@ struct X509_STORE;
/* Functions provided by cert handler. */

int x509_certreq_validate(u_int8_t *, u_int32_t);
-void *x509_certreq_decode(u_int8_t *, u_int32_t);
+int x509_certreq_decode(void **, u_int8_t *, u_int32_t);
void x509_cert_free(void *);
void *x509_cert_get(u_int8_t *, u_int32_t);
int x509_cert_get_key(void *, void *);
@@ -76,6 +76,7 @@ void *x509_cert_dup(void *);
void x509_serialize(void *, u_int8_t **, u_int32_t *);
char *x509_printable(void *);
void *x509_from_printable(char *);
+int x509_ca_count(void);

/* Misc. X509 certificate functions. */

@@ -84,7 +85,7 @@ int x509_cert_insert(int, vo
int x509_cert_subjectaltname(X509 * cert, u_char **, u_int *);
X509 *x509_from_asn(u_char *, u_int);
int x509_generate_kn(int, X509 *);
-int x509_read_from_dir(X509_STORE *, char *, int);
+int x509_read_from_dir(X509_STORE *, char *, int, int *);
int x509_read_crls_from_dir(X509_STORE *, char *);

#endif /* _X509_H_ */