Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change RSA padding default to OAEP #546

Closed
wants to merge 1 commit into from

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Oct 5, 2022

PKCS#1 v1.5 has been unsafe since 1998's Bleichenbacher attack. Even if these PKey::RSA methods are deprecated in favor of the new PKey::PKey#{encrypt,decrypt} interface, they can and are still used as https://shopify.engineering/building-flex-comp shows - in which case this library is recommending unsafe defaults to people who are not intimitely familiar with cryptography.

PKCS#1 v1.5 has been unsafe since 1998's Bleichenbacher attack. Even if these PKey::RSA methods are deprecated in favor of the new PKey::PKey#{encrypt,decrypt} interface, they can and are still used as https://shopify.engineering/building-flex-comp shows - in which case this library is recommending unsafe defaults to people who are not intimitely familiar with cryptography.
@rhenium
Copy link
Member

rhenium commented Oct 6, 2022

I disagree with this change in RSA#public_encrypt and #private_decrypt.

This will break existing applications using the default. It would be very annoying to track down such an issue since encrypted data doesn't contain information about the padding scheme.

Also, these legacy methods lack a way to specify OAEP parameters, which is necessary for interoperability with other applications. They use hard-coded parameters, SHA-1 in MGF1 and an empty label.

I think it's best to leave the behavior as it is now. They are meant for legacy applications only anyway.

@rhenium rhenium closed this Oct 6, 2022
@bdewater
Copy link
Contributor Author

bdewater commented Oct 6, 2022

Would you consider a patch instead calling out the insecure default in the documentation?

@rhenium
Copy link
Member

rhenium commented Oct 7, 2022

Yes, that would be useful!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants