On 06/13/2016 09:38 AM, Ewald Dieterich wrote:
I configured form authentication with mod_auth_form, mod_session_cookie
and mod_session_crypto in Apache 2.4.20 on Debian unstable and get
random AH01842 errors ("decrypt session failed, wrong passphrase"). The
passphrase was not changed when this happens.

It looks like the error occurs when the following conditions are met:

* mpm_worker enabled (never experienced the error with mpm_prefork)
* Same user doing multiple requests in parallel using the same session
(don't see the error when the user is doing only sequential requests)

Looks like the problem is this:

* In session_crypto_init() a crypto context is created from a global pool (server->pconf). * In encrypt_string() and decrypt_string() a key is created from the context via apr_crypto_passphrase() using the global pool for allocating memory for the key.
* Multiple threads might access the global pool at the same time.
* APR documentation about pool thread-safety: "Note that most operations on pools are not thread-safe: a single pool should only be accessed by a single thread at any given time."

I changed mod_session_crypto to use the request pool instead and it seems that this fixed my problem.

I think this also fixes a memory consumption problem: Keys are only created, but never explicitly destroyed (or reused). So for every request memory is allocated from the global pool, but this memory is never freed during the lifetime of mod_session_crypto. Using the request pool fixes this problem because it is destroyed when the request is done.

See the attached patch session-crypto.patch that I created for 2.4.20.
--- a/modules/session/mod_session_crypto.c
+++ b/modules/session/mod_session_crypto.c
@@ -34,7 +34,7 @@
 
 #include "apr_crypto.h"                /* for apr_*_crypt et al */
 
-#define CRYPTO_KEY "session_crypto_context"
+#define DRIVER_KEY "session_crypto_driver"
 
 module AP_MODULE_DECLARE_DATA session_crypto_module;
 
@@ -333,6 +333,35 @@
 
 }
 
+static int session_crypto_init_per_request(request_rec *r, const apr_crypto_t **ff)
+{
+    apr_crypto_t *f = NULL;
+
+    session_crypto_conf *conf = ap_get_module_config(r->server->module_config,
+            &session_crypto_module);
+
+    if (conf->library) {
+        const apr_crypto_driver_t *driver = NULL;
+        apr_pool_t *p = r->pool;
+        apr_status_t rv;
+
+        apr_pool_userdata_get((void **)&driver, DRIVER_KEY,
+                              r->server->process->pconf);
+
+        rv = apr_crypto_make(&f, driver, conf->params, p);
+        if (APR_SUCCESS != rv) {
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01848)
+                          "The crypto context could not be initialised");
+            return rv;
+        }
+    }
+
+    *ff = f;
+
+    return OK;
+}
+
+
 /**
  * Crypto encoding for the session.
  *
@@ -349,7 +378,13 @@
             &session_crypto_module);
 
     if (dconf->passphrases_set && z->encoded && *z->encoded) {
-        apr_pool_userdata_get((void **)&f, CRYPTO_KEY, r->server->process->pconf);
+        res = session_crypto_init_per_request(r, &f);
+        if (res != OK) {
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, res, r,
+                          "session_crypto_encode: session_crypto_init_per_request failed");
+            return res;
+        }
+
         res = encrypt_string(r, f, dconf, z->encoded, &encoded);
         if (res != OK) {
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, res, r, APLOGNO(01841)
@@ -380,8 +415,13 @@
             &session_crypto_module);
 
     if ((dconf->passphrases_set) && z->encoded && *z->encoded) {
-        apr_pool_userdata_get((void **)&f, CRYPTO_KEY,
-                r->server->process->pconf);
+        res = session_crypto_init_per_request(r, &f);
+        if (res != OK) {
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, res, r,
+                          "session_crypto_decode: session_crypto_init_per_request failed");
+            return res;
+        }
+
         res = decrypt_string(r, f, dconf, z->encoded, &encoded);
         if (res != APR_SUCCESS) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, res, r, APLOGNO(01842)
@@ -402,7 +442,6 @@
         apr_pool_t *ptemp, server_rec *s)
 {
     const apr_crypto_driver_t *driver = NULL;
-    apr_crypto_t *f = NULL;
 
     session_crypto_conf *conf = ap_get_module_config(s->module_config,
             &session_crypto_module);
@@ -451,19 +490,11 @@
             return rv;
         }
 
-        rv = apr_crypto_make(&f, driver, conf->params, p);
-        if (APR_SUCCESS != rv) {
-            ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, APLOGNO(01848)
-                    "The crypto library '%s' could not be initialised",
-                    conf->library);
-            return rv;
-        }
-
         ap_log_error(APLOG_MARK, APLOG_INFO, rv, s, APLOGNO(01849)
                 "The crypto library '%s' was loaded successfully",
                 conf->library);
 
-        apr_pool_userdata_set((const void *)f, CRYPTO_KEY,
+        apr_pool_userdata_set((const void *)driver, DRIVER_KEY,
                 apr_pool_cleanup_null, s->process->pconf);
 
     }

Reply via email to