Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Advanced session hijacking mitigation #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Advanced session hijacking mitigation #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 117 additions & 4 deletions src/mod_auth_cas.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
#include "apr_strings.h"
#include "apr_xml.h"

#include "mod_ssl.h"

#include "cas_saml_attr.h"

/* Apache is NOT a well-behaved citizen. It unconditionally
Expand All @@ -76,6 +78,9 @@ static apr_thread_mutex_t **ssl_locks;
static int ssl_num_locks;
#endif /* defined(OPENSSL_THREADS) && APR_HAS_THREADS */

static APR_OPTIONAL_FN_TYPE(ssl_var_lookup) *mod_ssl_var_lookup;
static APR_OPTIONAL_FN_TYPE(ssl_is_https) *mod_ssl_is_https;

int cas_flock(apr_file_t *fileHandle, int lockOperation, request_rec *r)
{
apr_os_file_t osFileHandle;
Expand Down Expand Up @@ -116,6 +121,7 @@ void *cas_create_server_config(apr_pool_t *pool, server_rec *svr)
c->CASAttributeDelimiter = CAS_DEFAULT_ATTRIBUTE_DELIMITER;
c->CASAttributePrefix = CAS_DEFAULT_ATTRIBUTE_PREFIX;
c->CASAuthoritative = CAS_DEFAULT_AUTHORITATIVE;
c->CASSecureSession = CAS_DEFAULT_SECURE_SESSION;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this parameter documented anywhere (most of the others at least have a mention in the README at least)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, looks like this isn't handled in any config parsing logic?


cas_setURL(pool, &(c->CASLoginURL), CAS_DEFAULT_LOGIN_URL);
cas_setURL(pool, &(c->CASValidateURL), CAS_DEFAULT_VALIDATE_URL);
Expand Down Expand Up @@ -414,14 +420,16 @@ apr_byte_t cas_setURL(apr_pool_t *pool, apr_uri_t *uri, const char *url)

apr_byte_t isSSL(const request_rec *r)
{

if (mod_ssl_is_https) {
return mod_ssl_is_https(r->connection) != 0;
} else {
#ifdef APACHE2_0
if(apr_strnatcasecmp("https", ap_http_method(r)) == 0)
#else
if(apr_strnatcasecmp("https", ap_http_scheme(r)) == 0)
#endif
return TRUE;

}
return FALSE;
}

Expand Down Expand Up @@ -977,6 +985,7 @@ apr_byte_t readCASCacheFile(request_rec *r, cas_cfg *c, char *name, cas_cache_en
cache->secure = FALSE;
cache->ticket = NULL;
cache->attrs = NULL;
cache->session = NULL;

do {
if(e == NULL)
Expand Down Expand Up @@ -1015,6 +1024,8 @@ apr_byte_t readCASCacheFile(request_rec *r, cas_cfg *c, char *name, cas_cache_en
cache->secure = TRUE;
else if (apr_strnatcasecmp(e->name, "ticket") == 0)
cache->ticket = apr_pstrndup(r->pool, val, strlen(val));
else if (apr_strnatcasecmp(e->name, "session") == 0)
cache->session = apr_pstrndup(r->pool, val, strlen(val));
else if (apr_strnatcasecmp(e->name, "attributes") == 0) {
cas_attr_builder *builder = cas_attr_builder_new(r->pool, &(cache->attrs));
apr_xml_elem *attrs;
Expand Down Expand Up @@ -1217,6 +1228,7 @@ apr_byte_t writeCASCacheEntry(request_rec *r, char *name, cas_cache_entry *cache
apr_file_printf(f, "<lastactive>%" APR_TIME_T_FMT "</lastactive>\n", cache->lastactive);
apr_file_printf(f, "<path>%s</path>\n", apr_xml_quote_string(r->pool, cache->path, TRUE));
apr_file_printf(f, "<ticket>%s</ticket>\n", apr_xml_quote_string(r->pool, cache->ticket, TRUE));
apr_file_printf(f, "<session>%s</session>\n", apr_xml_quote_string(r->pool, cache->session, TRUE));
if(cache->attrs != NULL) {
cas_saml_attr *a = cache->attrs;
apr_file_printf(f, "<attributes>\n");
Expand Down Expand Up @@ -1251,6 +1263,87 @@ apr_byte_t writeCASCacheEntry(request_rec *r, char *name, cas_cache_entry *cache
return TRUE;
}

/*
* hook function to retrieve the mod_ssl optional functions for looking up ssl state data
* ssl_var_lookup allows us to fetch the ssl environment variables (used for accessing the SSL_SESSION_ID)
* ssl_is_https is the mod_ssl to achieve the equivalent of isSSL
*/
static void retrieve_mod_ssl_functions(void) {

mod_ssl_var_lookup = APR_RETRIEVE_OPTIONAL_FN(ssl_var_lookup);
mod_ssl_is_https = APR_RETRIEVE_OPTIONAL_FN(ssl_is_https);

}

/*
* cas_secure_session()
*
* Build an internal session identifier for mod_auth_cas to use in session hijacking mitigation.
* existing mitigations neglect full cookie theft and instead rely on cookie modifications.
* The preferred method of session id generation is to borrow the SSL Session from mod_ssl
* given that the SSL Session is not exposed to browser javascript engines we can assert
* with a high degree of confidence that SSL_SESSION_ID will not likely be stolen.
* In the event we negotiating via HTTP we attempt to create as secure an id as we can.
* An HTTP id is comprised of the user's IP address which is acquired either via the useragent_ip
* field of the apache request (if available in our current version) or from the apr_get_remote_host
* function (which should be available in apache 2.x). This value is concatenated with the
* user-agent header to create a semi unique internal identifier which should not change
* during the session.
*
* Known issues:
* In the event the apache server is running behind NAT or a load-balancer the ip address
* obtained by ap_get_remote_host may be the same for all users. it is for this reason we
* would rather obtain useragent_ip when available.
*
* In the event the above takes place theft of the user-agent can be used to spoof our users
* ''' navigator.userAgent + document.cookie '''
*
* In order to address these we would need other internal identifier similar to the SSL
* session id which is unique and not exposed to the end-user
*
*/
char *cas_secure_session(request_rec *r) {

static const char kNULL_ID[] = "[NULL]";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use [NULL] instead of treating the empty string as a null session ID and "(void *) 0" in its place?


char *secure_id = NULL;

// first pass (our preferred method)
if (mod_ssl_var_lookup) {
secure_id = mod_ssl_var_lookup(r->pool, r->server, r->connection, r, "SSL_SESSION_ID");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'm guessing this assumes that subsequent TLS negotiations will use TLS session resumption, which probably holds for most clients -- have you tested with mobile clients or firewalls / proxies / load balancers fronting a mod_auth_cas install that may behave differently?

Do you offhand have any reference information on how long a client will cache a TLS session, and how that compares with the default values for the CAS session timeouts?

}

if (!secure_id || !strlen(secure_id)) {

char *ip_addr = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this play with someone coming from a large NAT pool?

char *user_agent = NULL;
char *plaintext = NULL;

// in apache 2.4 we are able to use the useragent_ip which should be the conneting host
// this is a much better solution as it will not be covered by natted requests.
#ifdef APACHE2_4
ip_addr = (char *)r->useragent_ip;
#endif

// if we are less than apache 2.4 or the useragent_ip field is NULL we can fallback to
// fetching the ip from the ap_get_remote_host function
if (!ip_addr)
ip_addr = (char *)ap_get_remote_host(r->connection, r->connection->base_server->lookup_defaults, REMOTE_NOLOOKUP, NULL);

user_agent = (char *)apr_table_get(r->headers_in, "User-Agent");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this header is not present or empty? Does apr_pstrcat safely handle a NULL concatenation? (can't recall)


plaintext = apr_pstrcat(r->pool, ip_addr, user_agent, NULL);

secure_id = ap_md5(r->pool, (unsigned char *)plaintext);
}

// If everything else has failed up to this point we are going to unfortunately use "[NULL]"
if (!secure_id)
secure_id = (char *)apr_pstrdup(r->pool, kNULL_ID);

return (char *)secure_id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for cast here?

}

char *createCASCookie(request_rec *r, char *user, cas_saml_attr *attrs, char *ticket)
{
char *path, *buf, *rv;
Expand All @@ -1275,6 +1368,7 @@ char *createCASCookie(request_rec *r, char *user, cas_saml_attr *attrs, char *ti
e.secure = (isSSL(r) == TRUE ? 1 : 0);
e.ticket = ticket;
e.attrs = attrs;
e.session = cas_secure_session(r);

/* this may block since this reads from /dev/random - however, it hasn't been a problem in testing */
apr_generate_random_bytes((unsigned char *) buf, c->CASCookieEntropy);
Expand Down Expand Up @@ -1629,6 +1723,23 @@ apr_byte_t isValidCASCookie(request_rec *r, cas_cfg *c, char *cookie, char **use
return FALSE;
}

/*
* session hijacking mitigation through the cas entry "session" variable.
* when possible we check for SSL_SESSION_ID matching (most secure) otherwise a number of other
* session idenifiers are used. cas_secure_session()
*/
if (c->CASSecureSession) {

if (apr_strnatcmp(cas_secure_session(r), cache.session) != 0) {

deleteCASCacheFile(r, cookie);
if(c->CASDebug)
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "Cookie '%s' originated from invalid source.", cookie);
CASCleanCache(r, c);
return FALSE;
}
}

/*
* mitigate session hijacking by not allowing cookies transmitted in the clear to be submitted
* for HTTPS URLs and by voiding HTTPS cookies sent in the clear
Expand Down Expand Up @@ -2301,7 +2412,7 @@ int cas_authorize(request_rec *r)
&auth_cas_module);

ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Entering cas_authorize.");
"Entering cas_authorize()");

if (!reqs_arr) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
Expand Down Expand Up @@ -2651,8 +2762,10 @@ void cas_register_hooks(apr_pool_t *p)
#else
ap_hook_check_user_id(cas_authenticate, NULL, NULL, APR_HOOK_MIDDLE);
#endif
ap_hook_auth_checker(cas_authorize, NULL, authzSucc, APR_HOOK_MIDDLE);
ap_hook_auth_checker(cas_authorize, NULL, authzSucc, APR_HOOK_MIDDLE);
ap_register_input_filter("CAS", cas_in_filter, NULL, AP_FTYPE_RESOURCE);
// mod_ssl_hook retrieve
ap_hook_optional_fn_retrieve(retrieve_mod_ssl_functions, NULL, NULL, APR_HOOK_MIDDLE);
}

const command_rec cas_cmds [] = {
Expand Down
15 changes: 15 additions & 0 deletions src/mod_auth_cas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@
#endif
#endif

#ifndef APACHE2_4
#ifdef AP_SERVER_MAJORVERSION_NUMBER
#ifdef AP_SERVER_MINORVERSION_NUMBER
#if ((AP_SERVER_MAJORVERSION_NUMBER == 2) && (AP_SERVER_MINORVERSION_NUMBER >= 4))
#define APACHE2_4
#endif
#endif
#endif
#endif

#define CAS_DEFAULT_VERSION 2
#define CAS_DEFAULT_DEBUG FALSE
#define CAS_DEFAULT_SCOPE NULL
Expand Down Expand Up @@ -93,6 +103,7 @@
#define CAS_DEFAULT_SCRUB_REQUEST_HEADERS NULL
#define CAS_DEFAULT_SSO_ENABLED FALSE
#define CAS_DEFAULT_AUTHORITATIVE FALSE
#define CAS_DEFAULT_SECURE_SESSION TRUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1713 mentions a TODO for enforcing, but here IIUC the default is set to enforcing mode?


#define CAS_MAX_RESPONSE_SIZE 65536
#define CAS_MAX_ERROR_SIZE 1024
Expand Down Expand Up @@ -120,6 +131,7 @@ typedef struct cas_cfg {
unsigned int CASSSOEnabled;
unsigned int CASAuthoritative;
unsigned int CASValidateSAML;
unsigned int CASSecureSession;
char *CASCertificatePath;
char *CASCookiePath;
char *CASCookieDomain;
Expand Down Expand Up @@ -151,6 +163,7 @@ typedef struct cas_cache_entry {
apr_byte_t secure;
char *ticket;
cas_saml_attr *attrs;
char *session;
} cas_cache_entry;

typedef struct cas_curl_buffer {
Expand Down Expand Up @@ -222,6 +235,8 @@ int check_merged_vhost_configs(apr_pool_t *pool, server_rec *s);
int check_vhost_config(apr_pool_t *pool, server_rec *s);
int merged_vhost_configs_exist(server_rec *s);

char *cas_secure_session(request_rec *r);

#if (defined(OPENSSL_THREADS) && APR_HAS_THREADS)
void cas_ssl_locking_callback(int mode, int type, const char *file, int line);
#endif
Expand Down