Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3DES implementation #202

Merged
merged 1 commit into from
Apr 20, 2018
Merged

Conversation

avargaRH
Copy link
Contributor

@avargaRH avargaRH commented Jan 22, 2018

This is the very first edition to triple DES. python_tripledes.py contains three classes, so it needs compress to one (optionally isolate the python_des.py file) code.


This change is Reviewable

@avargaRH avargaRH force-pushed the 3DESimplementation branch 6 times, most recently from 5cf7313 to d7d7b1d Compare February 1, 2018 16:13
@tomato42 tomato42 self-requested a review February 2, 2018 13:07
@tomato42 tomato42 added the enhancement new feature to be implemented label Feb 2, 2018
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

quite a few nits, please fix them

test cases are missing

# Sanity checking of arguments.
if IV and len(IV) != self.block_size:
raise ValueError("Invalid Initial Value (IV), must be \
a multiple of " + str(self.block_size) + " bytes")
Copy link
Member

Choose a reason for hiding this comment

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

this will result in an error with a newline, also format() operator should be used for string interpolation:

            raise ValueError("Invalid Initial Value (IV), must be "
                             "a multiple of {0} bytes".format(self.block_size))


self._iv = IV

def getKey(self):
Copy link
Member

Choose a reason for hiding this comment

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

key should be a property

key = self._guardAgainstUnicode(key)
self.__key = key

def getIV(self):
Copy link
Member

Choose a reason for hiding this comment

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

iv should be a property

_baseDes.setKey(self, key)
self.__create_sub_keys()

def __String_to_BitList(self, data):
Copy link
Member

Choose a reason for hiding this comment

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

methods names should not use upper case letters

_baseDes.__init__(self, IV)
self.setKey(key)

def setKey(self, key):
Copy link
Member

Choose a reason for hiding this comment

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

again, should be a property

iteration_adjustment = -1

i = 0
while i < 16:
Copy link
Member

Choose a reason for hiding this comment

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

for i in range(16):

# Error check the data
if not data:
return ''
if len(data) % self.block_size != 0:
Copy link
Member

Choose a reason for hiding this comment

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

!= 0 is redundant, all non zero values are considered True

return ''
if len(data) % self.block_size != 0:
if crypt_type == Des.DECRYPT: # Decryption work on 8 byte blocks
raise ValueError("Invalid data length, data must be \
Copy link
Member

Choose a reason for hiding this comment

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

newlines in error message

raise ValueError("Invalid data length, data must be \
a multiple of " + str(self.block_size) + " \
bytes\n.")
# print "Len of data: %f" % (len(data) / self.block_size)
Copy link
Member

Choose a reason for hiding this comment

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

leftover debug message

result.append(self.__BitList_to_String(processed_block))
i += 8

# print "Lines: %d, cached: %d" % (lines, cached)
Copy link
Member

Choose a reason for hiding this comment

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

leftover debug message

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

i've noted few issues with the test cases but I didn't put comments in all the places those issues are present, none the less, please fix all of them


self.assertEqual(
self.tripleDes.decrypt(b'\x95\xf8\xa5\xe5\xdd\x31\xd9\x00'),
b'\x80\x00\x00\x00\x00\x00\x00\x00')
Copy link
Member

Choose a reason for hiding this comment

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

nit: incorrect indentation, b'\x80\x00\x00\x00\x00\x00\x00\x00' should be on the same level as self.tripleDes

Copy link
Contributor Author

@avargaRH avargaRH Feb 28, 2018

Choose a reason for hiding this comment

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

    self.assertEqual(
        triple_des.decrypt(b'\x95\xf8\xa5\xe5\xdd\x31\xd9\x00'),
        b'\x80\x00\x00\x00\x00\x00\x00\x00')

According to "test_tlslite_utils_rsakey.py", is this really correct?

b'\x01\x01\x01\x01\x01\x01\x01\x01')
IV = b'\x00\x00\x00\x00\x00\x00\x00\x00'

self.tripleDes = Python_TripleDES(key, IV)
Copy link
Member

Choose a reason for hiding this comment

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

why self.trpleDes? why not just triple_des?

def test_3des_vartext_decrypt(self):
#Variable Plaintext Known Answer Test, decrypt one block

key = bytearray(b'\x01\x01\x01\x01\x01\x01\x01\x01' +
Copy link
Member

Choose a reason for hiding this comment

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

nit: redundant '+'

# work with one block messages, all in KO3

#def setUp(self, key, IV):
# self.tripleDes = Python_TripleDES(key, IV)
Copy link
Member

Choose a reason for hiding this comment

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

leftover debug code


self.assertEqual(
self.tripleDes.decrypt(b'\x69\x0f\x5b\x0d\x9a\x26\x93\x9b'),
b'\x01\xa1\xd6\xd0\x39\x77\x67\x42')
Copy link
Member

Choose a reason for hiding this comment

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

nit: incorrect indentation

b'\x80\x00\x00\x00\x00\x00\x00\x00'))

"""
class Test3DES_components(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

commented out code


def __string_to_bitlist(self, data):
"""Turn the string data, into a list of bits (1, 0)'s"""
if PY_VER < 3:
Copy link
Member

Choose a reason for hiding this comment

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

this need to compare to a tuple, not int:

if PY_VER < (3, ):

@@ -31,31 +32,31 @@
# Factory Functions for AES
# **************************************************************************

def createAES(key, IV, implList=None):
def createAES(key, iv, imp_list=None):
Copy link
Member

Choose a reason for hiding this comment

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

this changes API in existing code, please don't do it (if you really want to fix it, use @deprecated_params, example: 1e4c092 in #218, and do it in a separate PR)

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

also please squash the addition of 3des implementation and the test vectors together


# Sanity checking of arguments.
if iv and len(iv) != self.block_size:
raise ValueError("Invalid Initial Value (iv), must be a "
Copy link
Member

Choose a reason for hiding this comment

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

s/Initial Value/Initialization vector/


@iv.setter
def iv(self, iv):
# Set the Initial Value
Copy link
Member

Choose a reason for hiding this comment

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

Initialization Vector

def iv(self, iv):
# Set the Initial Value
if not iv or len(iv) != self.block_size:
raise ValueError("Invalid Initial Value (iv), must be a "
Copy link
Member

Choose a reason for hiding this comment

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

same here

def _guard_against_unicode(self, data):
# Only accept byte strings or ascii unicode values, otherwise
# there is no way to correctly decode the data into bytes.
if PY_VER < (3, ):
Copy link
Member

Choose a reason for hiding this comment

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

either if PY_VER < (3, ) or PY_VER = (sys.version_info[0], ) (line 33) both at the same time won't work

raise ValueError("Can only work with bytes, "
"not Unicode strings.")
else:
if isinstance(data, str):
Copy link
Member

Choose a reason for hiding this comment

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

in python 3 only bytes, bytearray or memoryview objects of them should ever be passed in so I think a DeprecationWarning here is a good idea (if we're as serious about testing inputs as this whole method indicates)

see 042c729 for example of use of warnings.warn (though here it may need to be stacklevel=1, not 2, please verify, we want error to list the file that called the function that called _guard_against_unicode)

self.__key3 = self.__key1
else:
self.__key3 = Des(key[16:], self._iv)
_baseDes.key = key
Copy link
Member

Choose a reason for hiding this comment

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

what's it doing here?

_baseDes.__init__(self, iv)
self.set_key(key)

def set_key(self, key):
Copy link
Member

Choose a reason for hiding this comment

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

this should be a property

Must be 8 bytes in length.
"""
def __init__(self, key, iv=None):
_baseDes.__init__(self, iv)
Copy link
Member

Choose a reason for hiding this comment

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

please use super()


# Override setter methods to work on all 3 keys.

def setiv(self, iv):
Copy link
Member

Choose a reason for hiding this comment

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

this should be a property

bytearray(b'\x80\x00\x00\x00\x00\x00\x00\x00'
b'\x80\x00\x00\x00\x00\x00\x00\x00'
b'\x80\x00\x00\x00\x00\x00\x00\x00')),
bytearray(b'\xa1\x55\xa6\xba\x61\xcf\xda\x01'
Copy link
Member

Choose a reason for hiding this comment

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

nit: incorrect indentation

@tomato42
Copy link
Member

also, while squashing, please rebase on top of current master

@tomato42
Copy link
Member

tomato42 commented Mar 9, 2018

Please, don't use "update branch", it's actively harmful for the workflow we're using. Please rebase.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Please, focus first on making sure the test cases pass, both on python 2 and python 3. Only after that baseline is established work on refactoring the code.

if PY_VER < (3, ):
# Turn the strings into integers. Python 3 uses a bytes
# class, which already has this behaviour.
data = [ord(c) for c in data if isinstance(c, str)]
Copy link
Member

Choose a reason for hiding this comment

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

that doesn't look correct, it won't work if data is a bytearray

@avargaRH avargaRH force-pushed the 3DESimplementation branch 3 times, most recently from 9997615 to 34fd443 Compare March 13, 2018 16:00
tests/tlstest.py Outdated

test_no += 1
#test_no += 1
Copy link
Member

Choose a reason for hiding this comment

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

I've commented it out for testing, not so that we commit it...

That code should either be removed or modified to use some other IMAP service, definitely not commented out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


from tlslite.utils import cryptomath

tripleDESPresent = False
tripleDESPresent = True
Copy link
Member

Choose a reason for hiding this comment

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

comment explaining why it's present would likely be a good idea – backwards compatibility and so on

also, the later re-sets to True probably should be removed

@@ -118,7 +119,7 @@ def createRC4(key, IV, implList=None):
raise NotImplementedError()

#Create a new TripleDES instance
def createTripleDES(key, IV, implList=None):
def createTripleDES(key, IV=None, implList=None):
Copy link
Member

Choose a reason for hiding this comment

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

why changed to IV=None?


# PY_VER is used to handle Python2 and Python3 differences.
PY_VER = sys.version_info
FLAG = 1
Copy link
Member

Choose a reason for hiding this comment

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

FLAG ??


# Sanity checking of arguments.
if not iv:
raise ValueError("Initialization Vector (iv) must be supplied")
Copy link
Member

Choose a reason for hiding this comment

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

if iv must be supplied, why a). it's initialised to None, b). why that check is performed in 3 places?

b'\x7c\xa1\x10\x45\x4a\x1a\x6e\x57'),
b'\x55\xfe\x07\x2a\x73\x51\xa5')

def test_bad_key_size(self):
Copy link
Member

Choose a reason for hiding this comment

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

same here as with the IV above

with self.assertRaises(ValueError):
Python_TripleDES(key, iv).decrypt(u'aáäbcčdďeéfghiíjklĺľmnňo')

def test_1des_bad_key(self):
Copy link
Member

Choose a reason for hiding this comment

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

again, why bad?

b'\x7c\xa1\x10\x45\x4a\x1a\x6e\x57')
iv = b'\x55\xfe\x07\x2a\x73\x51\xa5\xc8'

Python_TripleDES(key, iv).encrypt()
Copy link
Member

Choose a reason for hiding this comment

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

assert missing

b'\x7c\xa1\x10\x45\x4a\x1a\x6e\x57')
iv = b'\x55\xfe\x07\x2a\x73\x51\xa5\xc8'

Python_TripleDES(key, iv).decrypt()
Copy link
Member

Choose a reason for hiding this comment

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

assert missing

Python_TripleDES(key, iv).encrypt('161514131211109876543')

with self.assertRaises(ValueError):
Python_TripleDES(key, iv).decrypt('161514131211109876543')
Copy link
Member

Choose a reason for hiding this comment

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

this should be a separate test case, raising an exception generally is considered to leave the object in undefined state, so reusing the object for the test case may hide some behaviour

tests/tlstest.py Outdated
@@ -790,12 +790,6 @@ def connect():

print("Test {0} - Internet servers test".format(test_no))
try:
i = IMAP4_TLS("cyrus.andrew.cmu.edu")
Copy link
Member

Choose a reason for hiding this comment

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

I said "in a separate patch", but that I mean, it needs to be the only change in that path and the message of that patch needs to explain why that change is happening


from tlslite.utils import cryptomath

tripleDESPresent = False
tripleDESPresent = True
# 3DES cipher usage for backwards compatibility with older openSSL versions
Copy link
Member

Choose a reason for hiding this comment

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

doc strings of variables/constants need to be strings (""")

what older versions of OpenSSL?


# Sanity checking of arguments.
if not iv:
raise ValueError("Initialization Vector (iv) must be supplied")
Copy link
Member

Choose a reason for hiding this comment

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

the check is still being performed in 3 places: 45, 50 and 52

@property
def iv(self):
# return bytes
def get_iv(self):
Copy link
Member

Choose a reason for hiding this comment

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

did I say "change it back to a getter"?

@@ -280,53 +271,52 @@ def __permutate(self, table, block):
"""Permutate this block with the specified table"""
return [block[x] for x in table]

# Transform the secret key, so that it is ready for data processing
# Create the 16 subkeys, k[1] - k[16]
"""Transform the secret key, so that it is ready for data processing"""
Copy link
Member

Choose a reason for hiding this comment

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

that's not a valid doc string

with self.assertRaises(ValueError):
Python_TripleDES(b'\x7c\xa1\x10\x45\x4a\x1a\x6e\x57',
b'\x55\xfe\x07\x2a\x73\x51\xa5\x00')
if PY_VER > (3, ):
Copy link
Member

Choose a reason for hiding this comment

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

the canonical way is to use @unittest.skipIf() decorator: https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures

with self.assertRaises(ValueError):
Python_TripleDES(b'\x7c\xa1\x10\x45\x4a\x1a\x6e\x57',
b'\x55\xfe\x07\x2a\x73\x51\xa5\x00')
if PY_VER > (3, ):
# DeprecationWarning check should apply only on python3
Copy link
Member

Choose a reason for hiding this comment

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

we also need to verify that on Python2 no warnings were emitted (example code that needs to be modified to assert that there were 0 warnings)

PY_VER = sys.version_info


def new(key, iv):
Copy link
Member

Choose a reason for hiding this comment

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

it seems to me like this is still missing test coverage

@avargaRH avargaRH force-pushed the 3DESimplementation branch 2 times, most recently from c11423f to ad72b4b Compare April 4, 2018 08:39
@avargaRH avargaRH force-pushed the 3DESimplementation branch 3 times, most recently from b9d8cc6 to 1cf6167 Compare April 6, 2018 14:45
@tomato42
Copy link
Member

tomato42 commented Apr 9, 2018

please fix that python 3.2 issue

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

@avargaRH avargaRH force-pushed the 3DESimplementation branch 3 times, most recently from 61dc435 to 220f8a7 Compare April 16, 2018 07:21
warnings.warn("Only bytes, bytearray or memoryview "
"objects of them should be passed",
DeprecationWarning,
stacklevel=1)
Copy link
Member

Choose a reason for hiding this comment

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

given that this emits errors like this:

/home/travis/build/tomato42/tlslite-ng/tlslite/utils/python_tripledes.py:74: DeprecationWarning: Only bytes, bytearray or memoryview objects of them should be passed
  stacklevel=1)

1 here is not correct, will likely need 3 – we want the error to be emitted from user code, not this module's code


@unittest.skipIf(PY_VER < (3, ), "Unicode string syntax on py2: u'ľáľä'")
def test1_py3_unicode_instance(self):
with self.assertRaises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

this should also assert a warning (so two with, one inside the other) otherwise it pollutes the output of unittest run

key = b"\xaa"*24
iv = b"\xbb"*8

with self.assertRaises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

same here, assert for warning missing, but this assert for exception should stay

key = b"\xaa"*24
iv = b"\xbb"*8

with self.assertRaises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

same here, assert for warning missing

iv = b'\x55\xfe\x07\x2a\x73\x51\xa5\xc8'

with self.assertRaises(ValueError):
Python_TripleDES(key, iv).decrypt('161514131211109876543')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be b'1615...?

iv = b'\x55\xfe\x07\x2a\x73\x51\xa5\xc8'

with self.assertRaises(ValueError):
Python_TripleDES(key, iv).encrypt('161514131211109876543')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be b'161514...?


@unittest.skipIf(PY_VER < (3, ), "Unicode string syntax on py2: u'ľáľä'")
def test2_py3_unicode_instance(self):
with self.assertRaises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

assert for warning missing

Supports CBC (Cypher Block Chaining) mode.
"""
def __init__(self, key, iv=None):
super(Python_TripleDES, self).__init__(iv)
Copy link
Member

Choose a reason for hiding this comment

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

so the problem with this is that if we fix the stacklevel to 3 in warn, it will report this line as the problematic one

we can fix it in two ways, either change the _guard_against_unicode() to detect this case, or call _guard_against_unicode() on iv from here, before calling super(), the latter is likely easier to do

@tomato42
Copy link
Member

Reviewed 2 of 5 files at r12, 2 of 2 files at r13.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tomato42 tomato42 merged commit 403539b into tlsfuzzer:master Apr 20, 2018
@tomato42
Copy link
Member

@avargaRH thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature to be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants