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

(Issue 5) MGF hash sha1/sha256 function? #129

Closed
fkooman opened this issue Jun 1, 2019 · 13 comments
Assignees

Comments

@fkooman
Copy link

@fkooman fkooman commented Jun 1, 2019

I'm trying to understand the RSA-OAEP encryption requirements for IdPs / SPs.

The following default digest algorithm MUST be used in conjunction with the above key transport algorithms (the default mask generation function, MGF1 with SHA1, MUST be used):

http://www.w3.org/2001/04/xmlenc#sha256 [XMLEnc]

It seems most IdPs use SHA1 for both the MFG1 and digest? So, this profile requires you to use SHA1 for the MFG1 and SHA256 for the digest. Any reason why it is not SHA256 for both?

Also, why not require MGF1 with SHA256: http://www.w3.org/2009/xmlenc11#mgf1sha256 as algorithm identifier? Now it is not clear that SHA256 was used for the digest?

Probably I am missing something here...

@judielaine

This comment has been minimized.

Copy link
Collaborator

@judielaine judielaine commented Jun 5, 2019

I read the parenthesized reference to the default mask generation function to be a reiteration of a requirement stated elsewhere, particularly XMLEnc's §5.4.2 statement that "As described in the EME-OAEP-ENCODE function RFC 2437 [PKCS1, section 9.1.1.1], .... using the mask generator function MGF1 (with SHA1) specified in RFC 2437."

If i am correct, i wonder if rewording as follows would be more clear

Key Transport (the default mask generation function, MGF1 with SHA1, MUST be used)

      http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p [XMLEnc]
       http://www.w3.org/2009/xmlenc11#rsa-oaep [XMLEnc]

The following default digest algorithm MUST be used in conjunction with the above key transport algorithms :

http://www.w3.org/2001/04/xmlenc#sha256 [XMLEnc]

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jun 5, 2019

There is definitely clarification needed, it reads very badly now...but most IdPs have long since stopped using SHA-1 for general usage, the MGF1 case is an exception and was left as is for interoperability. It's not that unusual for libraries to lack support for any MGF pluggability. If there are security implications for use of SHA-1 there, I'm not aware of them.

@judielaine judielaine changed the title MGF hash sha1/sha256 function? (Issue 5) MGF hash sha1/sha256 function? Jun 5, 2019
@judielaine

This comment has been minimized.

Copy link
Collaborator

@judielaine judielaine commented Jun 5, 2019

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jun 5, 2019

I misinterpreted in part...I think it is true that the norm for OAEP digests is SHA-1, and I just verified Shibboleth has continued that also. So I think we should probably just open up that requirement altogether and would agree with the original suggestion.

@vedrnes

This comment has been minimized.

Copy link

@vedrnes vedrnes commented Jun 26, 2019

The writing as is right now is a problem for those using cryptographic hardware modules when decrypting content with RSA-OAEP. Namely, most vendors out there restrict the RSA-OAEP implementation to using the same digest function as both the MGF digest and message digest. So a combination of MGF1withSHA1 + SHA256 is problematic while MGF1withSHA1 + SHA1 (aka the default settings for mgf1p), MGF1withSHA256 + SHA256, MGF1withSHA384 + SHA384 and MGF1withSHA512 + SHA256 work well. Given concerns about SHA1 it may also be wise to recommend the MGF1withSHA256 + SHA256 as the default choice. I would propose this be added to the profile - matching the digests is a small price to pay for greatly enhancing the ability to use native RSA-OAEP implementations on hardware modules.

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jun 26, 2019

I was considering either enumerating the SHA1+SHA1 and SHA2+SHA2 combinations or leaving it at SHA1+SHA1 to address the problem.

I don't have a strong opinion either way since I don't know the security implications of SHA-1 in this context, it's always been more about hedging against regulatory moves to bar SHA-1 from implementations used in particular contexts, and I don't think that's happened to this point.

@vedrnes

This comment has been minimized.

Copy link

@vedrnes vedrnes commented Jun 27, 2019

I would propose edits along the following lines:

  1. For encrypting, MGF1 digest and message digest algorithms MUST match (this provides broad support on HSM based implementations)
  2. For encrypting, implementations SHOULD use SHA-256, SHA-384 and SHA-512 (this gives a nudge in the direction of avoiding use of SHA-1)
  3. For decrypting, implementations MUST support SHA-1, SHA-256, SHA-384 and SHA-512 (this gives interoperability in the terms of accepting SHA-1 on the decryption side)
@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jun 27, 2019

This is about deployment only, not what implementations need to do. It doesn't have to cover cases that extend to interoperability outside the profile's own boundaries like a conformance document for a standard would.

I checked the implementation profile precursor to this work and it was written as MUST support SHA-256 and SHOULD support SHA-1. That would suggest we probably need to go with SHA-2/SHA-2 as the language here. I suspect given the GCM issue that the SHA-2 usage for OAEP will be the least of the problems with finding implementations that will work.

@vedrnes

This comment has been minimized.

Copy link

@vedrnes vedrnes commented Jun 27, 2019

I guess a MUST for SHA-2 and SHOULD for SHA-1 is OK.
However, including the digest match statement is important. For example, even though both SHA-256 and SHA-384 can be denoted as "SHA-2", MGF1/message digest combination SHA-256 + SHA-256 is OK, but SHA-256 + SHA-384 is not .

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jun 27, 2019

SHA-256 is what was stipulated, I'm just using SHA-2 as shorthand here. But your issue here is with the implementation profile, which is done, not this deployment profile. We can simply mandate SHA-256 for both and clean up the text here.

@vedrnes

This comment has been minimized.

Copy link

@vedrnes vedrnes commented Jun 27, 2019

I agree, mandating SHA-256/SHA-256 will work.

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jul 16, 2019

Did some testing and was able to flip Shibboleth over to SHA2 and MGF1-SHA2 and successfully tested Shibboleth SPs and one or two others. As expected most other impls fail on it. Will discuss with group tomorrow but given GCM already being in this profile, I doubt this should give anybody pause.

@scantor

This comment has been minimized.

Copy link
Collaborator

@scantor scantor commented Jul 23, 2019

After discusssion by group, it was felt that absent a compelling security issue with using SHA-1 in this area we should leave it be and match up with the standard usage in this area.
I'll include a note about possibly revisiting if there's a reason to.

scantor added a commit that referenced this issue Jul 23, 2019
@scantor scantor closed this Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.