iked(8): move CERTREQ handler out of parser

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

iked(8): move CERTREQ handler out of parser

Tobias Heider-2
Hi,

currently some parts of the iked(8) payload parser act on the SA structure
before the message was fully parsed and checked, which is obviously bad for
security.
The parser should store it's results in a temporary location
(e.G. in the iked_message struct) and apply changes only when the message
has passed all checks.

This diff moves the CERTREQ payload handling from the parser to the IKE_AUTH
exchange handler function where it belongs.

Ok?

Index: iked.h
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.121
diff -u -p -u -r1.121 iked.h
--- iked.h 11 May 2019 16:30:23 -0000 1.121
+++ iked.h 9 Aug 2019 13:41:00 -0000
@@ -536,6 +536,9 @@ struct iked_message {
  struct iked_id msg_cert;
  struct ibuf *msg_cookie;
 
+ uint8_t msg_certreq_type; /* CR Encoding*/
+ struct ibuf *msg_certreq_auth; /* CR Authority */
+
  /* MOBIKE */
  int msg_update_sa_addresses;
  struct ibuf *msg_cookie2;
Index: ikev2.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.171
diff -u -p -u -r1.171 ikev2.c
--- ikev2.c 11 May 2019 16:30:23 -0000 1.171
+++ ikev2.c 9 Aug 2019 13:59:53 -0000
@@ -82,6 +82,8 @@ int ikev2_send_error(struct iked *, str
     struct iked_message *, uint8_t);
 int ikev2_send_init_error(struct iked *, struct iked_message *);
 
+int ikev2_handle_certreq(struct iked*, struct iked_message *);
+
 int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
     struct iked_spi *, uint8_t);
 int ikev2_ikesa_enable(struct iked *, struct iked_sa *, struct iked_sa *);
@@ -608,6 +610,9 @@ ikev2_ike_auth_recv(struct iked *env, st
  struct iked_policy *policy = sa->sa_policy;
  int ret = -1;
 
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return (-1);
+
  if (sa->sa_hdr.sh_initiator) {
  id = &sa->sa_rid;
  certid = &sa->sa_rcert;
@@ -2996,6 +3001,40 @@ ikev2_set_sa_proposal(struct iked_sa *sa
  return (-1);
  }
  }
+ return (0);
+}
+
+int
+ikev2_handle_certreq(struct iked* env, struct iked_message *msg)
+{
+ struct iked_sa *sa;
+
+ if (msg->msg_certreq_type == IKEV2_CERT_NONE)
+ return (0);
+
+ log_info("%s: handling certreq with type %d.", __func__,
+    msg->msg_certreq_type);
+
+ if ((sa = msg->msg_sa) == NULL)
+ return (-1);
+
+ /* Optional certreq for PSK */
+ if (sa->sa_hdr.sh_initiator)
+ sa->sa_stateinit |= IKED_REQ_CERT;
+ else
+ sa->sa_statevalid |= IKED_REQ_CERT;
+
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid,
+    msg->msg_certreq_type,
+    ibuf_data(msg->msg_certreq_auth),
+    ibuf_length(msg->msg_certreq_auth),
+    PROC_CERT);
+
+ /* Cleanup */
+ ibuf_release(msg->msg_certreq_auth);
+ msg->msg_certreq_auth = NULL;
+ msg->msg_certreq_type = IKEV2_CERT_NONE;
+
  return (0);
 }
 
Index: ikev2_pld.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.71
diff -u -p -u -r1.71 ikev2_pld.c
--- ikev2_pld.c 11 May 2019 16:30:23 -0000 1.71
+++ ikev2_pld.c 9 Aug 2019 13:44:07 -0000
@@ -826,7 +826,6 @@ int
 ikev2_pld_certreq(struct iked *env, struct ikev2_payload *pld,
     struct iked_message *msg, size_t offset, size_t left)
 {
- struct iked_sa *sa = msg->msg_sa;
  struct ikev2_cert cert;
  uint8_t *buf;
  ssize_t len;
@@ -847,27 +846,30 @@ ikev2_pld_certreq(struct iked *env, stru
  if (!ikev2_msg_frompeer(msg))
  return (0);
 
- if (cert.cert_type == IKEV2_CERT_X509_CERT) {
- if (!len)
- return (0);
- if ((len % SHA_DIGEST_LENGTH) != 0) {
- log_debug("%s: invalid certificate request", __func__);
- return (-1);
- }
+ /* The contents of the Certification Authority field are defined only
+ * for X.509 certificates */
+ switch (cert.cert_type) {
+ case IKEV2_CERT_X509_CERT:
+ case IKEV2_CERT_HASHURL_X509:
+ case IKEV2_CERT_HASHURL_X509_BUNDLE:
+ if (len == 0)
+ return (0);
+ if ((len % SHA_DIGEST_LENGTH) != 0) {
+ log_debug("%s: invalid certificate request",
+    __func__);
+ return (-1);
+ }
+ if ((msg->msg_parent->msg_certreq_auth =
+    ibuf_new(buf, len)) == NULL)
+ return (-1);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
+ default:
+ if (len != 0)
+ return (0);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
  }
-
- if (msg->msg_sa == NULL)
- return (-1);
-
- /* Optional certreq for PSK */
- if (sa->sa_hdr.sh_initiator)
- sa->sa_stateinit |= IKED_REQ_CERT;
- else
- sa->sa_statevalid |= IKED_REQ_CERT;
-
- ca_setreq(env, sa, &sa->sa_policy->pol_localid,
-    cert.cert_type, buf, len, PROC_CERT);
-
  return (0);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): move CERTREQ handler out of parser

Tobias Heider-2
Update because I forgot to add OCSP support which needs a bit of extra
treatment as it uses an additional CERTREQ payload.

Index: iked.h
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.121
diff -u -p -u -r1.121 iked.h
--- iked.h 11 May 2019 16:30:23 -0000 1.121
+++ iked.h 9 Aug 2019 14:20:27 -0000
@@ -536,6 +536,10 @@ struct iked_message {
  struct iked_id msg_cert;
  struct ibuf *msg_cookie;
 
+ uint8_t msg_certreq_type; /* CR Encoding*/
+ struct ibuf *msg_certreq_auth; /* CR Authority */
+ struct ibuf *msg_certreq_ocsp; /* CR OCSP values */
+
  /* MOBIKE */
  int msg_update_sa_addresses;
  struct ibuf *msg_cookie2;
Index: ikev2.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.171
diff -u -p -u -r1.171 ikev2.c
--- ikev2.c 11 May 2019 16:30:23 -0000 1.171
+++ ikev2.c 9 Aug 2019 14:23:15 -0000
@@ -82,6 +82,8 @@ int ikev2_send_error(struct iked *, str
     struct iked_message *, uint8_t);
 int ikev2_send_init_error(struct iked *, struct iked_message *);
 
+int ikev2_handle_certreq(struct iked*, struct iked_message *);
+
 int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
     struct iked_spi *, uint8_t);
 int ikev2_ikesa_enable(struct iked *, struct iked_sa *, struct iked_sa *);
@@ -608,6 +610,9 @@ ikev2_ike_auth_recv(struct iked *env, st
  struct iked_policy *policy = sa->sa_policy;
  int ret = -1;
 
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return (-1);
+
  if (sa->sa_hdr.sh_initiator) {
  id = &sa->sa_rid;
  certid = &sa->sa_rcert;
@@ -2996,6 +3001,52 @@ ikev2_set_sa_proposal(struct iked_sa *sa
  return (-1);
  }
  }
+ return (0);
+}
+
+int
+ikev2_handle_certreq(struct iked* env, struct iked_message *msg)
+{
+ struct iked_sa *sa;
+
+ if (msg->msg_certreq_type == IKEV2_CERT_NONE)
+ return (0);
+
+ if ((sa = msg->msg_sa) == NULL)
+ return (-1);
+
+ if (msg->msg_certreq_type != IKEV2_CERT_NONE) {
+ log_debug("%s: handling certreq with type %d.", __func__,
+    msg->msg_certreq_type);
+
+ /* Optional certreq for PSK */
+ if (sa->sa_hdr.sh_initiator)
+ sa->sa_stateinit |= IKED_REQ_CERT;
+ else
+ sa->sa_statevalid |= IKED_REQ_CERT;
+
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid,
+    msg->msg_certreq_type,
+    ibuf_data(msg->msg_certreq_auth),
+    ibuf_length(msg->msg_certreq_auth),
+    PROC_CERT);
+ }
+
+ if (msg->msg_certreq_ocsp != NULL) {
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid,
+    IKEV2_CERT_OCSP,
+    ibuf_data(msg->msg_certreq_auth),
+    ibuf_length(msg->msg_certreq_auth),
+    PROC_CERT);
+ }
+
+ /* Cleanup */
+ ibuf_release(msg->msg_certreq_auth);
+ msg->msg_certreq_auth = NULL;
+ ibuf_release(msg->msg_certreq_ocsp);
+ msg->msg_certreq_ocsp = NULL;
+ msg->msg_certreq_type = IKEV2_CERT_NONE;
+
  return (0);
 }
 
Index: ikev2_pld.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.71
diff -u -p -u -r1.71 ikev2_pld.c
--- ikev2_pld.c 11 May 2019 16:30:23 -0000 1.71
+++ ikev2_pld.c 9 Aug 2019 14:24:15 -0000
@@ -826,7 +826,6 @@ int
 ikev2_pld_certreq(struct iked *env, struct ikev2_payload *pld,
     struct iked_message *msg, size_t offset, size_t left)
 {
- struct iked_sa *sa = msg->msg_sa;
  struct ikev2_cert cert;
  uint8_t *buf;
  ssize_t len;
@@ -847,27 +846,39 @@ ikev2_pld_certreq(struct iked *env, stru
  if (!ikev2_msg_frompeer(msg))
  return (0);
 
- if (cert.cert_type == IKEV2_CERT_X509_CERT) {
- if (!len)
- return (0);
- if ((len % SHA_DIGEST_LENGTH) != 0) {
- log_debug("%s: invalid certificate request", __func__);
- return (-1);
- }
+ /* The contents of the Certification Authority field are defined only
+ * for X.509 certificates */
+ switch (cert.cert_type) {
+ case IKEV2_CERT_X509_CERT:
+ case IKEV2_CERT_HASHURL_X509:
+ case IKEV2_CERT_HASHURL_X509_BUNDLE:
+ if (len == 0)
+ return (0);
+ if ((len % SHA_DIGEST_LENGTH) != 0) {
+ log_debug("%s: invalid certificate request",
+    __func__);
+ return (-1);
+ }
+ if ((msg->msg_parent->msg_certreq_auth =
+    ibuf_new(buf, len)) == NULL)
+ return (-1);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
+ case IKEV2_CERT_OCSP:
+ if (len == 0)
+ return (0);
+
+ /* OCSP is treated seperately from common CR payloads */
+ if ((msg->msg_parent->msg_certreq_ocsp =
+    ibuf_new(buf, len)) == NULL)
+ return (-1);
+ break;
+ default:
+ if (len != 0)
+ return (0);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
  }
-
- if (msg->msg_sa == NULL)
- return (-1);
-
- /* Optional certreq for PSK */
- if (sa->sa_hdr.sh_initiator)
- sa->sa_stateinit |= IKED_REQ_CERT;
- else
- sa->sa_statevalid |= IKED_REQ_CERT;
-
- ca_setreq(env, sa, &sa->sa_policy->pol_localid,
-    cert.cert_type, buf, len, PROC_CERT);
-
  return (0);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: iked(8): move CERTREQ handler out of parser

Tobias Heider-2
One more update: I moved to ikev2_handle_certreq() calls somewhere else where
they fit better.

I'd be happy to get some feedback from different iked setups,
in my tests all seems to work as intended. In the best case you
won't notice any difference at all (only the CERTREQ handling will
happen a bit later in recv function).

Here is the diff:

Index: iked.h
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.121
diff -u -p -u -r1.121 iked.h
--- iked.h 11 May 2019 16:30:23 -0000 1.121
+++ iked.h 12 Aug 2019 15:19:56 -0000
@@ -536,6 +536,10 @@ struct iked_message {
  struct iked_id msg_cert;
  struct ibuf *msg_cookie;
 
+ uint8_t msg_certreq_type; /* CR Encoding*/
+ struct ibuf *msg_certreq_auth; /* CR Authority */
+ struct ibuf *msg_certreq_ocsp; /* CR OCSP values */
+
  /* MOBIKE */
  int msg_update_sa_addresses;
  struct ibuf *msg_cookie2;
Index: ikev2.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.171
diff -u -p -u -r1.171 ikev2.c
--- ikev2.c 11 May 2019 16:30:23 -0000 1.171
+++ ikev2.c 12 Aug 2019 16:03:46 -0000
@@ -82,6 +82,8 @@ int ikev2_send_error(struct iked *, str
     struct iked_message *, uint8_t);
 int ikev2_send_init_error(struct iked *, struct iked_message *);
 
+int ikev2_handle_certreq(struct iked*, struct iked_message *);
+
 int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
     struct iked_spi *, uint8_t);
 int ikev2_ikesa_enable(struct iked *, struct iked_sa *, struct iked_sa *);
@@ -874,6 +876,9 @@ ikev2_init_recv(struct iked *env, struct
     "IKE_SA_INIT exchange", __func__);
  break;
  }
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return;
+
  (void)ikev2_init_auth(env, msg);
  break;
  case IKEV2_EXCHANGE_IKE_AUTH:
@@ -2384,6 +2389,9 @@ ikev2_resp_recv(struct iked *env, struct
     sa->sa_policy->pol_auth.auth_eap)
  sa_state(env, sa, IKEV2_STATE_EAP);
 
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return;
+
  if (ikev2_ike_auth_recv(env, sa, msg) != 0) {
  log_debug("%s: failed to send auth response", __func__);
  ikev2_send_error(env, sa, msg, hdr->ike_exchange);
@@ -2996,6 +3004,46 @@ ikev2_set_sa_proposal(struct iked_sa *sa
  return (-1);
  }
  }
+ return (0);
+}
+
+int
+ikev2_handle_certreq(struct iked* env, struct iked_message *msg)
+{
+ struct iked_sa *sa;
+
+ if ((sa = msg->msg_sa) == NULL)
+ return (-1);
+
+ if (msg->msg_certreq_type != IKEV2_CERT_NONE) {
+ /* Optional certreq for PSK */
+ if (sa->sa_hdr.sh_initiator)
+ sa->sa_stateinit |= IKED_REQ_CERT;
+ else
+ sa->sa_statevalid |= IKED_REQ_CERT;
+
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid,
+    msg->msg_certreq_type,
+    ibuf_data(msg->msg_certreq_auth),
+    ibuf_length(msg->msg_certreq_auth),
+    PROC_CERT);
+ }
+
+ if (msg->msg_certreq_ocsp != NULL) {
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid,
+    IKEV2_CERT_OCSP,
+    ibuf_data(msg->msg_certreq_auth),
+    ibuf_length(msg->msg_certreq_auth),
+    PROC_CERT);
+ }
+
+ /* Cleanup */
+ ibuf_release(msg->msg_certreq_auth);
+ msg->msg_certreq_auth = NULL;
+ ibuf_release(msg->msg_certreq_ocsp);
+ msg->msg_certreq_ocsp = NULL;
+ msg->msg_certreq_type = IKEV2_CERT_NONE;
+
  return (0);
 }
 
Index: ikev2_pld.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.71
diff -u -p -u -r1.71 ikev2_pld.c
--- ikev2_pld.c 11 May 2019 16:30:23 -0000 1.71
+++ ikev2_pld.c 12 Aug 2019 15:52:02 -0000
@@ -826,7 +826,6 @@ int
 ikev2_pld_certreq(struct iked *env, struct ikev2_payload *pld,
     struct iked_message *msg, size_t offset, size_t left)
 {
- struct iked_sa *sa = msg->msg_sa;
  struct ikev2_cert cert;
  uint8_t *buf;
  ssize_t len;
@@ -847,27 +846,39 @@ ikev2_pld_certreq(struct iked *env, stru
  if (!ikev2_msg_frompeer(msg))
  return (0);
 
- if (cert.cert_type == IKEV2_CERT_X509_CERT) {
- if (!len)
- return (0);
- if ((len % SHA_DIGEST_LENGTH) != 0) {
- log_debug("%s: invalid certificate request", __func__);
- return (-1);
- }
+ /* The contents of the Certification Authority field are defined only
+ * for X.509 certificates */
+ switch (cert.cert_type) {
+ case IKEV2_CERT_X509_CERT:
+ case IKEV2_CERT_HASHURL_X509:
+ case IKEV2_CERT_HASHURL_X509_BUNDLE:
+ if (len == 0)
+ return (0);
+ if ((len % SHA_DIGEST_LENGTH) != 0) {
+ log_debug("%s: invalid certificate request",
+    __func__);
+ return (-1);
+ }
+ if ((msg->msg_parent->msg_certreq_auth =
+    ibuf_new(buf, len)) == NULL)
+ return (-1);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
+ case IKEV2_CERT_OCSP:
+ if (len == 0)
+ return (0);
+
+ /* OCSP is treated seperately from common CR payloads */
+ if ((msg->msg_parent->msg_certreq_ocsp =
+    ibuf_new(buf, len)) == NULL)
+ return (-1);
+ break;
+ default:
+ if (len != 0)
+ return (0);
+ msg->msg_parent->msg_certreq_type = cert.cert_type;
+ break;
  }
-
- if (msg->msg_sa == NULL)
- return (-1);
-
- /* Optional certreq for PSK */
- if (sa->sa_hdr.sh_initiator)
- sa->sa_stateinit |= IKED_REQ_CERT;
- else
- sa->sa_statevalid |= IKED_REQ_CERT;
-
- ca_setreq(env, sa, &sa->sa_policy->pol_localid,
-    cert.cert_type, buf, len, PROC_CERT);
-
  return (0);
 }