*) SECURITY: CVE-2013-2249 (cve.mitre.org) 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. This changes the format of the updatesession SQL statement. Existing configurations must be changed. [Takashi Sato <takashi tks.st>, Graham Leggett] --- httpd/httpd/trunk/modules/session/mod_session_dbd.c 2012/11/14 11:43:49 1409170 +++ httpd/httpd/trunk/modules/session/mod_session_dbd.c 2013/05/31 11:13:25 1488158 @@ -230,12 +230,11 @@ 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 @@ /** * 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 @@ 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 @@ 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 @@ 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 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 @@ /* 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 @@ 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; }