ikev2: Destroy IKE_SA when receiving invalid authenticated requests

RFC 7296, section 2.21.3:

   If a peer parsing a request notices that it is badly formatted (after
   it has passed the message authentication code checks and window
   checks) and it returns an INVALID_SYNTAX notification, then this
   error notification is considered fatal in both peers, meaning that
   the IKE SA is deleted without needing an explicit Delete payload.

diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c
index ee3422c068e16cba22b57702aaeeb423f536caf7..0b223d39110f9f737f22382ef8981bd135d2dfaa 100644 (file)
--- a/src/libcharon/sa/ikev2/task_manager_v2.c
+++ b/src/libcharon/sa/ikev2/task_manager_v2.c
@@ -1324,17 +1324,36 @@ static void send_notify_response(private_task_manager_t *this,
        response->destroy(response);
 }
 
+/**
+ * Send an INVALID_SYNTAX notify and destroy the IKE_SA for authenticated
+ * messages.
+ */
+static status_t send_invalid_syntax(private_task_manager_t *this,
+                                                                       message_t *msg)
+{
+       send_notify_response(this, msg, INVALID_SYNTAX, chunk_empty);
+       incr_mid(this, FALSE);
+
+       /* IKE_SA_INIT is currently the only type the parser accepts unprotected,
+        * don't destroy the IKE_SA if such a message is invalid */
+       if (msg->get_exchange_type(msg) == IKE_SA_INIT)
+       {
+               return FAILED;
+       }
+       return DESTROY_ME;
+}
+
 /**
  * Parse the given message and verify that it is valid.
  */
 static status_t parse_message(private_task_manager_t *this, message_t *msg)
 {
-       status_t status;
+       status_t parse_status, status;
        uint8_t type = 0;
 
-       status = msg->parse_body(msg, this->ike_sa->get_keymat(this->ike_sa));
+       parse_status = msg->parse_body(msg, this->ike_sa->get_keymat(this->ike_sa));
 
-       if (status == SUCCESS)
+       if (parse_status == SUCCESS)
        {       /* check for unsupported critical payloads */
                enumerator_t *enumerator;
                unknown_payload_t *unknown;
@@ -1350,8 +1369,8 @@ static status_t parse_message(private_task_manager_t *this, message_t *msg)
                                {
                                        type = unknown->get_type(unknown);
                                        DBG1(DBG_ENC, "payload type %N is not supported, "
-                                                "but its critical!", payload_type_names, type);
-                                       status = NOT_SUPPORTED;
+                                                "but payload is critical!", payload_type_names, type);
+                                       parse_status = NOT_SUPPORTED;
                                        break;
                                }
                        }
@@ -1359,11 +1378,13 @@ static status_t parse_message(private_task_manager_t *this, message_t *msg)
                enumerator->destroy(enumerator);
        }
 
-       if (status != SUCCESS)
+       status = parse_status;
+
+       if (parse_status != SUCCESS)
        {
                bool is_request = msg->get_request(msg);
 
-               switch (status)
+               switch (parse_status)
                {
                        case NOT_SUPPORTED:
                                DBG1(DBG_IKE, "critical unknown payloads found");
@@ -1379,18 +1400,14 @@ static status_t parse_message(private_task_manager_t *this, message_t *msg)
                                DBG1(DBG_IKE, "message parsing failed");
                                if (is_request)
                                {
-                                       send_notify_response(this, msg,
-                                                                                INVALID_SYNTAX, chunk_empty);
-                                       incr_mid(this, FALSE);
+                                       status = send_invalid_syntax(this, msg);
                                }
                                break;
                        case VERIFY_ERROR:
                                DBG1(DBG_IKE, "message verification failed");
                                if (is_request)
                                {
-                                       send_notify_response(this, msg,
-                                                                                INVALID_SYNTAX, chunk_empty);
-                                       incr_mid(this, FALSE);
+                                       status = send_invalid_syntax(this, msg);
                                }
                                break;
                        case FAILED:
@@ -1407,11 +1424,25 @@ static status_t parse_message(private_task_manager_t *this, message_t *msg)
                         is_request ? "request" : "response",
                         msg->get_message_id(msg));
 
-               charon->bus->alert(charon->bus, ALERT_PARSE_ERROR_BODY, msg, status);
+               charon->bus->alert(charon->bus, ALERT_PARSE_ERROR_BODY, msg,
+                                                  parse_status);
 
-               if (this->ike_sa->get_state(this->ike_sa) == IKE_CREATED)
-               {       /* invalid initiation attempt, close SA */
-                       return DESTROY_ME;
+               switch (this->ike_sa->get_state(this->ike_sa))
+               {
+                       case IKE_CREATED:
+                               /* invalid initiation attempt, close SA */
+                               status = DESTROY_ME;
+                               break;
+                       case IKE_CONNECTING:
+                       case IKE_REKEYED:
+                               /* don't trigger updown event in these states */
+                               break;
+                       default:
+                               if (status == DESTROY_ME)
+                               {
+                                       charon->bus->ike_updown(charon->bus, this->ike_sa, FALSE);
+                               }
+                               break;
                }
        }
        return status;