Refactor how KEYMGMT methods get associated with other methods
authorRichard Levitte <levitte@openssl.org>
Fri, 23 Aug 2019 12:03:28 +0000 (14:03 +0200)
committerRichard Levitte <levitte@openssl.org>
Tue, 3 Sep 2019 08:36:49 +0000 (10:36 +0200)
KEYMGMT methods were attached to other methods after those were fully
created and registered, thereby creating a potential data race, if two
threads tried to create the exact same method at the same time.

Instead of this, we change the method creating function to take an
extra data parameter, passed all the way from the public fetching
function.  In the case of EVP_KEYEXCH, we pass all the necessary data
that evp_keyexch_from_dispatch() needs to be able to fetch the
appropriate KEYMGMT method on the fly.

Fixes #9592

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9678)

crypto/err/openssl.txt
crypto/evp/digest.c
crypto/evp/evp_enc.c
crypto/evp/evp_err.c
crypto/evp/evp_fetch.c
crypto/evp/evp_locl.h
crypto/evp/exchange.c
crypto/evp/keymgmt_meth.c
crypto/evp/mac_meth.c
doc/internal/man3/evp_generic_fetch.pod
include/openssl/evperr.h

index 58f6c48..9b682d5 100644 (file)
@@ -2484,6 +2484,7 @@ EVP_R_NOT_XOF_OR_INVALID_LENGTH:178:not XOF or invalid length
 EVP_R_NO_CIPHER_SET:131:no cipher set
 EVP_R_NO_DEFAULT_DIGEST:158:no default digest
 EVP_R_NO_DIGEST_SET:139:no digest set
+EVP_R_NO_KEYMGMT_AVAILABLE:199:no keymgmt available
 EVP_R_NO_KEYMGMT_PRESENT:196:no keymgmt present
 EVP_R_NO_KEY_SET:154:no key set
 EVP_R_NO_OPERATION_SET:149:no operation set
index dc7f922..bb6d31b 100644 (file)
@@ -617,7 +617,7 @@ int EVP_MD_CTX_ctrl(EVP_MD_CTX *ctx, int cmd, int p1, void *p2)
 }
 
 static void *evp_md_from_dispatch(const char *name, const OSSL_DISPATCH *fns,
-                                  OSSL_PROVIDER *prov)
+                                  OSSL_PROVIDER *prov, void *unused)
 {
     EVP_MD *md = NULL;
     int fncnt = 0;
@@ -744,7 +744,7 @@ EVP_MD *EVP_MD_fetch(OPENSSL_CTX *ctx, const char *algorithm,
 {
     EVP_MD *md =
         evp_generic_fetch(ctx, OSSL_OP_DIGEST, algorithm, properties,
-                          evp_md_from_dispatch, evp_md_up_ref,
+                          evp_md_from_dispatch, NULL, evp_md_up_ref,
                           evp_md_free);
 
     return md;
@@ -756,5 +756,5 @@ void EVP_MD_do_all_ex(OPENSSL_CTX *libctx,
 {
     evp_generic_do_all(libctx, OSSL_OP_DIGEST,
                        (void (*)(void *, void *))fn, arg,
-                       evp_md_from_dispatch, evp_md_free);
+                       evp_md_from_dispatch, NULL, evp_md_free);
 }
index 96a15ef..142ffec 100644 (file)
@@ -1246,7 +1246,8 @@ int EVP_CIPHER_CTX_copy(EVP_CIPHER_CTX *out, const EVP_CIPHER_CTX *in)
 
 static void *evp_cipher_from_dispatch(const char *name,
                                       const OSSL_DISPATCH *fns,
-                                      OSSL_PROVIDER *prov)
+                                      OSSL_PROVIDER *prov,
+                                      void *unused)
 {
     EVP_CIPHER *cipher = NULL;
     int fnciphcnt = 0, fnctxcnt = 0;
@@ -1386,7 +1387,7 @@ EVP_CIPHER *EVP_CIPHER_fetch(OPENSSL_CTX *ctx, const char *algorithm,
 {
     EVP_CIPHER *cipher =
         evp_generic_fetch(ctx, OSSL_OP_CIPHER, algorithm, properties,
-                          evp_cipher_from_dispatch, evp_cipher_up_ref,
+                          evp_cipher_from_dispatch, NULL, evp_cipher_up_ref,
                           evp_cipher_free);
 
     return cipher;
@@ -1398,5 +1399,5 @@ void EVP_CIPHER_do_all_ex(OPENSSL_CTX *libctx,
 {
     evp_generic_do_all(libctx, OSSL_OP_CIPHER,
                        (void (*)(void *, void *))fn, arg,
-                       evp_cipher_from_dispatch, evp_cipher_free);
+                       evp_cipher_from_dispatch, NULL, evp_cipher_free);
 }
index 749f189..63174f9 100644 (file)
@@ -99,6 +99,8 @@ static const ERR_STRING_DATA EVP_str_reasons[] = {
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_CIPHER_SET), "no cipher set"},
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_DEFAULT_DIGEST), "no default digest"},
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_DIGEST_SET), "no digest set"},
+    {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEYMGMT_AVAILABLE),
+    "no keymgmt available"},
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEYMGMT_PRESENT), "no keymgmt present"},
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_KEY_SET), "no key set"},
     {ERR_PACK(ERR_LIB_EVP, 0, EVP_R_NO_OPERATION_SET), "no operation set"},
index fa85178..662195e 100644 (file)
@@ -41,7 +41,8 @@ struct method_data_st {
     const char *name;
     OSSL_METHOD_CONSTRUCT_METHOD *mcm;
     void *(*method_from_dispatch)(const char *, const OSSL_DISPATCH *,
-                                  OSSL_PROVIDER *);
+                                  OSSL_PROVIDER *, void *);
+    void *method_data;
     int (*refcnt_up_method)(void *method);
     void (*destruct_method)(void *method);
 };
@@ -142,7 +143,8 @@ static void *construct_method(const char *name, const OSSL_DISPATCH *fns,
 {
     struct method_data_st *methdata = data;
 
-    return methdata->method_from_dispatch(name, fns, prov);
+    return methdata->method_from_dispatch(name, fns, prov,
+                                          methdata->method_data);
 }
 
 static void destruct_method(void *method, void *data)
@@ -156,7 +158,9 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id,
                         const char *name, const char *properties,
                         void *(*new_method)(const char *name,
                                             const OSSL_DISPATCH *fns,
-                                            OSSL_PROVIDER *prov),
+                                            OSSL_PROVIDER *prov,
+                                            void *method_data),
+                        void *method_data,
                         int (*up_ref_method)(void *),
                         void (*free_method)(void *))
 {
@@ -205,6 +209,7 @@ void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id,
         mcmdata.destruct_method = free_method;
         mcmdata.refcnt_up_method = up_ref_method;
         mcmdata.destruct_method = free_method;
+        mcmdata.method_data = method_data;
         if ((method = ossl_method_construct(libctx, operation_id, name,
                                             properties, 0 /* !force_cache */,
                                             &mcm, &mcmdata)) != NULL) {
@@ -239,7 +244,7 @@ struct do_all_data_st {
     void (*user_fn)(void *method, void *arg);
     void *user_arg;
     void *(*new_method)(const char *name, const OSSL_DISPATCH *fns,
-                        OSSL_PROVIDER *prov);
+                        OSSL_PROVIDER *prov, void *method_data);
     void (*free_method)(void *);
 };
 
@@ -248,7 +253,7 @@ static void do_one(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *algo,
 {
     struct do_all_data_st *data = vdata;
     void *method = data->new_method(algo->algorithm_name,
-                                    algo->implementation, provider);
+                                    algo->implementation, provider, NULL);
 
     if (method != NULL) {
         data->user_fn(method, data->user_arg);
@@ -261,7 +266,9 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id,
                         void *user_arg,
                         void *(*new_method)(const char *name,
                                             const OSSL_DISPATCH *fns,
-                                            OSSL_PROVIDER *prov),
+                                            OSSL_PROVIDER *prov,
+                                            void *method_data),
+                        void *method_data,
                         void (*free_method)(void *))
 {
     struct do_all_data_st data;
@@ -270,5 +277,5 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id,
     data.free_method = free_method;
     data.user_fn = user_fn;
     data.user_arg = user_arg;
-    ossl_algorithm_do_all(libctx, operation_id, NULL, do_one, &data);
+    ossl_algorithm_do_all(libctx, operation_id, method_data, do_one, &data);
 }
index 3fd7321..a7b36db 100644 (file)
@@ -141,7 +141,9 @@ void *evp_generic_fetch(OPENSSL_CTX *ctx, int operation_id,
                         const char *algorithm, const char *properties,
                         void *(*new_method)(const char *name,
                                             const OSSL_DISPATCH *fns,
-                                            OSSL_PROVIDER *prov),
+                                            OSSL_PROVIDER *prov,
+                                            void *method_data),
+                        void *method_data,
                         int (*up_ref_method)(void *),
                         void (*free_method)(void *));
 void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id,
@@ -149,7 +151,9 @@ void evp_generic_do_all(OPENSSL_CTX *libctx, int operation_id,
                         void *user_arg,
                         void *(*new_method)(const char *name,
                                             const OSSL_DISPATCH *fns,
-                                            OSSL_PROVIDER *prov),
+                                            OSSL_PROVIDER *prov,
+                                            void *method_data),
+                        void *method_data,
                         void (*free_method)(void *));
 
 /* Helper functions to avoid duplicating code */
index 20503b5..e69e4fd 100644 (file)
@@ -32,20 +32,45 @@ static EVP_KEYEXCH *evp_keyexch_new(OSSL_PROVIDER *prov)
     return exchange;
 }
 
+struct keymgmt_data_st {
+    OPENSSL_CTX *ctx;
+    const char *properties;
+};
+
 static void *evp_keyexch_from_dispatch(const char *name,
                                        const OSSL_DISPATCH *fns,
-                                       OSSL_PROVIDER *prov)
+                                       OSSL_PROVIDER *prov,
+                                       void *vkeymgmt_data)
 {
+    /*
+     * Key exchange cannot work without a key, and key management
+     * from the same provider to manage its keys.  We therefore fetch
+     * a key management method using the same algorithm and properties
+     * and pass that down to evp_generic_fetch to be passed on to our
+     * evp_keyexch_from_dispatch, which will attach the key management
+     * method to the newly created key exchange method as long as the
+     * provider matches.
+     */
+    struct keymgmt_data_st *keymgmt_data = vkeymgmt_data;
+    EVP_KEYMGMT *keymgmt = EVP_KEYMGMT_fetch(keymgmt_data->ctx, name,
+                                             keymgmt_data->properties);
     EVP_KEYEXCH *exchange = NULL;
     int fncnt = 0;
 
+    if (keymgmt == NULL || EVP_KEYMGMT_provider(keymgmt) != prov) {
+        ERR_raise(ERR_LIB_EVP, EVP_R_NO_KEYMGMT_AVAILABLE);
+        goto err;
+    }
+
     if ((exchange = evp_keyexch_new(prov)) == NULL
         || (exchange->name = OPENSSL_strdup(name)) == NULL) {
-        EVP_KEYEXCH_free(exchange);
-        EVPerr(0, ERR_R_MALLOC_FAILURE);
-        return NULL;
+        ERR_raise(ERR_LIB_EVP, ERR_R_MALLOC_FAILURE);
+        goto err;
     }
 
+    exchange->keymgmt = keymgmt;
+    keymgmt = NULL;              /* avoid double free on failure below */
+
     for (; fns->function_id != 0; fns++) {
         switch (fns->function_id) {
         case OSSL_FUNC_KEYEXCH_NEWCTX:
@@ -96,13 +121,17 @@ static void *evp_keyexch_from_dispatch(const char *name,
          * and freectx. The dupctx, set_peer and set_params functions are
          * optional.
          */
-        EVP_KEYEXCH_free(exchange);
         EVPerr(EVP_F_EVP_KEYEXCH_FROM_DISPATCH,
                EVP_R_INVALID_PROVIDER_FUNCTIONS);
-        return NULL;
+        goto err;
     }
 
     return exchange;
+
+ err:
+    EVP_KEYEXCH_free(exchange);
+    EVP_KEYMGMT_free(keymgmt);
+    return NULL;
 }
 
 void EVP_KEYEXCH_free(EVP_KEYEXCH *exchange)
@@ -137,31 +166,16 @@ OSSL_PROVIDER *EVP_KEYEXCH_provider(const EVP_KEYEXCH *exchange)
 EVP_KEYEXCH *EVP_KEYEXCH_fetch(OPENSSL_CTX *ctx, const char *algorithm,
                                const char *properties)
 {
-    /*
-     * Key exchange cannot work without a key, and we key management
-     * from the same provider to manage its keys.
-     */
-    EVP_KEYEXCH *keyexch =
-        evp_generic_fetch(ctx, OSSL_OP_KEYEXCH, algorithm, properties,
-                          evp_keyexch_from_dispatch,
-                          (int (*)(void *))EVP_KEYEXCH_up_ref,
-                          (void (*)(void *))EVP_KEYEXCH_free);
-
-    /* If the method is newly created, there's no keymgmt attached */
-    if (keyexch->keymgmt == NULL) {
-        EVP_KEYMGMT *keymgmt = EVP_KEYMGMT_fetch(ctx, algorithm, properties);
-
-        if (keymgmt == NULL
-            || (EVP_KEYEXCH_provider(keyexch)
-                != EVP_KEYMGMT_provider(keymgmt))) {
-            EVP_KEYEXCH_free(keyexch);
-            EVP_KEYMGMT_free(keymgmt);
-            EVPerr(EVP_F_EVP_KEYEXCH_FETCH, EVP_R_NO_KEYMGMT_PRESENT);
-            return NULL;
-        }
+    EVP_KEYEXCH *keyexch = NULL;
+    struct keymgmt_data_st keymgmt_data;
+
+    keymgmt_data.ctx = ctx;
+    keymgmt_data.properties = properties;
+    keyexch = evp_generic_fetch(ctx, OSSL_OP_KEYEXCH, algorithm, properties,
+                                evp_keyexch_from_dispatch, &keymgmt_data,
+                                (int (*)(void *))EVP_KEYEXCH_up_ref,
+                                (void (*)(void *))EVP_KEYEXCH_free);
 
-        keyexch->keymgmt = keymgmt;
-    }
     return keyexch;
 }
 
index 67c33eb..72ef1bd 100644 (file)
@@ -34,7 +34,7 @@ static void *keymgmt_new(void)
 }
 
 static void *keymgmt_from_dispatch(const char *name, const OSSL_DISPATCH *fns,
-                                   OSSL_PROVIDER *prov)
+                                   OSSL_PROVIDER *prov, void *unused)
 {
     EVP_KEYMGMT *keymgmt = NULL;
 
@@ -156,7 +156,7 @@ EVP_KEYMGMT *EVP_KEYMGMT_fetch(OPENSSL_CTX *ctx, const char *algorithm,
 {
     EVP_KEYMGMT *keymgmt =
         evp_generic_fetch(ctx, OSSL_OP_KEYMGMT, algorithm, properties,
-                          keymgmt_from_dispatch,
+                          keymgmt_from_dispatch, NULL,
                           (int (*)(void *))EVP_KEYMGMT_up_ref,
                           (void (*)(void *))EVP_KEYMGMT_free);
 
index 1c0d642..a317127 100644 (file)
@@ -48,7 +48,7 @@ static void *evp_mac_new(void)
 }
 
 static void *evp_mac_from_dispatch(const char *name, const OSSL_DISPATCH *fns,
-                                   OSSL_PROVIDER *prov)
+                                   OSSL_PROVIDER *prov, void *unused)
 {
     EVP_MAC *mac = NULL;
     int fnmaccnt = 0, fnctxcnt = 0;
@@ -154,7 +154,7 @@ EVP_MAC *EVP_MAC_fetch(OPENSSL_CTX *libctx, const char *algorithm,
                        const char *properties)
 {
     return evp_generic_fetch(libctx, OSSL_OP_MAC, algorithm, properties,
-                             evp_mac_from_dispatch, evp_mac_up_ref,
+                             evp_mac_from_dispatch, NULL, evp_mac_up_ref,
                              evp_mac_free);
 }
 
@@ -205,5 +205,5 @@ void EVP_MAC_do_all_ex(OPENSSL_CTX *libctx,
 {
     evp_generic_do_all(libctx, OSSL_OP_MAC,
                        (void (*)(void *, void *))fn, arg,
-                       evp_mac_from_dispatch, evp_mac_free);
+                       evp_mac_from_dispatch, NULL, evp_mac_free);
 }
index 0688ac0..b77391e 100644 (file)
@@ -11,8 +11,11 @@ evp_generic_fetch - generic algorithm fetcher and method creator for EVP
 
  void *evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id,
                          const char *name, const char *properties,
-                         void *(*new_method)(const OSSL_DISPATCH *fns,
-                                             OSSL_PROVIDER *prov),
+                         void *(*new_method)(const char *name,
+                                             const OSSL_DISPATCH *fns,
+                                             OSSL_PROVIDER *prov,
+                                             void *method_data),
+                         void *method_data,
                          int (*up_ref_method)(void *),
                          void (*free_method)(void *));
 
@@ -31,6 +34,8 @@ The three functions are supposed to:
 
 creates an internal method from function pointers found in the
 dispatch table C<fns>.
+The algorithm I<name>, provider I<prov>, and I<method_data> are
+also passed to be used as new_method() sees fit.
 
 =item up_ref_method()
 
index 34966f8..714f170 100644 (file)
@@ -41,10 +41,6 @@ int ERR_load_EVP_strings(void);
 #  define EVP_F_ARIA_GCM_INIT_KEY                          0
 #  define EVP_F_ARIA_INIT_KEY                              0
 #  define EVP_F_B64_NEW                                    0
-#  define EVP_F_BLAKE2B_MAC_CTRL                           0
-#  define EVP_F_BLAKE2B_MAC_INIT                           0
-#  define EVP_F_BLAKE2S_MAC_CTRL                           0
-#  define EVP_F_BLAKE2S_MAC_INIT                           0
 #  define EVP_F_CAMELLIA_INIT_KEY                          0
 #  define EVP_F_CHACHA20_POLY1305_CTRL                     0
 #  define EVP_F_CMLL_T4_INIT_KEY                           0
@@ -218,6 +214,7 @@ int ERR_load_EVP_strings(void);
 # define EVP_R_NO_CIPHER_SET                              131
 # define EVP_R_NO_DEFAULT_DIGEST                          158
 # define EVP_R_NO_DIGEST_SET                              139
+# define EVP_R_NO_KEYMGMT_AVAILABLE                       199
 # define EVP_R_NO_KEYMGMT_PRESENT                         196
 # define EVP_R_NO_KEY_SET                                 154
 # define EVP_R_NO_OPERATION_SET                           149