mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
Add: Quick reference guide for the password reset token fix
This commit is contained in:
parent
4dec1187e2
commit
013ba0480a
1 changed files with 202 additions and 0 deletions
202
README_FIX.md
Normal file
202
README_FIX.md
Normal file
|
|
@ -0,0 +1,202 @@
|
|||
# Django Password Reset Token Security Fix
|
||||
|
||||
## Overview
|
||||
This repository contains a complete fix for a Django security vulnerability where changing a user's email address did not invalidate existing password reset tokens.
|
||||
|
||||
## Vulnerability Description
|
||||
**Sequence:**
|
||||
1. User with email `foo@example.com` requests a password reset
|
||||
2. Password reset token is generated and sent via email
|
||||
3. User changes their email address to `bar@example.com`
|
||||
4. User uses the original password reset token
|
||||
5. **VULNERABILITY**: Token is accepted even though email changed
|
||||
|
||||
**Impact**: An attacker who intercepts the password reset token could use it to reset the password even after the victim changes their email address.
|
||||
|
||||
## The Fix
|
||||
|
||||
### What Changed
|
||||
The `PasswordResetTokenGenerator._make_hash_value()` method now includes the user's email address in the token hash computation.
|
||||
|
||||
**Before (Vulnerable):**
|
||||
```python
|
||||
def _make_hash_value(self, user, timestamp):
|
||||
login_timestamp = '' if user.last_login is None else user.last_login.replace(microsecond=0, tzinfo=None)
|
||||
return str(user.pk) + user.password + str(login_timestamp) + str(timestamp)
|
||||
```
|
||||
|
||||
**After (Secure):**
|
||||
```python
|
||||
def _make_hash_value(self, user, timestamp):
|
||||
login_timestamp = '' if user.last_login is None else user.last_login.replace(microsecond=0, tzinfo=None)
|
||||
email_field_name = user.get_email_field_name()
|
||||
email = getattr(user, email_field_name, '') or ''
|
||||
return str(user.pk) + user.password + str(login_timestamp) + email + str(timestamp)
|
||||
```
|
||||
|
||||
### Why This Works
|
||||
- **Email is included in hash**: Changing email changes the hash value
|
||||
- **Token validation fails**: New hash doesn't match old token
|
||||
- **Token is invalidated**: User must request a new password reset
|
||||
|
||||
### Key Design Decisions
|
||||
1. **Uses `get_email_field_name()`**: Supports custom user models
|
||||
2. **Safe email retrieval**: Handles users without email field
|
||||
3. **Backward compatible approach**: Uses standard Django API
|
||||
|
||||
## Files in This Solution
|
||||
|
||||
### Documentation
|
||||
- **README_FIX.md** - This file, quick overview
|
||||
- **SOLUTION_SUMMARY.md** - Comprehensive technical summary
|
||||
- **IMPLEMENTATION_PLAN.md** - Detailed implementation guide
|
||||
- **DJANGO_FIX_SUMMARY.md** - Technical summary
|
||||
|
||||
### Implementation Files
|
||||
- **django_password_reset_token_fix.patch** - Unified diff patch file
|
||||
- **test_password_reset_fix.py** - Interactive demonstration script
|
||||
|
||||
## Quick Start
|
||||
|
||||
### View the Demonstration
|
||||
```bash
|
||||
python test_password_reset_fix.py
|
||||
```
|
||||
|
||||
This shows:
|
||||
- How the vulnerability works
|
||||
- How the fix resolves it
|
||||
- The actual code changes
|
||||
- Test case that validates the fix
|
||||
|
||||
### Apply the Fix
|
||||
Option 1 - Using the patch:
|
||||
```bash
|
||||
cd /path/to/django
|
||||
patch -p1 < django_password_reset_token_fix.patch
|
||||
```
|
||||
|
||||
Option 2 - Manual application:
|
||||
1. Edit `django/contrib/auth/tokens.py`
|
||||
2. Modify the `_make_hash_value()` method as shown
|
||||
3. Edit `tests/auth_tests/test_tokens.py`
|
||||
4. Add the new test method
|
||||
|
||||
### Verify the Fix
|
||||
```bash
|
||||
# Run the new test
|
||||
python tests/runtests.py auth_tests.test_tokens.TokenGeneratorTest.test_token_invalidated_after_email_change
|
||||
|
||||
# Run all token tests
|
||||
python tests/runtests.py auth_tests.test_tokens
|
||||
|
||||
# Expected result: All 8 tests pass
|
||||
```
|
||||
|
||||
## Test Results
|
||||
|
||||
All tests pass successfully:
|
||||
```
|
||||
Testing against Django installed in '/tmp/django-fix/django' with up to 48 processes
|
||||
Creating test database for alias 'default'...
|
||||
System check identified no issues (0 silenced).
|
||||
........
|
||||
----------------------------------------------------------------------
|
||||
Ran 8 tests in 0.005s
|
||||
|
||||
OK
|
||||
Destroying test database for alias 'default'...
|
||||
```
|
||||
|
||||
### Tests Included
|
||||
1. ✓ `test_make_token` - Basic token generation
|
||||
2. ✓ `test_10265` - Token consistency
|
||||
3. ✓ `test_timeout` - Token expiration
|
||||
4. ✓ `test_check_token_with_nonexistent_token_and_user` - Null handling
|
||||
5. ✓ `test_token_with_different_secret` - Secret validation
|
||||
6. ✓ `test_token_default_hashing_algorithm` - Algorithm selection
|
||||
7. ✓ `test_legacy_token_validation` - Backward compatibility
|
||||
8. ✓ `test_token_invalidated_after_email_change` - **NEW** Email change test
|
||||
|
||||
## Implementation Details
|
||||
|
||||
### Token Hash Computation
|
||||
**Before**: `pk + password + last_login + timestamp`
|
||||
**After**: `pk + password + last_login + email + timestamp`
|
||||
|
||||
### Supported Scenarios
|
||||
- ✓ Standard Django User model
|
||||
- ✓ Custom user models with different email field names
|
||||
- ✓ Users without an email field
|
||||
- ✓ All password reset token scenarios
|
||||
|
||||
### Edge Cases Handled
|
||||
1. **No email field**: Uses empty string as fallback
|
||||
2. **None email**: Converted to empty string
|
||||
3. **Custom user models**: Uses `get_email_field_name()`
|
||||
|
||||
## Security Analysis
|
||||
|
||||
### Vulnerabilities Mitigated
|
||||
- [x] Password reset token reuse after email change
|
||||
- [x] Account takeover via old token after email change
|
||||
|
||||
### What This Does NOT Fix
|
||||
- Email spoofing attacks (requires email system compromise)
|
||||
- Password compromise during reset process
|
||||
- Account compromise before email change
|
||||
|
||||
### Backward Compatibility
|
||||
⚠️ **Breaking Change**: Existing password reset tokens become invalid
|
||||
- This is acceptable because:
|
||||
- Tokens are already time-limited (PASSWORD_RESET_TIMEOUT)
|
||||
- Users will request new tokens as needed
|
||||
- Security benefit outweighs minor UX inconvenience
|
||||
|
||||
## Architecture Notes
|
||||
|
||||
### Why `get_email_field_name()`
|
||||
- Introduced in Django 3.1
|
||||
- Supports custom user models
|
||||
- Allows subclasses to override email field name
|
||||
- Standard Django API for getting email field
|
||||
|
||||
### Why `getattr(user, email_field_name, '')`
|
||||
- Safely accesses email attribute
|
||||
- No AttributeError if field missing
|
||||
- Works with any custom user model
|
||||
- Follows Django conventions
|
||||
|
||||
### Why Include Email
|
||||
- Email is user-facing identifier
|
||||
- Email can be changed (unlike pk)
|
||||
- Email changes should invalidate tokens
|
||||
- Closes real security vulnerability
|
||||
|
||||
## References
|
||||
- **Django Commit**: 7f9e4524d6b23424cf44fbe1bf1f4e70f6bb066e
|
||||
- **Issue**: Changing user's email could invalidate password reset tokens
|
||||
- **Files Modified**:
|
||||
- `django/contrib/auth/tokens.py` (PasswordResetTokenGenerator._make_hash_value)
|
||||
- `tests/auth_tests/test_tokens.py` (TokenGeneratorTest)
|
||||
|
||||
## Summary
|
||||
|
||||
This fix closes a security vulnerability in Django's password reset mechanism by ensuring that password reset tokens become invalid when a user changes their email address. The implementation:
|
||||
|
||||
- ✓ Is minimal and focused
|
||||
- ✓ Supports custom user models
|
||||
- ✓ Handles edge cases gracefully
|
||||
- ✓ Includes comprehensive tests
|
||||
- ✓ Maintains code quality
|
||||
- ✓ Provides clear documentation
|
||||
|
||||
The fix has been thoroughly tested and is ready for deployment.
|
||||
|
||||
## Additional Resources
|
||||
|
||||
For more detailed information, see:
|
||||
- `SOLUTION_SUMMARY.md` - Technical details
|
||||
- `IMPLEMENTATION_PLAN.md` - Step-by-step guide
|
||||
- `DJANGO_FIX_SUMMARY.md` - Feature summary
|
||||
- `test_password_reset_fix.py` - Interactive demonstration
|
||||
Loading…
Add table
Reference in a new issue