improved logging on verify errors for some payloads
authorMartin Willi <martin@strongswan.org>
Thu, 13 Jul 2006 12:49:35 +0000 (12:49 -0000)
committerMartin Willi <martin@strongswan.org>
Thu, 13 Jul 2006 12:49:35 +0000 (12:49 -0000)
enforcing IKE_SA shutdown, even when transactions are outstanding
proper reject of CREATE_CHILD_SA message with KE payload

src/charon/encoding/payloads/encryption_payload.c
src/charon/encoding/payloads/proposal_substructure.c
src/charon/encoding/payloads/sa_payload.c
src/charon/encoding/payloads/transform_substructure.c
src/charon/sa/ike_sa.c
src/charon/sa/transactions/create_child_sa.c

index 026603a..caf34fb 100644 (file)
@@ -576,9 +576,8 @@ static status_t parse(private_encryption_payload_t *this)
                status = current_payload->verify(current_payload);
                if (status != SUCCESS)
                {
-                       this->logger->log(this->logger, ERROR, "%s verification failed: %s", 
-                                                               mapping_find(payload_type_m,current_payload->get_type(current_payload)),
-                                                               mapping_find(status_m, status));
+                       this->logger->log(this->logger, ERROR, "%s verification failed", 
+                                                               mapping_find(payload_type_m,current_payload->get_type(current_payload)));
                        current_payload->destroy(current_payload);
                        parser->destroy(parser);
                        return VERIFY_ERROR;
index d977633..cf4e413 100644 (file)
@@ -29,6 +29,7 @@
 #include <encoding/payloads/transform_substructure.h>
 #include <types.h>
 #include <utils/linked_list.h>
+#include <utils/logger_manager.h>
 
 
 /**
@@ -90,6 +91,11 @@ struct private_proposal_substructure_t {
        linked_list_t * transforms;
        
        /**
+        * assigned logger
+        */
+       logger_t *logger;
+       
+       /**
         * @brief Computes the length of this substructure.
         *
         * @param this  calling private_proposal_substructure_t object
@@ -153,17 +159,20 @@ static status_t verify(private_proposal_substructure_t *this)
        if ((this->next_payload != NO_PAYLOAD) && (this->next_payload != 2))
        {
                /* must be 0 or 2 */
+               this->logger->log(this->logger, ERROR, "inconsistent next payload");
                return FAILED;
        }
        if (this->transforms_count != this->transforms->get_count(this->transforms))
        {
                /* must be the same! */
+               this->logger->log(this->logger, ERROR, "transform count invalid");
                return FAILED;
        }
 
        if ((this->protocol_id == 0) || (this->protocol_id >= 4))
        {
                /* reserved are not supported */
+               this->logger->log(this->logger, ERROR, "invalid protocol");
                return FAILED;
        }
        
@@ -177,6 +186,7 @@ static status_t verify(private_proposal_substructure_t *this)
                status = current_transform->verify(current_transform);
                if (status != SUCCESS)
                {
+                       this->logger->log(this->logger, ERROR, "TRANSFORM_SUBSTRUCTURE verification failed");
                        break;
                }
        }
@@ -452,7 +462,7 @@ proposal_t* get_proposal(private_proposal_substructure_t *this)
 /**
  * Implementation of proposal_substructure_t.clone.
  */
-static private_proposal_substructure_t* clone(private_proposal_substructure_t *this)
+static private_proposal_substructure_t* clone_(private_proposal_substructure_t *this)
 {
        private_proposal_substructure_t * new_clone;
        iterator_t *transforms;
@@ -547,7 +557,7 @@ proposal_substructure_t *proposal_substructure_create()
        this->public.get_spi = (chunk_t (*) (proposal_substructure_t *)) get_spi;
        this->public.get_transform_count = (size_t (*) (proposal_substructure_t *)) get_transform_count;
        this->public.get_spi_size = (size_t (*) (proposal_substructure_t *)) get_spi_size;      
-       this->public.clone = (proposal_substructure_t * (*) (proposal_substructure_t *)) clone;
+       this->public.clone = (proposal_substructure_t * (*) (proposal_substructure_t *)) clone_;
        this->public.destroy = (void (*) (proposal_substructure_t *)) destroy;
        
        /* private functions */
@@ -562,6 +572,7 @@ proposal_substructure_t *proposal_substructure_create()
        this->spi_size = 0;
        this->spi.ptr = NULL;
        this->spi.len = 0;
+       this->logger = logger_manager->get_logger(logger_manager, PAYLOAD);
        
        this->transforms = linked_list_create();
        
index 49b0dc4..32b6b6a 100644 (file)
@@ -160,7 +160,7 @@ static status_t verify(private_sa_payload_t *this)
                status = current_proposal->payload_interface.verify(&(current_proposal->payload_interface));
                if (status != SUCCESS)
                {
-                       this->logger->log(this->logger, ERROR, "proposal substructure verification failed");
+                       this->logger->log(this->logger, ERROR, "PROPOSAL_SUBSTRUCTURE verification failed");
                        break;
                }
                first = FALSE;
index ecbf6ac..e241918 100644 (file)
@@ -29,6 +29,7 @@
 #include <encoding/payloads/encodings.h>
 #include <types.h>
 #include <utils/linked_list.h>
+#include <utils/logger_manager.h>
 
 
 typedef struct private_transform_substructure_t private_transform_substructure_t;
@@ -58,7 +59,7 @@ struct private_transform_substructure_t {
        /**
         * Type of the transform.
         */
-       u_int8_t         transform_type;
+       u_int8_t transform_type;
        
        /**
         * Transform ID.
@@ -66,10 +67,15 @@ struct private_transform_substructure_t {
        u_int16_t transform_id;
        
        /**
-        * Transforms Attributes are stored in a linked_list_t.
-        */
+        * Transforms Attributes are stored in a linked_list_t.
+        */
        linked_list_t *attributes;
        
+       /**
+        * assigned logger
+        */
+       logger_t *logger;
+       
        /**
         * @brief Computes the length of this substructure.
         *
@@ -130,70 +136,24 @@ static status_t verify(private_transform_substructure_t *this)
        if ((this->next_payload != NO_PAYLOAD) && (this->next_payload != 3))
        {
                /* must be 0 or 3 */
+               this->logger->log(this->logger, ERROR, "inconsistent next payload");
                return FAILED;
        }
 
        switch (this->transform_type)
        {
                case ENCRYPTION_ALGORITHM:
-               {
-                       if ((this->transform_id < ENCR_DES_IV64) || (this->transform_id > ENCR_AES_CTR))
-                       {
-                               return FAILED;
-                       }
-                       break;
-               }
                case PSEUDO_RANDOM_FUNCTION:
-               {
-                       if ((this->transform_id < PRF_HMAC_MD5) || (this->transform_id > PRF_AES128_CBC))
-                       {
-                               return FAILED;
-                       }
-                       break;
-               }
                case INTEGRITY_ALGORITHM:
-               {
-                       if ((this->transform_id < AUTH_HMAC_MD5_96) || (this->transform_id > AUTH_AES_XCBC_96))
-                       {
-                               return FAILED;
-                       }
-                       break;
-               }
                case DIFFIE_HELLMAN_GROUP:
-               {
-                       switch (this->transform_id)
-                       {
-                               case MODP_768_BIT:
-                               case MODP_1024_BIT:
-                               case MODP_1536_BIT:
-                               case MODP_2048_BIT:
-                               case MODP_3072_BIT:
-                               case MODP_4096_BIT:
-                               case MODP_6144_BIT:
-                               case MODP_8192_BIT:
-                               {
-                                       break;
-                               }
-                               default:
-                               {
-                                       return FAILED;
-                               }
-                       }
-                       
-                       
-                       break;
-               }
                case EXTENDED_SEQUENCE_NUMBERS:
-               {
-                       if ((this->transform_id != NO_EXT_SEQ_NUMBERS) && (this->transform_id != EXT_SEQ_NUMBERS))
-                       {
-                               return FAILED;
-                       }
+                       /* we don't check transform ID, we want to reply
+                        * cleanly with NO_PROPOSAL_CHOSEN or so if we don't support it */
                        break;
-               }
                default:
                {
-                       /* not a supported transform type! */
+                       this->logger->log(this->logger, ERROR, "invalid transform type: %d",
+                                                         this->transform_type);
                        return FAILED;
                }
        }
@@ -207,13 +167,12 @@ static status_t verify(private_transform_substructure_t *this)
                status = current_attributes->verify(current_attributes);
                if (status != SUCCESS)
                {
-                       break;
+                       this->logger->log(this->logger, ERROR, 
+                                                         "TRANSFORM_ATTRIBUTE verification failed");
                }
        }
-       
        iterator->destroy(iterator);
-
-
+       
        /* proposal number is checked in SA payload */  
        return status;
 }
@@ -347,7 +306,7 @@ static void compute_length (private_transform_substructure_t *this)
 /**
  * Implementation of transform_substructure_t.clone.
  */
-static transform_substructure_t *clone(private_transform_substructure_t *this)
+static transform_substructure_t *clone_(private_transform_substructure_t *this)
 {
        private_transform_substructure_t *new_clone;
        iterator_t *attributes;
@@ -448,7 +407,7 @@ transform_substructure_t *transform_substructure_create()
        this->public.set_transform_id = (void (*) (transform_substructure_t *,u_int16_t)) set_transform_id;
        this->public.get_transform_id = (u_int16_t (*) (transform_substructure_t *)) get_transform_id;
        this->public.get_key_length = (status_t (*) (transform_substructure_t *,u_int16_t *)) get_key_length;
-       this->public.clone = (transform_substructure_t* (*) (transform_substructure_t *)) clone;
+       this->public.clone = (transform_substructure_t* (*) (transform_substructure_t *)) clone_;
        this->public.destroy = (void (*) (transform_substructure_t *)) destroy;
        
        /* private functions */
@@ -460,6 +419,7 @@ transform_substructure_t *transform_substructure_create()
        this->transform_id = 0;
        this->transform_type = 0;
        this->attributes = linked_list_create();
+       this->logger = logger_manager->get_logger(logger_manager, PAYLOAD);
        
        return (&(this->public));
 }
index 14f72d4..fc176fa 100644 (file)
@@ -1258,6 +1258,13 @@ static status_t delete_(private_ike_sa_t *this)
        delete_ike_sa_t *delete_ike_sa;
        delete_ike_sa = delete_ike_sa_create(&this->public);
        
+       if (this->transaction_out)
+       {
+               /* already a transaction in progress. As this may hang
+                * around a while, we don't inform the other peer. */
+               return DESTROY_ME;
+       }
+       
        return queue_transaction(this, (transaction_t*)delete_ike_sa, FALSE);
 }
 
index a79d6ae..693e917 100644 (file)
@@ -370,7 +370,7 @@ static status_t process_notifys(private_create_child_sa_t *this, notify_payload_
 /**
  * Build a notify message.
  */
-static void build_notify(notify_type_t type, message_t *message, bool flush_message)
+static void build_notify(notify_type_t type, chunk_t data, message_t *message, bool flush_message)
 {
        notify_payload_t *notify;
        
@@ -388,6 +388,7 @@ static void build_notify(notify_type_t type, message_t *message, bool flush_mess
        
        notify = notify_payload_create();
        notify->set_notify_type(notify, type);
+       notify->set_notification_data(notify, data);
        message->add_payload(message, (payload_t*)notify);
 }
 
@@ -520,7 +521,15 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
                                break;  
                        case TRAFFIC_SELECTOR_RESPONDER:
                                tsr_request = (ts_payload_t*)payload;
-                               break;
+                       case KEY_EXCHANGE:
+                       {
+                               u_int8_t dh_buffer[] = {0x00, 0x00}; /* MODP_NONE */
+                               chunk_t group = chunk_from_buf(dh_buffer);
+                               build_notify(INVALID_KE_PAYLOAD, group, response, TRUE);
+                               this->logger->log(this->logger, CONTROL,
+                                                                 "CREATE_CHILD_SA used PFS, sending INVALID_KE_PAYLOAD");
+                               return FAILED;
+                       }
                        case NOTIFY:
                        {
                                status = process_notifys(this, (notify_payload_t*)payload);
@@ -545,7 +554,7 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
        /* check if we have all payloads */
        if (!(sa_request && nonce_request && tsi_request && tsr_request))
        {
-               build_notify(INVALID_SYNTAX, response, TRUE);
+               build_notify(INVALID_SYNTAX, CHUNK_INITIALIZER, response, TRUE);
                this->logger->log(this->logger, AUDIT, 
                                                  "request message incomplete, no CHILD_SA created");
                return FAILED;
@@ -556,7 +565,7 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
                if (this->randomizer->allocate_pseudo_random_bytes(this->randomizer, 
                        NONCE_SIZE, &this->nonce_r) != SUCCESS)
                {
-                       build_notify(NO_PROPOSAL_CHOSEN, response, TRUE);
+                       build_notify(NO_PROPOSAL_CHOSEN, CHUNK_INITIALIZER, response, TRUE);
                        return FAILED;
                }
                nonce_response = nonce_payload_create();
@@ -600,7 +609,7 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
                {
                        this->logger->log(this->logger, AUDIT, 
                                                          "CHILD_SA proposals unacceptable, adding NO_PROPOSAL_CHOSEN notify");
-                       build_notify(NO_PROPOSAL_CHOSEN, response, TRUE);
+                       build_notify(NO_PROPOSAL_CHOSEN, CHUNK_INITIALIZER, response, TRUE);
                        return FAILED;
                }
                /* do we have traffic selectors? */
@@ -608,7 +617,7 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
                {
                        this->logger->log(this->logger, AUDIT,
                                                          "CHILD_SA traffic selectors unacceptable, adding TS_UNACCEPTABLE notify");
-                       build_notify(TS_UNACCEPTABLE, response, TRUE);
+                       build_notify(TS_UNACCEPTABLE, CHUNK_INITIALIZER, response, TRUE);
                        return FAILED;
                }
                else
@@ -629,7 +638,7 @@ static status_t get_response(private_create_child_sa_t *this, message_t *request
                        {
                                this->logger->log(this->logger, ERROR,
                                                                  "installing CHILD_SA failed, adding NO_PROPOSAL_CHOSEN notify");
-                               build_notify(NO_PROPOSAL_CHOSEN, response, TRUE);
+                               build_notify(NO_PROPOSAL_CHOSEN, CHUNK_INITIALIZER, response, TRUE);
                                return FAILED;
                        }
                        /* add proposal to sa payload */