Skip to content

Fix AES IV reuse - drop support for CTR and ECB - address CVE-2017-1000246 #519

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

Conversation

c00kiemon5ter
Copy link
Member

@c00kiemon5ter c00kiemon5ter commented Jul 13, 2018

Discussion on #417

Always generate a random IV for AES operations.

Quoting @obi1kenobi:

Initialization vector reuse like this is a security concern, since it leaks
information about the encrypted data to attackers, regardless of the
encryption mode used.
Instead of relying on a fixed, randomly-generated IV, it would be better to
randomly-generate a new IV for every encryption operation.

  • Drop AESCipher CTR and ECB support
  • Fix AESCipher IV reuse
  • Address CVE-2017-1000246

Fixes #417

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@codecov-io
Copy link

codecov-io commented Jul 13, 2018

Codecov Report

Merging #519 into master will increase coverage by 0.23%.
The diff coverage is 77.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   65.19%   65.42%   +0.23%     
==========================================
  Files         100      103       +3     
  Lines       25660    25681      +21     
==========================================
+ Hits        16729    16802      +73     
+ Misses       8931     8879      -52
Impacted Files Coverage Δ
src/saml2/authn.py 0% <0%> (ø) ⬆️
src/saml2/cryptography/pki.py 100% <100%> (ø)
src/saml2/sigver.py 70.57% <100%> (-0.24%) ⬇️
src/saml2/aes.py 100% <100%> (+100%) ⬆️
src/saml2/cert.py 91.08% <100%> (-3.44%) ⬇️
src/saml2/server.py 72.47% <75%> (-0.13%) ⬇️
src/saml2/cryptography/symmetric.py 78.46% <78.46%> (ø)
src/saml2/cryptography/asymmetric.py 90.47% <90.47%> (ø)
src/saml2/saml.py 92.08% <0%> (-0.21%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e4e1b...0e6aab4. Read the comment docs.

@c00kiemon5ter c00kiemon5ter force-pushed the fix-aes-ctr-ecb-CVE-2017-1000246 branch 5 times, most recently from e0661cc to 0917440 Compare July 17, 2018 13:50
@c00kiemon5ter c00kiemon5ter changed the title Fix AES IV reuse - drop support for CTR and ECB - address CVE-2017-1000246 Introduce cryptography module Aug 2, 2018
@c00kiemon5ter c00kiemon5ter force-pushed the fix-aes-ctr-ecb-CVE-2017-1000246 branch from 0917440 to 3cb617b Compare August 2, 2018 11:44
Quoting @obi1kenobi:
> Initialization vector reuse like this is a security concern, since it leaks
> information about the encrypted data to attackers, regardless of the
> encryption mode used.
> Instead of relying on a fixed, randomly-generated IV, it would be better to
> randomly-generate a new IV for every encryption operation.

Breaks AESCipher ECB support
Reported as CVE-2017-1000246
Fixes IdentityPython#417

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter c00kiemon5ter force-pushed the fix-aes-ctr-ecb-CVE-2017-1000246 branch from 3cb617b to 0e6aab4 Compare August 2, 2018 11:44
@c00kiemon5ter c00kiemon5ter changed the title Introduce cryptography module Fix AES IV reuse - drop support for CTR and ECB - address CVE-2017-1000246 Aug 2, 2018
@c00kiemon5ter c00kiemon5ter mentioned this pull request Aug 2, 2018
6 tasks
@c00kiemon5ter c00kiemon5ter merged commit 79d6798 into IdentityPython:master Aug 2, 2018
@fmarco fmarco mentioned this pull request Aug 3, 2018
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.

Reuse of AES initialization vector in AESCipher / UsernamePasswordMako / Server
3 participants