diff --git a/reviewboard/scmtools/crypto_utils.py b/reviewboard/scmtools/crypto_utils.py index 924ce84e4d10d65b9eb242c92f84a97aedd6ffaf..025a24a66c6161b33b6fa921d993537a8b3a069c 100644 --- a/reviewboard/scmtools/crypto_utils.py +++ b/reviewboard/scmtools/crypto_utils.py @@ -10,6 +10,7 @@ from django.conf import settings from django.utils import six from reviewboard.deprecation import RemovedInReviewBoard40Warning +from reviewboard.scmtools.errors import DecryptPasswordError AES_BLOCK_SIZE = algorithms.AES.block_size / 8 @@ -176,7 +177,15 @@ def decrypt_password(encrypted_password, key=None): ValueError: The encryption key was not in the right format. """ - return aes_decrypt(base64.b64decode(encrypted_password), key=key) + password = None + try: + password = aes_decrypt(base64.b64decode(encrypted_password), key=key) + # Check if the password can be interpreted as UTF-8. + password.decode('utf-8') + except (ValueError, TypeError, UnicodeDecodeError): + raise DecryptPasswordError() + + return password # The following are deprecated. They're likely not used anywhere, but we diff --git a/reviewboard/scmtools/errors.py b/reviewboard/scmtools/errors.py index 2ed4a05b1bfc19776e77d34fb9c91941f9b634ba..94386b023a31357f2af8704b5a4f6138c0693cd5 100644 --- a/reviewboard/scmtools/errors.py +++ b/reviewboard/scmtools/errors.py @@ -110,3 +110,12 @@ class UnverifiedCertificateError(SCMError): SCMError.__init__(self, _('A verified SSL certificate is required ' 'to connect to this repository.')) self.certificate = certificate + + +class DecryptPasswordError(SCMError): + """An error indicating that a password could not be decryted.""" + def __init__(self): + SCMError.__init__(self, _( + 'Unable to decode the password for repository. This may be caused ' + 'by a change in settings.SECRET_KEY or bad encoded data from a ' + 'database import.')) diff --git a/reviewboard/scmtools/models.py b/reviewboard/scmtools/models.py index 96d3a7569fb465332cc5efc7ce1e0a6f4102728a..7c25a2ff8850661139eb613ea3e6d300aa2491c9 100644 --- a/reviewboard/scmtools/models.py +++ b/reviewboard/scmtools/models.py @@ -26,6 +26,7 @@ from reviewboard.hostingsvcs.models import HostingServiceAccount from reviewboard.hostingsvcs.service import get_hosting_service from reviewboard.scmtools.crypto_utils import (decrypt_password, encrypt_password) +from reviewboard.scmtools.errors import DecryptPasswordError from reviewboard.scmtools.managers import RepositoryManager, ToolManager from reviewboard.scmtools.signals import (checked_file_exists, checking_file_exists, @@ -221,7 +222,11 @@ class Repository(models.Model): password = password[len(self.ENCRYPTED_PASSWORD_PREFIX):] if password: - password = decrypt_password(password).decode('utf-8') + try: + password = decrypt_password(password).decode('utf-8') + except DecryptPasswordError as e: + logging.exception(e) + password = None else: password = None else: @@ -331,7 +336,12 @@ class Repository(models.Model): if self.hosting_account and self.hosting_account.service: username = username or self.hosting_account.username - password = password or self.hosting_account.service.get_password() + try: + if not password: + password = self.hosting_account.service.get_password() + except DecryptPasswordError as e: + logging.exception(e) + password = None return { 'username': username, diff --git a/reviewboard/scmtools/tests/test_crypto_utils.py b/reviewboard/scmtools/tests/test_crypto_utils.py index a46a619180be56bd3efd6167f5bd6d42198aa5b9..0e6f9849115c5009b520ad279c56068a8deb8956 100644 --- a/reviewboard/scmtools/tests/test_crypto_utils.py +++ b/reviewboard/scmtools/tests/test_crypto_utils.py @@ -12,6 +12,7 @@ from reviewboard.scmtools.crypto_utils import (aes_decrypt, encrypt, encrypt_password, get_default_aes_encryption_key) +from reviewboard.scmtools.errors import DecryptPasswordError from reviewboard.testing.testcase import TestCase @@ -107,6 +108,34 @@ class CryptoUtilsTests(TestCase): self.assertEqual(decrypt_password(encrypted, key=self.CUSTOM_KEY), self.PLAIN_TEXT) + def test_decrypt_password_with_short_key(self): + """Testing decrypt_password with too short key""" + # The encrypted value was made with PyCrypto, to help with + # compatibility testing from older installs. + encrypted = b'/pOO3VWHRXd1ZAeHZo8MBGQsNClD4lS7XK9WAydt8zW/ob+e63E=' + short_key = b'0123456789' # -> ValueError + + self.assertRaises(DecryptPasswordError, + decrypt_password, encrypted, key=short_key) + + def test_decrypt_password_with_wrong_key(self): + """Testing decrypt_password with wrong key""" + # The encrypted value was made with PyCrypto, to help with + # compatibility testing from older installs. + encrypted = b'/pOO3VWHRXd1ZAeHZo8MBGQsNClD4lS7XK9WAydt8zW/ob+e63E=' + wrong_key = b'abcdef0123456789' # -> UnicodeDecodeError + + self.assertRaises(DecryptPasswordError, + decrypt_password, encrypted, key=wrong_key) + + def test_decrypt_password_with_invalid_data(self): + """Testing decrypt_password with invalid data""" + # "!" is not part of the base-64 alphabet -> TypeError + encrypted = b'!pOO3VWHRXd1ZAeHZo8MBGQsNClD4lS7XK9WAydt8zW/ob+e63E=' + + self.assertRaises(DecryptPasswordError, + decrypt_password, encrypted, key=self.CUSTOM_KEY) + def test_encrypt_password(self): """Testing encrypt_password""" # The encrypted value will change every time, since the iv changes, diff --git a/reviewboard/scmtools/tests/test_repository.py b/reviewboard/scmtools/tests/test_repository.py index 217a421f5740f61759a29b26fe21ce84dec718e8..2feae0d4a02196282f0dc60080e212f48aca7484 100644 --- a/reviewboard/scmtools/tests/test_repository.py +++ b/reviewboard/scmtools/tests/test_repository.py @@ -2,10 +2,15 @@ from __future__ import unicode_literals import os +from mock import Mock +from nose.plugins.logcapture import LogCapture from django.core.cache import cache from django.core.exceptions import ValidationError +from django.utils.translation import ugettext as _ +from reviewboard.hostingsvcs.models import HostingServiceAccount from reviewboard.scmtools.core import HEAD +from reviewboard.scmtools.errors import DecryptPasswordError from reviewboard.scmtools.models import Repository, Tool from reviewboard.scmtools.signals import (checked_file_exists, checking_file_exists, @@ -335,3 +340,58 @@ class RepositoryTests(TestCase): tool=Tool.objects.get(name='Git')) self.assertEqual(len(self.repository.name), 255) + + def test_password(self): + """Testing Repository.password""" + password = "P4ssw0rd!" + self.repository = Repository.objects.create(name='New test repo', + path=self.repository.path, + tool=self.repository.tool) + self.repository.password = password + + self.assertEqual(self.repository.password, password) + + def test_password_with_invalid_data(self): + """Testing Repository.password with invalid data""" + invalid = '\t!ZshCbwfSv5kOae46Dihj4xzvu8Cedw59Q==' + self.repository = Repository.objects.create(name='New test repo', + path=self.repository.path, + tool=self.repository.tool, + encrypted_password=invalid) + lc = LogCapture() + lc.begin() + password = self.repository.password + lc.end() + + self.assertEqual(password, None) + log_msg = _( + 'Unable to decode the password for repository. This may be caused ' + 'by a change in settings.SECRET_KEY or bad encoded data from a ' + 'database import.') + self.assertIn(log_msg, lc.handler.buffer[0]) + + def test_get_credentials_with_hosting_service_decrypt_password_error(self): + """Testing Repository.get_credentials() with hosting service unable + to decrypt password""" + invalid = '\t!ZshCbwfSv5kOae46Dihj4xzvu8Cedw59Q==' + hosting = HostingServiceAccount.objects.create(service_name='test', + username='myuser') + hosting._service = Mock() + hosting._service.get_password = Mock( + side_effect=DecryptPasswordError()) + self.repository = Repository.objects.create(name='New test repo', + path=self.repository.path, + tool=self.repository.tool, + encrypted_password=invalid, + hosting_account=hosting) + lc = LogCapture() + lc.begin() + credentials = self.repository.get_credentials() + lc.end() + + self.assertEqual(credentials['password'], None) + log_msg = _( + 'Unable to decode the password for repository. This may be caused ' + 'by a change in settings.SECRET_KEY or bad encoded data from a ' + 'database import.') + self.assertIn(log_msg, lc.handler.buffer[0])