Skip to content

Commit 87a533b

Browse files
committed
Fix session key security issues
Since the password was set to None, the session hash was always identical and predictable for an attacker. A new random salt is added to replace the password which served this funciton before. Should the new session salt is set be default to a rendom value. Should the salt be set to None for some reason, the `get_session_auth_hash` method will raise a `ValueError`. The password field is now removed from the user model. It will raise a `FieldDoesNotExist` error, should the attribute be access further preventing similar security issues.
1 parent f1e07a1 commit 87a533b

File tree

3 files changed

+59
-0
lines changed

3 files changed

+59
-0
lines changed
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Generated by Django 2.2.1 on 2019-05-27 10:39
2+
3+
from django.db import migrations, models
4+
import django.utils.crypto
5+
6+
7+
class Migration(migrations.Migration):
8+
9+
dependencies = [
10+
('mailauth_user', '0001_initial'),
11+
]
12+
13+
operations = [
14+
migrations.AddField(
15+
model_name='emailuser',
16+
name='session_salt',
17+
field=models.CharField(default=django.utils.crypto.get_random_string, editable=False, max_length=12),
18+
),
19+
]

mailauth/contrib/user/models.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from django.contrib.auth.base_user import BaseUserManager
22
from django.contrib.auth.models import AbstractUser
33
from django.db import models
4+
from django.utils.crypto import salted_hmac, get_random_string
45
from django.utils.translation import ugettext_lazy as _
56

67

@@ -39,6 +40,7 @@ class AbstractEmailUser(AbstractUser):
3940
email = models.EmailField(_('email address'), unique=True, db_index=True)
4041
username = None
4142
password = None
43+
session_salt = models.CharField(max_length=12, editable=False, default=get_random_string, unique=True)
4244

4345
def has_usable_password(self):
4446
return False
@@ -48,6 +50,18 @@ def has_usable_password(self):
4850
class Meta(AbstractUser.Meta):
4951
abstract = True
5052

53+
def get_session_auth_hash(self):
54+
"""
55+
Return an HMAC of the password field.
56+
"""
57+
key_salt = "mailauth.contrib.user.models.EmailUserManager.get_session_auth_hash"
58+
if not self.session_salt:
59+
raise ValueError("'session_salt' must be set")
60+
return salted_hmac(key_salt, self.session_salt).hexdigest()
61+
62+
63+
delattr(AbstractEmailUser, 'password')
64+
5165

5266
class EmailUser(AbstractEmailUser):
5367
class Meta(AbstractEmailUser.Meta):

tests/contrib/auth/test_models.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import pytest
2+
from django.core.exceptions import FieldDoesNotExist
23

34
from mailauth.contrib.user.models import EmailUser
45

@@ -7,6 +8,31 @@ class TestAbstractEmailUser:
78
def test_has_usable_password(self):
89
assert not EmailUser().has_usable_password()
910

11+
def test_get_session_auth_hash__default(self, db):
12+
user = EmailUser(email='spiderman@avengers.com')
13+
14+
assert user.session_salt
15+
assert user.get_session_auth_hash()
16+
17+
def test_get_session_auth_hash__value_error(self, db):
18+
user = EmailUser(email='spiderman@avengers.com', session_salt=None)
19+
20+
with pytest.raises(ValueError) as e:
21+
user.get_session_auth_hash()
22+
23+
assert "'session_salt' must be set" in str(e)
24+
25+
def test_get_session_auth_hash__unique(self, db):
26+
spiderman = EmailUser(email='spiderman@avengers.com')
27+
ironman = EmailUser(email='ironman@avengers.com')
28+
29+
assert spiderman.get_session_auth_hash() != ironman.get_session_auth_hash()
30+
31+
def test_password_field(self):
32+
user = EmailUser(email='spiderman@avengers.com')
33+
with pytest.raises(FieldDoesNotExist):
34+
user.password
35+
1036

1137
class TestEmailUserManager:
1238

0 commit comments

Comments
 (0)