Skip to content

Commit 21d608c

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 21d608c

File tree

3 files changed

+60
-0
lines changed

3 files changed

+60
-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+
import django.utils.crypto
4+
from django.db import migrations, models
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: 15 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 get_random_string, salted_hmac
45
from django.utils.translation import ugettext_lazy as _
56

67

@@ -39,6 +40,10 @@ 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(
44+
max_length=12, editable=False,
45+
default=get_random_string,
46+
)
4247

4348
def has_usable_password(self):
4449
return False
@@ -48,6 +53,16 @@ def has_usable_password(self):
4853
class Meta(AbstractUser.Meta):
4954
abstract = True
5055

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

5267
class EmailUser(AbstractEmailUser):
5368
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='[email protected]')
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='[email protected]', 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='[email protected]')
27+
ironman = EmailUser(email='[email protected]')
28+
29+
assert spiderman.get_session_auth_hash() != ironman.get_session_auth_hash()
30+
31+
def test_password_field(self):
32+
user = EmailUser(email='[email protected]')
33+
with pytest.raises(FieldDoesNotExist):
34+
user.password
35+
1036

1137
class TestEmailUserManager:
1238

0 commit comments

Comments
 (0)