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

Fix for CVE-2016-6298 #66

Closed
wants to merge 2 commits into from
Closed

Fix for CVE-2016-6298 #66

wants to merge 2 commits into from

Conversation

simo5
Copy link
Member

@simo5 simo5 commented Aug 31, 2016

This fixes the security issue reported in #65

RFC 3218 describes an oracle attack called Million Messages Attack
against RSA with PKCS1 v1.5 padding.

Depending on how JWEs are used a server may become an Oracle, and the
mitigation presecribed in RFC 3218 2.3.2 need to be implemented.

Many thanks to Dennis Detering for his responsible disclosure and help
verifying the mitigation approach.

Signed-off-by: Simo Sorce <simo@redhat.com>
import time

for _ in range(self.iterations):
start = time.clock()
Copy link
Member

Choose a reason for hiding this comment

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

time.clock() is the worst clock on Unix. You want:

  • time.perf_counter() on Python 3.3+
  • time.time() on POSIX
  • time.clock() on Windows

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a tool to get a rough idea of the timing, the variation is still high enough to not really matter I think.

@tiran
Copy link
Member

tiran commented Aug 31, 2016

I'm not fully convinced that the mitigation removes all timing variations. It's almost impossible to write constant timing code in Python. Heck, it's even extremely hard in C.

How about we mark PKCS1v15, NULL and other bad algos as insecure and not offer them by default? I have an idea to make the registry more flexible.

@simo5
Copy link
Member Author

simo5 commented Aug 31, 2016

We can't remove all timing variations but that is not the point, we just need to avoid big enough variations to be measurable over the network. Different messages already cause different timings on their own.
It makes no sense to remove RSA1_5 because we have a mitigation and the vulnerability is usage specific, it is only present if an attacker can capture messages and use a server as an oracle. There are perfectly valid uses of this algorithm and it is mandated by the standard,

This test is not very reliable and takes a long time so it is provided but
diasabled by default.
It is only useful to verify if any regression regarding MMA occurs, so it can
be just run occasionally.

Signed-off-by: Simo Sorce <simo@redhat.com>
@Merenon
Copy link

Merenon commented Aug 31, 2016

This patch looks pretty good to me. It's a great idea to always throw an exception: My test cases showed great results with this fix and I believe the difference in timing is negligible over the network.

I agree, that RSA 1.5 is not the best algorithm to support, but as Simo already mentioned, it's necessary to support it in order to be compliant with the specification.
I guess, there are some discussions about deprecating this algorithm though...

Thanks for your fast reaction in fixing this, Simo! It was a pleasure!

@simo5 simo5 closed this in eb5be5b Aug 31, 2016
@rokclimb15
Copy link

@simo5 I'm working on backporting the fix to Ubuntu 16.04 (0.2.1). Could you possibly share your tests for the issue so I can write a unit test and confirm the backported fix works?

https://people.canonical.com/~ubuntu-security/cve/2016/CVE-2016-6298.html

@simo5
Copy link
Member Author

simo5 commented Sep 14, 2017

@rokclimb15 all the tests I have are in the TestMMA function in the second commit of this PR.
HTH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants