CVE-2013-2249

mod_session_dbd: Make sure that dirty flag is respected when saving
sessions, and ensure the session ID is changed each time the session
changes.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1488158 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 4c8c9d0cae228a8399915011b167d4d5e41e08c1..c8fe9a5421ca0d6e8113302b1c5a658d321fbf69 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_session_dbd: Make sure that dirty flag is respected when saving
+     sessions, and ensure the session ID is changed each time the session
+     changes. [Takashi Sato <takashi tks.st>, Graham Leggett]
+
   *) mod_dav: Sending a MERGE request against a URI handled by mod_dav_svn with
      the source href (sent as part of the request body as XML) pointing to a
      URI that is not configured for DAV will trigger a segfault. [Ben Reser

diff --git a/modules/session/mod_session.c b/modules/session/mod_session.c
index a3354a59c4bc1b472af60abd10c88ac518dc16f6..7213eb3c8e999fa9f936cde40d6485afc9d31a3b 100644 (file)
--- a/modules/session/mod_session.c
+++ b/modules/session/mod_session.c
@@ -132,8 +132,6 @@ static apr_status_t ap_session_load(request_rec * r, session_rec ** z)
         zz = (session_rec *) apr_pcalloc(r->pool, sizeof(session_rec));
         zz->pool = r->pool;
         zz->entries = apr_table_make(zz->pool, 10);
-        zz->uuid = (apr_uuid_t *) apr_pcalloc(zz->pool, sizeof(apr_uuid_t));
-        apr_uuid_get(zz->uuid);
 
     }
     else {
@@ -446,6 +444,7 @@ static apr_status_t session_output_filter(ap_filter_t * f,
             }
             if (override) {
                 z->encoded = override;
+                z->dirty = 1;
                 session_identity_decode(r, z);
             }
         }

diff --git a/modules/session/mod_session_cookie.c b/modules/session/mod_session_cookie.c
index 15b3d9c6c524210811dcc0487d9406cf69a7c7cd..6a02322bf19a9756bb13e21e045b787b0a1885ca 100644 (file)
--- a/modules/session/mod_session_cookie.c
+++ b/modules/session/mod_session_cookie.c
@@ -157,7 +157,6 @@ static apr_status_t session_cookie_load(request_rec * r, session_rec ** z)
     zz->pool = m->pool;
     zz->entries = apr_table_make(m->pool, 10);
     zz->encoded = val;
-    zz->uuid = (apr_uuid_t *) apr_pcalloc(m->pool, sizeof(apr_uuid_t));
     *z = zz;
 
     /* put the session in the notes so we don't have to parse it again */

diff --git a/modules/session/mod_session_dbd.c b/modules/session/mod_session_dbd.c
index d6349a8d306dfaf646f9a1e16304fe641840b075..a6ab40ea6f573b8d4915e64a7f2eaf6674e31612 100644 (file)
--- a/modules/session/mod_session_dbd.c
+++ b/modules/session/mod_session_dbd.c
@@ -230,12 +230,11 @@ static apr_status_t session_dbd_load(request_rec * r, session_rec ** z)
     zz = (session_rec *) apr_pcalloc(r->pool, sizeof(session_rec));
     zz->pool = r->pool;
     zz->entries = apr_table_make(zz->pool, 10);
-    zz->uuid = (apr_uuid_t *) apr_pcalloc(zz->pool, sizeof(apr_uuid_t));
-    if (key) {
-        apr_uuid_parse(zz->uuid, key);
-    }
-    else {
-        apr_uuid_get(zz->uuid);
+    if (key && val) {
+        apr_uuid_t *uuid = apr_pcalloc(zz->pool, sizeof(apr_uuid_t));
+        if (APR_SUCCESS == apr_uuid_parse(uuid, key)) {
+            zz->uuid = uuid;
+        }
     }
     zz->encoded = val;
     *z = zz;
@@ -250,8 +249,8 @@ static apr_status_t session_dbd_load(request_rec * r, session_rec ** z)
 /**
  * Save the session by the key specified.
  */
-static apr_status_t dbd_save(request_rec * r, const char *key, const char *val,
-                             apr_int64_t expiry)
+static apr_status_t dbd_save(request_rec * r, const char *oldkey,
+        const char *newkey, const char *val, apr_int64_t expiry)
 {
 
     apr_status_t rv;
@@ -272,22 +271,24 @@ static apr_status_t dbd_save(request_rec * r, const char *key, const char *val,
     if (rv) {
         return rv;
     }
-    rv = apr_dbd_pvbquery(dbd->driver, r->pool, dbd->handle, &rows, statement,
-                          val, &expiry, key, NULL);
-    if (rv) {
-        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01857)
-                      "query execution error updating session '%s' "
-                      "using database query '%s': %s", key, conf->updatelabel,
-                      apr_dbd_error(dbd->driver, dbd->handle, rv));
-        return APR_EGENERAL;
-    }
 
-    /*
-     * if some rows were updated it means a session existed and was updated,
-     * so we are done.
-     */
-    if (rows != 0) {
-        return APR_SUCCESS;
+    if (oldkey) {
+        rv = apr_dbd_pvbquery(dbd->driver, r->pool, dbd->handle, &rows,
+                statement, val, &expiry, newkey, oldkey, NULL);
+        if (rv) {
+            ap_log_rerror(
+                    APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01857) "query execution error updating session '%s' "
+                    "using database query '%s': %s/%s", oldkey, newkey, conf->updatelabel, apr_dbd_error(dbd->driver, dbd->handle, rv));
+            return APR_EGENERAL;
+        }
+
+        /*
+         * if some rows were updated it means a session existed and was updated,
+         * so we are done.
+         */
+        if (rows != 0) {
+            return APR_SUCCESS;
+        }
     }
 
     if (conf->insertlabel == NULL) {
@@ -301,11 +302,11 @@ static apr_status_t dbd_save(request_rec * r, const char *key, const char *val,
         return rv;
     }
     rv = apr_dbd_pvbquery(dbd->driver, r->pool, dbd->handle, &rows, statement,
-                          val, &expiry, key, NULL);
+                          val, &expiry, newkey, NULL);
     if (rv) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01859)
                       "query execution error inserting session '%s' "
-                      "in database with '%s': %s", key, conf->insertlabel,
+                      "in database with '%s': %s", newkey, conf->insertlabel,
                       apr_dbd_error(dbd->driver, dbd->handle, rv));
         return APR_EGENERAL;
     }
@@ -320,7 +321,7 @@ static apr_status_t dbd_save(request_rec * r, const char *key, const char *val,
 
     ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01860)
                   "the session insert query did not cause any rows to be added "
-                  "to the database for session '%s', session not inserted", key);
+                  "to the database for session '%s', session not inserted", newkey);
 
     return APR_EGENERAL;
 
@@ -397,27 +398,38 @@ static apr_status_t dbd_clean(apr_pool_t *p, server_rec *s)
 static apr_status_t session_dbd_save(request_rec * r, session_rec * z)
 {
 
-    char *buffer;
     apr_status_t ret = APR_SUCCESS;
     session_dbd_dir_conf *conf = ap_get_module_config(r->per_dir_config,
                                                       &session_dbd_module);
 
     /* support anonymous sessions */
     if (conf->name_set || conf->name2_set) {
+        char *oldkey = NULL, *newkey = NULL;
 
         /* don't cache pages with a session */
         apr_table_addn(r->headers_out, "Cache-Control", "no-cache");
 
-        /* must we create a uuid? */
-        buffer = apr_pcalloc(r->pool, APR_UUID_FORMATTED_LENGTH + 1);
-        apr_uuid_format(buffer, z->uuid);
+        /* if the session is new or changed, make a new session ID */
+        if (z->uuid) {
+            oldkey = apr_pcalloc(r->pool, APR_UUID_FORMATTED_LENGTH + 1);
+            apr_uuid_format(oldkey, z->uuid);
+        }
+        if (z->dirty || !oldkey) {
+            z->uuid = apr_pcalloc(z->pool, sizeof(apr_uuid_t));
+            apr_uuid_get(z->uuid);
+            newkey = apr_pcalloc(r->pool, APR_UUID_FORMATTED_LENGTH + 1);
+            apr_uuid_format(newkey, z->uuid);
+        }
+        else {
+            newkey = oldkey;
+        }
 
         /* save the session with the uuid as key */
         if (z->encoded && z->encoded[0]) {
-            ret = dbd_save(r, buffer, z->encoded, z->expiry);
+            ret = dbd_save(r, oldkey, newkey, z->encoded, z->expiry);
         }
         else {
-            ret = dbd_remove(r, buffer);
+            ret = dbd_remove(r, oldkey);
         }
         if (ret != APR_SUCCESS) {
             return ret;
@@ -425,13 +437,13 @@ static apr_status_t session_dbd_save(request_rec * r, session_rec * z)
 
         /* create RFC2109 compliant cookie */
         if (conf->name_set) {
-            ap_cookie_write(r, conf->name, buffer, conf->name_attrs, z->maxage,
+            ap_cookie_write(r, conf->name, newkey, conf->name_attrs, z->maxage,
                             r->headers_out, r->err_headers_out, NULL);
         }
 
         /* create RFC2965 compliant cookie */
         if (conf->name2_set) {
-            ap_cookie_write2(r, conf->name2, buffer, conf->name2_attrs, z->maxage,
+            ap_cookie_write2(r, conf->name2, newkey, conf->name2_attrs, z->maxage,
                              r->headers_out, r->err_headers_out, NULL);
         }
 
@@ -446,7 +458,7 @@ static apr_status_t session_dbd_save(request_rec * r, session_rec * z)
         apr_table_addn(r->headers_out, "Cache-Control", "no-cache");
 
         if (r->user) {
-            ret = dbd_save(r, r->user, z->encoded, z->expiry);
+            ret = dbd_save(r, r->user, r->user, z->encoded, z->expiry);
             if (ret != APR_SUCCESS) {
                 return ret;
             }