BUG/MEDIUM: ssl: Fix crash when calling "update ssl ocsp-response" when an update...

The CLI command "update ssl ocsp-response" was forcefully removing an
OCSP response from the update tree regardless of whether it used to be
in it beforehand or not. But since the main OCSP upate task works by
removing the entry being currently updated from the update tree and then
reinserting it when the update process is over, it meant that in the CLI
command code we were modifying a structure that was already being used.

These concurrent accesses were not properly locked on the "regular"
update case because it was assumed that once an entry was removed from
the update tree, the update task was the only one able to work on it.

Rather than locking the whole update process, an "updating" flag was
added to the certificate_ocsp in order to prevent the "update ssl
ocsp-response" command from trying to update a response already being
updated.

An easy way to reproduce this crash was to perform two "simultaneous"
calls to "update ssl ocsp-response" on the same certificate. It would
then crash on an eb64_delete call in the main ocsp update task function.

This patch can be backported up to 2.8. Wait a little bit before
backporting.

diff --git a/include/haproxy/ssl_ocsp-t.h b/include/haproxy/ssl_ocsp-t.h
index 34e55d48bd948d8e287c7f63566377520703fdc9..f9fef4d8125c96a91a067c213ee0648d1e75d2ce 100644 (file)
--- a/include/haproxy/ssl_ocsp-t.h
+++ b/include/haproxy/ssl_ocsp-t.h
@@ -61,8 +61,9 @@ struct certificate_ocsp {
        unsigned int last_update_status;/* Status of the last OCSP update */
        unsigned int num_success;       /* Number of successful updates */
        unsigned int num_failure;       /* Number of failed updates */
-       unsigned int fail_count:31;     /* Number of successive failures */
+       unsigned int fail_count:30;     /* Number of successive failures */
        unsigned int update_once:1;     /* Set if an entry should not be reinserted into te tree after update */
+       unsigned int updating:1;        /* Set if an entry is already being updated */
        char path[VAR_ARRAY];
 };
 

diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c
index 61375d5dd7cb131656dba1324d7acc67f8ef45a0..95e113e8e0197b852d78b4eb436eed63ee9d7742 100644 (file)
--- a/src/ssl_ocsp.c
+++ b/src/ssl_ocsp.c
@@ -1002,12 +1002,6 @@ static inline void ssl_ocsp_set_next_update(struct certificate_ocsp *ocsp)
  */
 int ssl_ocsp_update_insert(struct certificate_ocsp *ocsp)
 {
-       /* This entry was only supposed to be updated once, it does not need to
-        * be reinserted into the update tree.
-        */
-       if (ocsp->update_once)
-               return 0;
-
        /* Set next_update based on current time and the various OCSP
         * minimum/maximum update times.
         */
@@ -1016,7 +1010,12 @@ int ssl_ocsp_update_insert(struct certificate_ocsp *ocsp)
        ocsp->fail_count = 0;
 
        HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
-       eb64_insert(&ocsp_update_tree, &ocsp->next_update);
+       ocsp->updating = 0;
+       /* An entry with update_once set to 1 was only supposed to be updated
+        * once, it does not need to be reinserted into the update tree.
+        */
+       if (!ocsp->update_once)
+               eb64_insert(&ocsp_update_tree, &ocsp->next_update);
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
 
        return 0;
@@ -1033,12 +1032,6 @@ int ssl_ocsp_update_insert_after_error(struct certificate_ocsp *ocsp)
 {
        int replay_delay = 0;
 
-       /* This entry was only supposed to be updated once, it does not need to
-        * be reinserted into the update tree.
-        */
-       if (ocsp->update_once)
-               return 0;
-
        /*
         * Set next_update based on current time and the various OCSP
         * minimum/maximum update times.
@@ -1061,7 +1054,12 @@ int ssl_ocsp_update_insert_after_error(struct certificate_ocsp *ocsp)
                ocsp->next_update.key = date.tv_sec + replay_delay;
 
        HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock);
-       eb64_insert(&ocsp_update_tree, &ocsp->next_update);
+       ocsp->updating = 0;
+       /* An entry with update_once set to 1 was only supposed to be updated
+        * once, it does not need to be reinserted into the update tree.
+        */
+       if (!ocsp->update_once)
+               eb64_insert(&ocsp_update_tree, &ocsp->next_update);
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);
 
        return 0;
@@ -1268,6 +1266,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
                eb64_delete(&ocsp->next_update);
 
                ++ocsp->refcount;
+               ocsp->updating = 1;
                ctx->cur_ocsp = ocsp;
                ocsp->last_update_status = OCSP_UPDT_UNKNOWN;
 
@@ -1448,21 +1447,24 @@ static int cli_parse_update_ocsp_response(char **args, char *payload, struct app
                goto end;
        }
 
-       update_once = (ocsp->next_update.node.leaf_p == NULL);
-       eb64_delete(&ocsp->next_update);
+       /* No need to try to update this response, it is already being updated. */
+       if (!ocsp->updating) {
+               update_once = (ocsp->next_update.node.leaf_p == NULL);
+               eb64_delete(&ocsp->next_update);
 
-       /* Insert the entry at the beginning of the update tree.
-        * We don't need to increase the reference counter on the
-        * certificate_ocsp structure because we would not have a way to
-        * decrease it afterwards since this update operation is asynchronous.
-        * If the corresponding entry were to be destroyed before the update can
-        * be performed, which is pretty unlikely, it would not be such a
-        * problem because that would mean that the OCSP response is not
-        * actually used.
-        */
-       ocsp->next_update.key = 0;
-       eb64_insert(&ocsp_update_tree, &ocsp->next_update);
-       ocsp->update_once = update_once;
+               /* Insert the entry at the beginning of the update tree.
+                * We don't need to increase the reference counter on the
+                * certificate_ocsp structure because we would not have a way to
+                * decrease it afterwards since this update operation is asynchronous.
+                * If the corresponding entry were to be destroyed before the update can
+                * be performed, which is pretty unlikely, it would not be such a
+                * problem because that would mean that the OCSP response is not
+                * actually used.
+                */
+               ocsp->next_update.key = 0;
+               eb64_insert(&ocsp_update_tree, &ocsp->next_update);
+               ocsp->update_once = update_once;
+       }
 
        HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock);