Skip to content

Commit

Permalink
Fix critical security issue with misusing the Django Signer API
Browse files Browse the repository at this point in the history
After the code was refactored to use the official Signer class the salt
was passed wrongly as secret key, replacing the SECRET_KEY set in
Django settings file. This allows, with verification enabled, to guess
the signature by a potential attacker very easily. In reset password
case, the guessing by attacker can be mitigated somewhat by using
RESET_PASSWORD_VERIFICATION_ONE_TIME_USE but it leads to a different
signature schema.

This bug affects versions 0.2.*, 0.3.*, 0.4.*
  • Loading branch information
apragacz committed Jun 30, 2019
1 parent dd7a2a5 commit 26d094f
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 3 deletions.
2 changes: 1 addition & 1 deletion rest_registration/verification.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def __init__(self, data):
data[self.TIMESTAMP_FIELD] = get_current_timestamp()
self._data = data
self._salt = self._calculate_salt(data)
self._signer = Signer(self._salt)
self._signer = Signer(salt=self._salt)

def _calculate_signature(self, data):
if self.SIGNATURE_FIELD in data:
Expand Down
21 changes: 21 additions & 0 deletions tests/api/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,27 @@ def test_no_password_ok(self):
)


@override_settings(REST_REGISTRATION=REST_REGISTRATION_WITH_VERIFICATION)
class RegisterSignerTestCase(TestCase):

def test_signer_with_different_secret_keys(self):
user = self.create_test_user(is_active=False)
data_to_sign = {'user_id': user.pk}
secrets = [
'#0ka!t#6%28imjz+2t%l(()yu)tg93-1w%$du0*po)*@l+@+4h',
'feb7tjud7m=91$^mrk8dq&nz(0^!6+1xk)%gum#oe%(n)8jic7',
]
signatures = []
for secret in secrets:
with override_settings(
SECRET_KEY=secret):
signer = RegisterSigner(data_to_sign)
data = signer.get_signed_data()
signatures.append(data[signer.SIGNATURE_FIELD])

assert signatures[0] != signatures[1]


def build_custom_verification_url(signer):
base_url = signer.get_base_url()
signed_data = signer.get_signed_data()
Expand Down
27 changes: 26 additions & 1 deletion tests/api/test_register_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from rest_framework.test import force_authenticate

from rest_registration.api.views.register_email import RegisterEmailSigner
from tests.utils import shallow_merge_dicts
from tests.utils import TestCase, shallow_merge_dicts

from .base import APIViewTestCase

Expand All @@ -18,6 +18,31 @@
}


@override_settings(REST_REGISTRATION=REST_REGISTRATION_WITH_EMAIL_VERIFICATION)
class RegisterEmailSignerTestCase(TestCase):

def test_signer_with_different_secret_keys(self):
email = 'testuser1@example.com'
user = self.create_test_user(is_active=False)
data_to_sign = {
'user_id': user.pk,
'email': email,
}
secrets = [
'#0ka!t#6%28imjz+2t%l(()yu)tg93-1w%$du0*po)*@l+@+4h',
'feb7tjud7m=91$^mrk8dq&nz(0^!6+1xk)%gum#oe%(n)8jic7',
]
signatures = []
for secret in secrets:
with override_settings(
SECRET_KEY=secret):
signer = RegisterEmailSigner(data_to_sign)
data = signer.get_signed_data()
signatures.append(data[signer.SIGNATURE_FIELD])

assert signatures[0] != signatures[1]


class BaseRegisterEmailViewTestCase(APIViewTestCase):

def setUp(self):
Expand Down
23 changes: 22 additions & 1 deletion tests/api/test_reset_password.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from rest_framework import status

from rest_registration.api.views.reset_password import ResetPasswordSigner
from tests.utils import shallow_merge_dicts
from tests.utils import TestCase, shallow_merge_dicts

from .base import APIViewTestCase

Expand All @@ -18,6 +18,27 @@
}


@override_settings(REST_REGISTRATION=REST_REGISTRATION_WITH_RESET_PASSWORD)
class ResetPasswordSignerTestCase(TestCase):

def test_signer_with_different_secret_keys(self):
user = self.create_test_user(is_active=False)
data_to_sign = {'user_id': user.pk}
secrets = [
'#0ka!t#6%28imjz+2t%l(()yu)tg93-1w%$du0*po)*@l+@+4h',
'feb7tjud7m=91$^mrk8dq&nz(0^!6+1xk)%gum#oe%(n)8jic7',
]
signatures = []
for secret in secrets:
with override_settings(
SECRET_KEY=secret):
signer = ResetPasswordSigner(data_to_sign)
data = signer.get_signed_data()
signatures.append(data[signer.SIGNATURE_FIELD])

assert signatures[0] != signatures[1]


@override_settings(
REST_REGISTRATION=REST_REGISTRATION_WITH_RESET_PASSWORD,
)
Expand Down

2 comments on commit 26d094f

@mlissner
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many years have passed, but you may be amused to know that django 4.2 no longer allows positional arguments to signer. See bullet here: https://docs.djangoproject.com/en/4.2/releases/4.2/#id1

@apragacz
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this is amusing, I guess I wasn't the only one bitten by this issue :D (passing salt wrongly).

But kudos go to @peterthomassen for finding that bug in the code (here is detailed security advisory ).

Please sign in to comment.