From 7af21c53da7bb1de024274ee6da30bc22316a079 Mon Sep 17 00:00:00 2001 From: Olav Morken Date: Mon, 13 Mar 2017 09:55:48 +0100 Subject: [PATCH] Fix Cross-Site Session Transfer vulnerability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mod_auth_mellon did not verify that the site the session was created for was the same site as the site the user accessed. This allows an attacker with access to one web site on a server to use the same session to get access to a different site running on the same server. This patch fixes this vulnerability by storing the cookie parameters used when creating the session in the session, and verifying those parameters when the session is loaded. Thanks to François Kooman for reporting this vulnerability. This vulnerability has been assigned CVE-2017-6807. --- NEWS | 24 ++++++++++++++++++++++++ auth_mellon.h | 6 +++++- auth_mellon_cache.c | 18 +++++++++++++++++- auth_mellon_cookie.c | 28 ++++++++++++++++++++++++++++ auth_mellon_session.c | 43 ++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 114 insertions(+), 5 deletions(-) diff --git a/auth_mellon.h b/auth_mellon.h index d19ef02..78a5f0d 100644 --- a/auth_mellon.h +++ b/auth_mellon.h @@ -290,6 +290,7 @@ typedef struct am_cache_env_t { typedef struct am_cache_entry_t { char key[AM_CACHE_KEYSIZE]; + am_cache_storage_t cookie_token; apr_time_t access; apr_time_t expires; int logged_in; @@ -373,6 +374,7 @@ void *auth_mellon_server_config(apr_pool_t *p, server_rec *s); const char *am_cookie_get(request_rec *r); void am_cookie_set(request_rec *r, const char *id); void am_cookie_delete(request_rec *r); +const char *am_cookie_token(request_rec *r); void am_cache_init(am_mod_cfg_rec *mod_cfg); @@ -380,7 +382,9 @@ am_cache_entry_t *am_cache_lock(server_rec *s, am_cache_key_t type, const char *key); const char *am_cache_entry_get_string(am_cache_entry_t *e, am_cache_storage_t *slot); -am_cache_entry_t *am_cache_new(server_rec *s, const char *key); +am_cache_entry_t *am_cache_new(server_rec *s, + const char *key, + const char *cookie_token); void am_cache_unlock(server_rec *s, am_cache_entry_t *entry); void am_cache_update_expires(am_cache_entry_t *t, apr_time_t expires); diff --git a/auth_mellon_cache.c b/auth_mellon_cache.c index cdb1e91..9a5bb0e 100644 --- a/auth_mellon_cache.c +++ b/auth_mellon_cache.c @@ -273,12 +273,15 @@ const char *am_cache_entry_get_string(am_cache_entry_t *e, * Parameters: * server_rec *s The current server. * const char *key The key of the session to allocate. + * const char *cookie_token The cookie token to tie the session to. * * Returns: * The new session entry on success. NULL if key is a invalid session * key. */ -am_cache_entry_t *am_cache_new(server_rec *s, const char *key) +am_cache_entry_t *am_cache_new(server_rec *s, + const char *key, + const char *cookie_token) { am_cache_entry_t *t; am_mod_cfg_rec *mod_cfg; @@ -374,6 +377,7 @@ am_cache_entry_t *am_cache_new(server_rec *s, const char *key) t->logged_in = 0; t->size = 0; + am_cache_storage_null(&t->cookie_token); am_cache_storage_null(&t->user); am_cache_storage_null(&t->lasso_identity); am_cache_storage_null(&t->lasso_session); @@ -384,6 +388,18 @@ am_cache_entry_t *am_cache_new(server_rec *s, const char *key) t->pool[0] = '\0'; t->pool_used = 1; + rv = am_cache_entry_store_string(t, &t->cookie_token, cookie_token); + if (rv != 0) { + /* For some strange reason our cookie token is too big to fit in the + * session. This should never happen outside of absurd configurations. + */ + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Unable to store cookie token in new session."); + t->key[0] = '\0'; /* Mark the entry as free. */ + apr_global_mutex_unlock(mod_cfg->lock); + return NULL; + } + return t; } diff --git a/auth_mellon_cookie.c b/auth_mellon_cookie.c index 8b3bc23..445022f 100644 --- a/auth_mellon_cookie.c +++ b/auth_mellon_cookie.c @@ -252,3 +252,31 @@ void am_cookie_delete(request_rec *r) apr_table_addn(r->err_headers_out, "Set-Cookie", cookie); } + +/* Get string that is used to tie a session to a specific cookie. + * + * request_rec *r The current request. + * Returns: + * The cookie token, as a fixed length byte buffer. + */ +const char *am_cookie_token(request_rec *r) +{ + const char *cookie_name = am_cookie_name(r); + const char *cookie_domain = ap_get_server_name(r); + const char *cookie_path = "/"; + am_dir_cfg_rec *cfg = am_get_dir_cfg(r); + + if (cfg->cookie_domain) { + cookie_domain = cfg->cookie_domain; + } + + if (cfg->cookie_path) { + cookie_path = cfg->cookie_path; + } + + return apr_psprintf(r->pool, "Name='%s' Domain='%s' Path='%s'", + cookie_name, + cookie_domain, + cookie_path + ); +} diff --git a/auth_mellon_session.c b/auth_mellon_session.c index eb6439a..fca6c01 100644 --- a/auth_mellon_session.c +++ b/auth_mellon_session.c @@ -22,6 +22,42 @@ #include "auth_mellon.h" +/* Retrieve a session from the cache and validate its cookie settings + * + * Parameters: + * request_rec *r The request we received from the user. + * am_cache_key_t type AM_CACHE_SESSION or AM_CACHE_NAMEID + * const char *key The session key or user + * + * Returns: + * The session associated, or NULL if unable to retrieve the given session. + */ +am_cache_entry_t *am_lock_and_validate(request_rec *r, + am_cache_key_t type, + const char *key) +{ + am_cache_entry_t *session = am_cache_lock(r->server, type, key); + if (session == NULL) { + return NULL; + } + + const char *cookie_token_session = am_cache_entry_get_string( + session, &session->cookie_token); + const char *cookie_token_target = am_cookie_token(r); + if (strcmp(cookie_token_session, cookie_token_target)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, + "Session cookie parameter mismatch. " + "Session created with {%s}, but current " + "request has {%s}.", + cookie_token_session, + cookie_token_target); + am_cache_unlock(r->server, session); + return NULL; + } + + return session; +} + /* This function gets the session associated with a user, using a cookie * * Parameters: @@ -45,7 +81,7 @@ am_cache_entry_t *am_get_request_session(request_rec *r) return NULL; } - return am_cache_lock(r->server, AM_CACHE_SESSION, session_id); + return am_lock_and_validate(r, AM_CACHE_SESSION, session_id); } /* This function gets the session associated with a user, using a NameID @@ -60,7 +96,7 @@ am_cache_entry_t *am_get_request_session(request_rec *r) */ am_cache_entry_t *am_get_request_session_by_nameid(request_rec *r, char *nameid) { - return am_cache_lock(r->server, AM_CACHE_NAMEID, nameid); + return am_lock_and_validate(r, AM_CACHE_NAMEID, nameid); } /* This function creates a new session. @@ -87,7 +123,8 @@ am_cache_entry_t *am_new_request_session(request_rec *r) /* Set session id. */ am_cookie_set(r, session_id); - return am_cache_new(r->server, session_id); + const char *cookie_token = am_cookie_token(r); + return am_cache_new(r->server, session_id, cookie_token); }