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

check if the ciphers are supported by m2crypto before using them #411

Merged

Conversation

inikolcev
Copy link
Collaborator

@inikolcev inikolcev commented Jun 3, 2020

fixes #405

It looks like AES-CTR was introduced in m2crypto version 0.25.0
This adds a method to check if the ciphers are supported and if not, it falls back to the python implementation for that cipher.


This change is Reviewable

@inikolcev inikolcev requested a review from tomato42 June 3, 2020 17:06
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 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @inikolcev)


tlslite/utils/openssl_aes.py, line 12 at r1 (raw file):

if m2cryptoLoaded:

    def check_cipher_support(mode):

why here not in tlslite/utils/cipherfactory.py?
this check is not free, couldn't we do it once?

@inikolcev inikolcev force-pushed the check_m2crypto_supported_ciphers branch from 3df9309 to 4d22cc9 Compare June 5, 2020 10:46
@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging 4d22cc9 into 7c6fbf9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

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 r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @inikolcev)


tlslite/utils/cryptomath.py, line 34 at r2 (raw file):

    m2cryptoLoaded = True
    m2cryptoAesCBC = False
    m2cryptoAesCTR = False

as pylint points out, the this is a constant, so it should use all-caps name


tlslite/utils/cryptomath.py, line 36 at r2 (raw file):

    m2cryptoAesCTR = False
    if hasattr(m2, 'aes_192_cbc'):
        m2cryptoAesCBC = True

are there really versions of m2crypto that don't support cbc? why we're looking for aes_192 when no tls cipher uses aes_192?

@inikolcev
Copy link
Collaborator Author

inikolcev commented Jun 5, 2020

as pylint points out, the this is a constant, so it should use all-caps name

Yep, will fix it.

are there really versions of m2crypto that don't support cbc? why we're looking for aes_192 when no tls cipher uses aes_192?

I'm not sure actually if there are versions without cbc support, didn't see anything in the m2crypto changelog. We use it heavily so I figured it won't hurt to have a check for it too. Do you think it is not needed?

I don't think it matters in which key size we are looking, if one is supported they all are.

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.

We use it heavily so I figured it won't hurt to have a check for it too. Do you think it is not needed?

it's a fine cross-check, but if AES-CBC is not there, I think we can assume that it's not real m2crypto ans simply say that we didn't detect m2crypto, so the fallback to python implementation happens earlier

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

@inikolcev inikolcev force-pushed the check_m2crypto_supported_ciphers branch from 4d22cc9 to c47d290 Compare June 5, 2020 13:41
@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging c47d290 into b60b6d1 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

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 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @inikolcev)

tomato42
tomato42 previously approved these changes Jun 5, 2020
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: :shipit: complete! all files reviewed, all discussions resolved

@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging 5d498f7 into b60b6d1 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

tomato42
tomato42 previously approved these changes Jun 5, 2020
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 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging 3ee3b48 into b60b6d1 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

tomato42
tomato42 previously approved these changes Jun 5, 2020
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 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging e7b276c into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

tomato42
tomato42 previously approved these changes Jun 5, 2020
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 1 of 1 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lgtm-com
Copy link

lgtm-com bot commented Jun 5, 2020

This pull request fixes 1 alert when merging c86b766 into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

tomato42
tomato42 previously approved these changes Jun 5, 2020
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 1 of 1 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging d56bae0 into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@inikolcev inikolcev force-pushed the check_m2crypto_supported_ciphers branch from d56bae0 to 94661a3 Compare June 7, 2020 22:41
@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging 94661a3 into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@inikolcev inikolcev force-pushed the check_m2crypto_supported_ciphers branch from 94661a3 to 5984f53 Compare June 7, 2020 22:53
@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2020

This pull request fixes 1 alert when merging 5984f53 into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@inikolcev inikolcev force-pushed the check_m2crypto_supported_ciphers branch from 5984f53 to a54e566 Compare June 7, 2020 23:56
@lgtm-com
Copy link

lgtm-com bot commented Jun 8, 2020

This pull request fixes 1 alert when merging a54e566 into 1bb89a9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@inikolcev
Copy link
Collaborator Author

@tomato42 I fixed the openssl errors but there were still issues after that so I removed M2CRYPTO_OLD from travis for now.

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.

shame, but so be it

Reviewed 1 of 1 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@tomato42 tomato42 merged commit 0913660 into tlsfuzzer:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AES-GCM ciphers fail with m2crypto 0.21.1
2 participants