From 26d094fab65ea8c2694fdfb6a3ab95a7808b62d5 Mon Sep 17 00:00:00 2001 From: Andrzej Pragacz Date: Mon, 1 Jul 2019 00:48:17 +0200 Subject: [PATCH] Fix critical security issue with misusing the Django Signer API 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.* --- rest_registration/verification.py | 2 +- tests/api/test_register.py | 21 +++++++++++++++++++++ tests/api/test_register_email.py | 27 ++++++++++++++++++++++++++- tests/api/test_reset_password.py | 23 ++++++++++++++++++++++- 4 files changed, 70 insertions(+), 3 deletions(-) diff --git a/rest_registration/verification.py b/rest_registration/verification.py index 52c99d8..c56c861 100644 --- a/rest_registration/verification.py +++ b/rest_registration/verification.py @@ -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: diff --git a/tests/api/test_register.py b/tests/api/test_register.py index 5317828..c07d49b 100644 --- a/tests/api/test_register.py +++ b/tests/api/test_register.py @@ -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() diff --git a/tests/api/test_register_email.py b/tests/api/test_register_email.py index 1b0fbed..ac1dbc6 100644 --- a/tests/api/test_register_email.py +++ b/tests/api/test_register_email.py @@ -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 @@ -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): diff --git a/tests/api/test_reset_password.py b/tests/api/test_reset_password.py index c74e2ce..62e27db 100644 --- a/tests/api/test_reset_password.py +++ b/tests/api/test_reset_password.py @@ -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 @@ -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, )