-
Notifications
You must be signed in to change notification settings - Fork 97
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.
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
base: master
Are you sure you want to change the base?
Conversation
@@ -97,6 +102,7 @@ int cas_flock(apr_file_t *fileHandle, int lockOperation, request_rec *r) | |||
/* mod_auth_cas configuration specific functions */ | |||
void *cas_create_server_config(apr_pool_t *pool, server_rec *svr) | |||
{ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the extra whitespace here (and in other functions)?
Can you also add tests for this new functionality? I should state that I'm not 100% sold on the benefit of this just yet, at least for 'default enabled' mode - seems like there are a lot of ways for it to go wrong, and arguably the app needs to fix the XSS that led them to stolen cookies in the first place. Plus this will probably break situations like someone taking a session cookie and pasting it into, e.g. curl via command line. There's also a relatively recent addition to browsers called Channel ID, which attempts to solve a similar problem: http://www.browserauth.net/channel-bound-cookies I know Chrome supports it, I don't know if IE/FF do, but it may be a better route to go than crafting a custom mechanism like this. I'd be curious what others have to say. |
|
||
char *secure_id = NULL; | ||
|
||
// first pass (our prefered method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/prefered/preferred/
utilizing ssl_session_id (when mod_ssl is enabled) and ip/user-agent when using raw http connections an additional check is done to ensure that cooked are not copied between systems thus preventing session hijacking If a user with a valid CAS session attempts to copy another user's cookie the existing CAS session will simply reestablish and the stolen cookie will be invalidated. While this could potentially create a denial of service; the risk is limited to non-https connections.
While I agree with you that httpOnly and channel-bound cookies are a better solution to they don't addresses those using antiquated browsers. |
I think if you're using a browser that doesn't support httpOnly in 2014 (>99% support since 2011 according to https://www.owasp.org/index.php/HttpOnly#Browsers_Supporting_HttpOnly), you have far bigger problems than session hijacking :-) Your whole system is going to get owned by a drive-by exploit. As far as TLS-OBC goes, I think we'd need to find out browser support (Chrome supports it, and this Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=680292 seems to indicate NSS might support it) as well as whether mod_ssl exposes this value. I do think there could be some value to pin the session ID, but my other questions remain around:
In the case of TLS-OBC, #1 is solved because the client re-uses the same certificate, and #2 is likely solved because I doubt intermediate network gear would implement TLS-OBC. |
utilizing ssl_session_id (when mod_ssl is enabled) and ip/user-agent
when using raw http connections an additional check is done to ensure
that cooked are not copied between systems thus preventing session
hijacking
If a user with a valid CAS session attempts to copy another user's
cookie the existing CAS session will simply reestablish and the stolen
cookie will be invalidated.
While this could potentially create a denial of service; the risk is
limited to non-https connections.
In addition the isSSL function now utilizes the mod_ssl optional function ssl_is_https