Fix/session binding validation #61
Conversation
Replaced strict IP/UA binding with flexible multi-level approach Added IP history tracking with JSON serialization Implemented two-phase token invalidation for secure logouts Added maximum token lifetime checks Added activity timestamp tracking Added config-based security settings config/initializers/session_security.rb: Added configuration for security settings (binding strictness, timeouts) Configurable settings for IP changes, suspicious activity detection Set defaults for token lifetime and enforcement windows config/initializers/reload_authentication.rb: Added initializer to ensure proper module loading Force reload of authentication helpers during application startup db/migrate/20250428135250_add_session_binding_columns_to_auth_tokens.rb: Added columns for IP history (last_seen_ip, ip_history) Added suspicious_activity_detected_at for grace period tracking Added columns for flexible IP binding implementation db/migrate/20250429074259_add_timestamps_to_auth_tokens.rb: Added invalidation_requested_at column for two-phase logout Added last_activity_at for token activity tracking Added index on invalidation_requested_at for performance AuthToken.column_names: Added documentation for new column structure and usage db/schema.rb: Updated schema with new auth_tokens table structure Security validation test results confirm both issues are fixed: Session tokens cannot be used with different usernames (binding fix) Session tokens are properly invalidated after logout (fixation fix)
AuthToken.column_names
Outdated
| @@ -0,0 +1 @@ | |||
| [B[A | |||
There was a problem hiding this comment.
That was a documentation and the content went missing somehow but I've added it back!
-Added the missing documentation for new column structure and usage auth_token.rb - Added `destroy_invalidated_tokens` to clean up tokens marked for invalidation - Implemented `initialize_session_binding` to record IP and User-Agent at token creation - Introduced `ip_history_array` and `add_ip_to_history` to track and update IP usage - Added `invalidate` method to mark tokens for invalidation via timestamp - These additions support robust session binding enforcement and replay prevention 20241025050957_add_scorm_feat.rb - Change `unless` to `if` in the migration file - Wrapped `add_column`, `remove_column`, and `rename_column` operations with `column_exists?` checks to avoid migration crashes - Guarded index operations with `index_exists?` to prevent duplication errors during re-runs - Migrated SCORM-related columns to be added only if missing in `tasks` and `task_definitions` - Applied safer handling for polymorphic column renaming and indexing in `task_comments` - Added corresponding `down` method for reversibility with conditional checks 20250419030255_add_session_binding_to_auth_tokens.rb - Added migration to include `session_ip` and `session_user_agent` columns in auth_tokens table - Created `destroy_invalidated_tokens` method to clean up tokens marked for invalidation - Implemented `initialize_session_binding` to store IP/User-Agent on token creation - Added helpers: `ip_history_array`, `add_ip_to_history`, and `invalidate` for session tracking - These changes enable session binding enforcement and support mitigation of session hijacking/fixation risks session_security_fix_verification.md - Documentation for verifying session security fix - Added test cases for session binding and fixation prevention - Included instructions for running tests and verifying the fix test-session-binding.sh - Test script for session binding - Validates that the session binding feature works as intended - Ensures that tokens are invalidated when the session binding is broken - Provides feedback on the success or failure of the test test-session-fixation.sh - Test script for session fixation - Validates that the session fixation vulnerability is mitigated - Ensures that tokens are not reused across different sessions - Provides feedback on the success or failure of the test
db/migrate/20250419030255_add_session_binding_to_auth_tokens.rb
|
Hello @theiris6, This PR helps close the vulnerability of session hijacking. The modifications follow Doubtfire's standard. The code is readable, well-structured, and easy to read. Testing with Postman and BurpSuite shows this works well, and I have encountered no errors. Thank you for the opportunity to review your work. |
- Disgard changes in 20241025050957 add_scorm_feat - Add new migration file for consolidated security and features - Consolidate security and feature migrations into one - Ensure that the new migration file is properly formatted and includes all necessary changes
|
|
||
| # Check filenames in the upload requirements for each task definition | ||
| # and replace any invalid characters using sanitize filename | ||
| def change |
There was a problem hiding this comment.
Hi Iris, I am not sure that amending historical migrations is great practice. We should consider creating new migrations to implement these changes.
The changes themselves I agree and think are sound
There was a problem hiding this comment.
This has been resolved, thanks Iris!
| logger.info "DEBUG: Entered token expiry check for #{user.username}" | ||
|
|
||
| current_ip = request.ip | ||
| current_ua = request.user_agent |
There was a problem hiding this comment.
As discussed user_agent does not include PII or raise any data protection concerns - that is great
|
I've pulled this down and can see that the script runs well, follows project guidelines, and is well documented. We discussed not amending historical migrations and instead running new migrations and can see that has been done. Great work |
atharv02-git
left a comment
atharv02-git
left a comment
There was a problem hiding this comment.
Hi Iris, I’ve gone through your work and here’s my detailed review:
Observations
-
Successful Exploitation (Pre-Fix):
Before pulling your changes, I was able to successfully simulate the session hijacking vulnerability. By intercepting and injecting a privileged user’s token, I was able to perform admin-level actions like creating a unit using a low-privilege account confirming the issue. -
Script Execution:
After pulling your branch, I ran your automated test script, and it worked smoothly without any additional errors or warnings. The script effectively validated both session binding and invalidation, so I was able to automate the test reliably. -
Manual Retest (Post-Fix):
I manually re-ran the attack using Burp Suite (same method as before), but this time with your security patch applied. As expected, the session binding checks kicked in and prevented all privileged actions from being executed under the hijacked context. This confirms the vulnerability has been effectively patched.
Suggestions for Improvement
-
Documentation Depth (
.mdFile):
While your markdown file clearly explains the vulnerabilities and confirms that the fix works, it could benefit from:- A step-by-step guide on how to run the test script.
- Instructions on how to manually simulate the attack (tools required, headers to modify, endpoints to test).
- Think of it as a future-proof instruction manual something a teammate could use 2 years from now to replicate the same test.
-
Change Log in the
.mdFile:
Consider adding a brief section listing the files that were modified (e.g., models, initializers, migrations, helpers), along with a one-liner at the top of the files changed mentioning:- Why each file was changed,
- And how it supports the fix or the automation script.
This will help reviewers and future developers quickly trace how the all the files fit together without having to differentiate each file manually.
Overall this was top-notch work from your end, Iris. Keep up the awesome work! 👏🫡
|
LGTM. |
5 participants
Description
This PR addresses two critical security vulnerabilities in the authentication system:
Session Binding Vulnerability: The previous implementation used overly strict session binding that locked out legitimate users when their IP/User-Agent changed while still being vulnerable to session hijacking across different user accounts.
Session Fixation Vulnerability: The current authentication system allows session tokens to remain valid after logout if the logout request is intercepted, creating a security risk where stolen tokens can be reused.
Dependencies
Fixes # Session Binding/Fixation
Type of change
How Has This Been Tested?
Two custom test scripts were created to verify both security vulnerabilities are fixed:
Tests if tokens can be used across different user contexts
Verifies that student accounts cannot use admin tokens to perform privileged actions
Ensures normal admin functionality is preserved
Run:
chmod +x test-session-binding.sh
./test-session-binding.sh
Result should looks like this:
Simulates an intercepted logout request
Verifies token becomes invalid after the enforcement window
Confirms users can still login normally afterward
Run:
chmod +x test-session-fixation.sh
./test-session-fixation.sh
Checklist:
If you have any questions, please contact @macite or @jakerenzella.