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

[nmiyake]

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]

@FiloSottile

[brandonweeks]

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

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

[shuxiao9058]

the same issue.

[ribeiroit]

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]

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]

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

[vieirin]

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

[sakjur]

@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]

Any Update on this?

[hmmftg]

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]

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]

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]

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

[arudzitis-stripe]

@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]

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

[andybons]

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

[arudzitis-stripe]

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

[rsc]

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]

CC @golang/security

[rsc]

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]

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

[rsc]

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

[rsc]

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

[arudzitis-stripe]

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