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

crypto/rsa: allow hash.Hash for OAEP and MGF1 to be specified independently #19974

Closed
nmiyake opened this issue Apr 14, 2017 · 24 comments
Closed
Labels
Milestone

Comments

@nmiyake
Copy link
Contributor

nmiyake commented Apr 14, 2017

rsa.EncryptOAEP and rsa.DecryptOAEP both take a hash.Hash as input, and this hash function is used as the hash function for both OAEP and the MGF1 XOR. However, an option should be provided to specify the hash function for OAEP and MGF1 separately, as it is permissible for the hash functions for these two operations to be different.

This is pertinent for compatibility with other languages and RSA implementations, as the Sun JDK's implementation of the RSA/ECB/OAEPWITHSHA-256ANDMGF1PADDING provider uses SHA-256 for OAEP but SHA-1 for MGF1. As it currently stands, the rsa package in Go is not compatible with this encryption mode in Java.

For reference, the OpenSSL API also allows for the hash functions for OAEP and MGF1 to be specified separately: https://github.com/openssl/openssl/blob/master/crypto/rsa/rsa_oaep.c#L44, const EVP_MD *md, const EVP_MD *mgf1md.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2018
@andybons andybons added this to the Unplanned milestone Apr 11, 2018
@andybons
Copy link
Member

@FiloSottile

@FiloSottile FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Crypto Proposal related to crypto packages or other security issues and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 17, 2018
@brandonweeks
Copy link

As another data point, Android P has introduced usage of RSA/ECB/OAEPWITHSHA-256ANDMGF1PADDING.

https://developer.android.com/reference/android/security/keystore/WrappedKeyEntry

@shuxiao9058
Copy link

the same issue.

@ribeiroit
Copy link

Hi guys, I could help to implement the resolution for this issue, but I need your help to decide what's the best scenario for this.

Do you think the OEAPOptions is the best way?

Thank you

@FiloSottile
Copy link
Contributor

FiloSottile commented Sep 12, 2019

OAEP is one of those unfortunate standards with a million parameters, which the Go standard library intentionally culls to a reasonable subset. For example, there is no good reason I know of to use different hashes at different stages of the algorithm.

On balance, I'm mostly ok with adding a field to OEAPOptions for decryption, as it won't disturb common users. However, adding a whole new function would be more invasive, and I don't think it would be worth it.

Would having support for independent hashes only for decryption make sense for your use cases, or would it be useless without encryption? (It makes sense to me also because the library is already asymmetrical due to crypto.Decrypter not having an encryption counterpart, and decryption is more likely to be about compatibility with a third party.)

@ribeiroit
Copy link

I got your point, but thinking in interoperability maybe is the deal here. I think that it could reduce the fricction along another implementations. As the use case that I exposed related to android.

I think that makes sense use for both encrypt/decrypt.

Well, this is my understood at reading RFC 3447, starting chapter "7. Encryption schemes" and keeping up beyond this section. But, looking to the implementation that we have at the golang, I think that is a gonna be just a decision to update of the usage context and allow the encryption besides the decryption.

Thanks

sakjur added a commit to sakjur/go that referenced this issue Nov 11, 2019
Android and Java sometimes uses separate hash functions for the OAEP digest and
MGF1 mask generating function. By allowing the OAEPOption to support overriding
the mask generating function's hash algorithm it is possible to support
decryption of RSA-OAEP payloads generated with such a configuration.

Fixes golang#19974
sakjur added a commit to sakjur/go that referenced this issue Nov 11, 2019
Android and Java sometimes uses separate hash functions for the OAEP digest
and MGF1 mask generating function. By allowing the OAEPOption to support
overriding the mask generating function's hash algorithm it is possible to
support decryption of RSA-OAEP payloads generated with such a configuration.

Fixes golang#19974
@vieirin
Copy link

vieirin commented Nov 10, 2020

Update on this? Saw @sakjur commits but I couldn't find any PRs related

@sakjur
Copy link

sakjur commented Nov 10, 2020

@vieiramanoel That was mostly an experiment that didn't lead anywhere and I later ended up not needing to complete 🙂 I'm not sure if whatever I have in my branch works at all, and it was mostly an accident that my WIP commits ended up being linked from here.

@varunkumark1997
Copy link

Any Update on this?

@hmmftg
Copy link

hmmftg commented Sep 25, 2021

Still waiting for this feature,
I personally asked this on SO https://stackoverflow.com/questions/69286881/java-rsa-ecb-oaepwithsha-256andmgf1padding-migrate-to-go and pick a solution from there and it works https://play.golang.org/p/9Le_3GiQVUE
It's not so complicated change but it seems unattended.

@GalvinGao
Copy link

Any updates on this? Me personally is not quite sure about the status of this issue. I'm more than open to implement it and file a PR, but I'm unsure whether such work is appropriate for the go standard library :D

@arudzitis-stripe
Copy link
Contributor

Hello everyone! Long time user, first time contributor :-)

This is something we recently ran into while attempting to decrypt ciphertexts sent to us by a third-party. Unfortunately, we do not have the ability to change the parameters they use in this protocol. I am very interested (and motivated!) to work on upstreaming a change to resolve this.

It has been a while, but it sounds like the right direction was to add something to OEAPOptions. However, the challenge seems to be that everything flows the exported function:

DecryptOAEP(hash hash.Hash, random io.Reader, priv *PrivateKey, ciphertext []byte, label []byte) 

And it is desirable to not change the signature of this function. As a general approach, how would you feel about creating a new non-exported function (decryptOAEP), which is referenced by both the exported DecryptOAEP and the func (priv *PrivateKey) Decrypt(...) functions?

cc @FiloSottile

@gopherbot
Copy link

Change https://go.dev/cl/418874 mentions this issue: crypto: allow hash.Hash for OAEP and MGF1 to be specified independently

@arudzitis-stripe
Copy link
Contributor

@FiloSottile I see you might be looking at my pull request. I expect we will want to add test coverage for any change. However, it looks like there is an existing set of hard-coded test vectors that are used for OAEP, and it wasn't clear to me how those are being generated.

@arudzitis-stripe
Copy link
Contributor

I've rebased the PR on the latest code. It's been a while, but I'm still interested in discussing this change :-)

@andybons andybons changed the title crypto/rsa: allow hash.Hash for OAEP and MGF1 to be specified independently proposal: crypto/rsa: allow hash.Hash for OAEP and MGF1 to be specified independently Oct 4, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 4, 2022
@andybons
Copy link
Member

andybons commented Oct 4, 2022

Throwing in proposal review bucket since this is an API change.

@arudzitis-stripe
Copy link
Contributor

Thank you so much @ianlancetaylor and @andybons. Please let me know if there are any updates needed to the API change.

@rsc
Copy link
Contributor

rsc commented Oct 5, 2022

The proposed new API is to add MGFHash to rsa.OAEPOptions:

type OAEPOptions struct {
	// Hash is the hash function that will be used when generating the mask.
	Hash crypto.Hash

	// NEW: MGFHash is the hash function used for MGF1.
	// If zero, Hash is used instead.
	MGFHash crypto.Hash

	// Label is an arbitrary byte string that must be equal to the value
	// used when encrypting.
	Label []byte
}

(The CL uses *crypto.Hash but we can use the plain zero value for a presence check instead.)

Does anyone object to this API?

@ianlancetaylor
Copy link
Contributor

CC @golang/security

@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

Does anyone object to adding OAEPOptions.MGFHash as in #19974 (comment)?

@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/rsa: allow hash.Hash for OAEP and MGF1 to be specified independently crypto/rsa: allow hash.Hash for OAEP and MGF1 to be specified independently Oct 26, 2022
@rsc rsc modified the milestones: Proposal, Backlog Oct 26, 2022
@arudzitis-stripe
Copy link
Contributor

I've updated the change to reflect the proposed API: https://go-review.googlesource.com/c/go/+/418874

@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Nov 14, 2022
@golang golang locked and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests