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 fixes #377

Merged
merged 3 commits into from
Dec 2, 2019
Merged

3des fixes #377

merged 3 commits into from
Dec 2, 2019

Conversation

t184256
Copy link
Collaborator

@t184256 t184256 commented Nov 27, 2019

Two 3DES fixes - one for the Python implementation, one for the M2Crypto one - and a new test.

commit 5c4078185500f835a12423c6bd5b863681c44b08
    Fix IV handling for 3DES, test consecutive calls

    Correctly set IV for consecutive 3DES encrypt/decrypt invocations.
    Test that the results of three invocations yield the same result
    as a combined single one, cross-check this across three implementations.
commit 8f1fa9bec91ca8c83c77142a3ffb6a5c78e73586
    Streamline M2Crypto 3DES IV handling

    M2Crypto defaults to padding the ciphertext,
    and the previous implementation danced around that awkwardly
    by padding and unpadding ciphertext on decryption
    and updating the IV manually.
    `m2.cipher_set_padding(context, 0)`
    allows to shoulder the IV handling back to where it belongs
    and to get rid of unnecessary context reinitializations.
    (Investigative work courtesy of @tomato42:
     https://github.com/tomato42/tlslite-ng/pull/377#pullrequestreview-323893850)
commit 69c4aac9c7e35becef17ca0de9aaed694d12dcb1
    Disable test timings analysis in 3DES split tests

    The implementation is slow and (re)execution time varies greatly,
    making hypothesis error out in CI.
    This change disables the timings analysis
    for tests in `test_tlslite_utils_tripledes_split.py`.

This change is Reviewable

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.

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t184256)

a discussion (no related file):
could you rebase on top of master?


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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @t184256)


tlslite/utils/openssl_tripledes.py, line 48 at r2 (raw file):

            m2.cipher_ctx_free(context)
            if ciphertext:
                self.IV = ciphertext[-self.block_size:]

ok, so actually the issue is because m2crypto defaults to padding of the ciphertext (and with padding, it requires a call to m2.cipher_final(context) to give out the last block of data)

we need to do m2.cipher_set_padding(context, 0) after cipher_init then:
a). IV should get handled automatically by m2crypto
b). we won't need to re-init the context for every new block processed

that also means we will need def __del__ with a call to m2.cipher_ctx_free(context)

t184256 added a commit to t184256/tlslite-ng that referenced this pull request Nov 27, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work cortesy of @tomato42:
 tlsfuzzer#377 (review))
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Nov 27, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review))
Copy link
Collaborator Author

@t184256 t184256 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @tomato42)

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

could you rebase on top of master?

Done.



tlslite/utils/openssl_tripledes.py, line 48 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

ok, so actually the issue is because m2crypto defaults to padding of the ciphertext (and with padding, it requires a call to m2.cipher_final(context) to give out the last block of data)

we need to do m2.cipher_set_padding(context, 0) after cipher_init then:
a). IV should get handled automatically by m2crypto
b). we won't need to re-init the context for every new block processed

that also means we will need def __del__ with a call to m2.cipher_ctx_free(context)

Done, thanks for solving that.

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.

Given that we are testing every commit in turn, the @settings needs to be added at the same time as we start using hypothesis...

Reviewed 2 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t184256)


tlslite/utils/openssl_tripledes.py, line 22 at r4 (raw file):

            self.decrypt_context = m2.cipher_ctx_new()
            m2.cipher_init(self.encrypt_context, cipherType, key, IV, 1)
            m2.cipher_init(self.decrypt_context, cipherType, key, IV, 0)

that feels wasteful though, I wonder if it wouldn't be better to have a self._context that defaults to None and we initialise it on first encrypt or decrypt as necessary...

t184256 added a commit to t184256/tlslite-ng that referenced this pull request Dec 2, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review))
Copy link
Collaborator Author

@t184256 t184256 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @tomato42)


tlslite/utils/openssl_tripledes.py, line 22 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

that feels wasteful though, I wonder if it wouldn't be better to have a self._context that defaults to None and we initialise it on first encrypt or decrypt as necessary...

Done.

t184256 added a commit to t184256/tlslite-ng that referenced this pull request Dec 2, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review))
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.

Reviewed 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @t184256)


tlslite/utils/openssl_tripledes.py, line 27 at r8 (raw file):

                           int(encrypt))
            m2.cipher_set_padding(self._context, 0)
            self._encrypt = encrypt

all fields should be initialised in __init__


tlslite/utils/openssl_tripledes.py, line 43 at r8 (raw file):

            else:
                assert not self._encrypt, \
                       '.decrypt() not allowed after .decrypt()'

typo: '.decrypt() not allowed after .encrypt()'


unit_tests/test_tlslite_utils_tripledes_split.py, line 69 at r7 (raw file):

    @unittest.skipIf(not cryptomath.pycryptoLoaded, "requires pycrypto")
    @_given
    @settings(deadline=None)

shouldn't this be gated behind sys.version_info too? (just because we don't use pycrypto with py2.6 on Travis doesn't mean it's an unsupported configuration)

t184256 added a commit to t184256/tlslite-ng that referenced this pull request Dec 2, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review))
Copy link
Collaborator Author

@t184256 t184256 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @tomato42)


tlslite/utils/openssl_tripledes.py, line 27 at r8 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

all fields should be initialised in __init__

OK.


tlslite/utils/openssl_tripledes.py, line 43 at r8 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

typo: '.decrypt() not allowed after .encrypt()'

Fixed.


unit_tests/test_tlslite_utils_tripledes_split.py, line 69 at r7 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

shouldn't this be gated behind sys.version_info too? (just because we don't use pycrypto with py2.6 on Travis doesn't mean it's an unsupported configuration)

Thanks for spotting that! It was a simple omission, no need to search for hidden motives here =)

tomato42
tomato42 previously approved these changes Dec 2, 2019
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.

Reviewed 2 of 2 files at r9, 2 of 2 files at r10, 1 of 1 files at r11.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@tomato42 tomato42 added bug unintented behaviour in tlslite-ng code maintenance issues related to project maintenance, CI, documentation, etc. labels Dec 2, 2019
@tomato42 tomato42 added this to the v0.8.0 milestone Dec 2, 2019
Correctly set IV for consecutive 3DES encrypt/decrypt invocations.
Test that the results of three invocations yield the same result
as a combined single one, cross-check this across three implementations.
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review))
Make M2Crypto 3DES follow suit
and require separate instances for encryption and decryption.
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.

Reviewed 2 of 2 files at r12, 2 of 2 files at r13, 1 of 1 files at r14.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@tomato42
Copy link
Member

tomato42 commented Dec 2, 2019

Looks good, thanks!

@tomato42 tomato42 merged commit d62e1d3 into tlsfuzzer:master Dec 2, 2019
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Dec 18, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
@t184256 t184256 mentioned this pull request Dec 18, 2019
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Dec 18, 2019
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Jan 13, 2020
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Jan 13, 2020
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Jan 13, 2020
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Jan 13, 2020
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
t184256 added a commit to t184256/tlslite-ng that referenced this pull request Jan 15, 2020
M2Crypto defaults to padding the ciphertext,
and the previous implementation danced around that awkwardly
by padding and unpadding ciphertext on decryption
and updating the IV manually.
`m2.cipher_set_padding(context, 0)`
allows to shoulder the IV handling back to where it belongs
and to get rid of unnecessary context reinitializations.
(Investigative work courtesy of @tomato42:
 tlsfuzzer#377 (review)).

This commit mirrors changes from tlsfuzzer#377, but for AES
and adds the same unit test as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintented behaviour in tlslite-ng code maintenance issues related to project maintenance, CI, documentation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants