Skip to content

Commit

Permalink
[bandit] Use SHA256 instead of MD5 and SHA1
Browse files Browse the repository at this point in the history
Issue: [B303:blacklist] Use of insecure MD2, MD4, MD5 or SHA1 hash
       function.

Fix: switch to SHA256 and use the helper checksum() function to
     calculate all hashes

NEW DATABASE MIGRATION:
    - increase checksum fields size to hold the new checksum values
  • Loading branch information
atodorov committed Apr 13, 2018
1 parent 32d7df5 commit e17c071
Show file tree
Hide file tree
Showing 7 changed files with 72 additions and 22 deletions.
8 changes: 4 additions & 4 deletions tcms/core/contrib/auth/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
import datetime
import random

from hashlib import sha1

from django.db import models
from django.conf import settings

from tcms.core.utils.checksum import checksum


class UserActivateKey(models.Model):
user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE)
Expand All @@ -19,8 +19,8 @@ class Meta:

@classmethod
def set_random_key_for_user(cls, user, force=False):
salt = sha1(str(random.random()).encode('utf-8')).hexdigest()[:5]
activation_key = sha1((salt + user.username).encode('utf-8')).hexdigest()
salt = checksum(str(random.random()))[:5]
activation_key = checksum(salt + user.username)

# Create and save their profile
user_activation_key, created = cls.objects.get_or_create(user=user)
Expand Down
19 changes: 9 additions & 10 deletions tcms/core/contrib/auth/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# pylint: disable=invalid-name

import datetime
from hashlib import sha1
from mock import patch

from django.urls import reverse
Expand All @@ -12,6 +11,7 @@
from django.test import TestCase, override_settings

from tcms import signals
from tcms.core.utils.checksum import checksum
from .models import UserActivateKey


Expand Down Expand Up @@ -40,9 +40,8 @@ def test_set_random_key(self, random, mock_datetime):

self.assertEqual(self.new_user, activation_key.user)

s_random = sha1(str(fake_random).encode('utf-8')).hexdigest()[:5]
expected_key = sha1('{}{}'.format(
s_random, self.new_user.username).encode('utf-8')).hexdigest()
s_random = checksum(str(fake_random))[:5]
expected_key = checksum(s_random + self.new_user.username)
self.assertEqual(expected_key, activation_key.activation_key)

self.assertEqual(datetime.datetime(2017, 5, 17),
Expand Down Expand Up @@ -114,8 +113,8 @@ def test_open_registration_page(self):

def assert_user_registration(self, username, follow=False):

with patch('tcms.core.contrib.auth.models.sha1') as sha_1:
sha_1.return_value.hexdigest.return_value = self.fake_activate_key
with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = self.fake_activate_key

response = self.client.post(self.register_url,
{'username': username,
Expand Down Expand Up @@ -229,8 +228,8 @@ def test_fail_if_activation_key_does_not_exist(self):
def test_fail_if_activation_key_expired(self):
fake_activation_key = 'secret-activation-key'

with patch('tcms.core.contrib.auth.models.sha1') as sha_1:
sha_1.return_value.hexdigest.return_value = fake_activation_key
with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = fake_activation_key
key = UserActivateKey.set_random_key_for_user(self.new_user)
key.key_expires = datetime.datetime.now() - datetime.timedelta(days=10)
key.save()
Expand All @@ -247,8 +246,8 @@ def test_fail_if_activation_key_expired(self):
def test_confirm(self):
fake_activate_key = 'secret-activate-key'

with patch('tcms.core.contrib.auth.models.sha1') as sha_1:
sha_1.return_value.hexdigest.return_value = fake_activate_key
with patch('tcms.core.contrib.auth.models.checksum') as _checksum:
_checksum.return_value = fake_activate_key
UserActivateKey.set_random_key_for_user(self.new_user)

confirm_url = reverse('tcms-confirm',
Expand Down
6 changes: 3 additions & 3 deletions tcms/core/utils/checksum.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
def checksum(value):
if value is None:
return ''
md5 = hashlib.md5()
md5.update(value.encode("UTF-8"))
return md5.hexdigest()
_checksum = hashlib.sha256()
_checksum.update(value.encode("UTF-8"))
return _checksum.hexdigest()
33 changes: 33 additions & 0 deletions tcms/testcases/migrations/0017_increase_checksum_fields_size.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Generated by Django 2.0.4 on 2018-04-13 08:31

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('testcases', '0016_duration_field'),
]

operations = [
migrations.AlterField(
model_name='testcasetext',
name='action_checksum',
field=models.CharField(max_length=64),
),
migrations.AlterField(
model_name='testcasetext',
name='breakdown_checksum',
field=models.CharField(max_length=64),
),
migrations.AlterField(
model_name='testcasetext',
name='effect_checksum',
field=models.CharField(max_length=64),
),
migrations.AlterField(
model_name='testcasetext',
name='setup_checksum',
field=models.CharField(max_length=64),
),
]
8 changes: 4 additions & 4 deletions tcms/testcases/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,10 @@ class TestCaseText(TCMSActionModel):
effect = models.TextField(blank=True)
setup = models.TextField(blank=True)
breakdown = models.TextField(blank=True)
action_checksum = models.CharField(max_length=32)
effect_checksum = models.CharField(max_length=32)
setup_checksum = models.CharField(max_length=32)
breakdown_checksum = models.CharField(max_length=32)
action_checksum = models.CharField(max_length=64)
effect_checksum = models.CharField(max_length=64)
setup_checksum = models.CharField(max_length=64)
breakdown_checksum = models.CharField(max_length=64)

class Meta:
db_table = u'test_case_texts'
Expand Down
18 changes: 18 additions & 0 deletions tcms/testplans/migrations/0012_increase_checksum_field_size.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 2.0.4 on 2018-04-13 08:31

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('testplans', '0011_remove_testplanemailsettings_notify_on_plan_delete'),
]

operations = [
migrations.AlterField(
model_name='testplantext',
name='checksum',
field=models.CharField(max_length=64),
),
]
2 changes: 1 addition & 1 deletion tcms/testplans/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ class TestPlanText(TCMSActionModel):
create_date = models.DateTimeField(auto_now_add=True,
db_column='creation_ts')
plan_text = models.TextField(blank=True)
checksum = models.CharField(max_length=32)
checksum = models.CharField(max_length=64)

class Meta:
db_table = u'test_plan_texts'
Expand Down

0 comments on commit e17c071

Please sign in to comment.