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

Disallow verifying signatures with private RSA keys. #142

Closed
wants to merge 1 commit into from

Conversation

zejn
Copy link
Collaborator

@zejn zejn commented Apr 28, 2019

Some backends are smart and know how to verify with private keys too.
Disallow that on those backends.

Backwards incompatible change.

Reported in #53.

Some backends are smart and know how to verify with private keys too.
Disallow that on those backends.
@zejn zejn added this to the 4.0 milestone Apr 28, 2019
@codecov
Copy link

codecov bot commented Apr 28, 2019

Codecov Report

Merging #142 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   96.55%   96.57%   +0.01%     
==========================================
  Files          14       14              
  Lines        1075     1079       +4     
==========================================
+ Hits         1038     1042       +4     
  Misses         37       37
Impacted Files Coverage Δ
jose/backends/pycrypto_backend.py 95.83% <100%> (+0.07%) ⬆️
jose/backends/rsa_backend.py 96.55% <100%> (+0.04%) ⬆️

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 deea760...40920a5. Read the comment docs.

@blag
Copy link
Contributor

blag commented Dec 16, 2019

If multiple backends allow people to verify with private keys, doesn't seem to imply that somebody somewhere has a valid usecase for doing so?

As mpdavis said:

This happens because the private key contains both the public and private key info. The verify method extracts the public key info and happily uses it.

I think it might be appropriate to simply throw a suppressible warning but continue the verification as normal.

@zejn
Copy link
Collaborator Author

zejn commented Dec 17, 2019

I agree that there may be a use case for this: It can certainly be a bit easier to just put in the private key and do verification with it. But I think there may be cases that is not correct, eg. somebody just using (same) private key on both ends of communication channel and thinking they're secure.

That's why I presonally would rather see this chosen explicitly.

I didn't want to just merge it since I think it merits a discussion.

@blag
Copy link
Contributor

blag commented Dec 17, 2019

But I think there may be cases that is not correct, eg. somebody just using (same) private key on both ends of communication channel and thinking they're secure.

Well, the recipient is (AIUI) just calculating the public key from the private key, so while it isn't the way RSA intended RSA to be used, it's also not guaranteed to be insecure (aside from securely distributing the private key itself, but this library doesn't deal with that). It's definitely super weird, IMO, but it's not necessarily "wrong", and I don't think it's something we should concern ourselves with preventing outright.

I would be more comfortable with just emitting a warning. @zejn @dumptyd Is that acceptable to you for now? If somebody can demonstrate that this is guaranteed to be insecure, we can always implement this PR at a later date.

@zejn
Copy link
Collaborator Author

zejn commented Dec 18, 2019

Warning seems sensible and OK.

@dumptyd
Copy link

dumptyd commented Dec 18, 2019

If somebody can demonstrate that this is guaranteed to be insecure, we can always implement this PR at a later date.

I couldn't think of such a scenario.

Is that acceptable to you for now?

Yup, I have no issues with that. Thanks!

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.

3 participants