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

8284553: Deprecate the DEFAULT static field of OAEPParameterSpec #8191

Closed
wants to merge 4 commits into from

Conversation

valeriepeng
Copy link

@valeriepeng valeriepeng commented Apr 12, 2022

This trivial change is to deprecate the DEFAULT static field of OAEPParameterSpec class. Wordings are mostly the same as the previous PSSParameterSpec deprecation change. Rest are just minor code re-factoring.

The CSR will be filed once review is somewhat finished.

Thanks,
Valerie


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
  • JDK-8284919: Deprecate the DEFAULT static field of OAEPParameterSpec (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191
$ git checkout pull/8191

Update a local copy of the PR:
$ git checkout pull/8191
$ git pull https://git.openjdk.java.net/jdk pull/8191/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8191

View PR using the GUI difftool:
$ git pr show -t 8191

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8191.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2022

👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 12, 2022
@openjdk
Copy link

openjdk bot commented Apr 12, 2022

@valeriepeng The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label Apr 12, 2022
@valeriepeng
Copy link
Author

/csr

@mlbridge
Copy link

mlbridge bot commented Apr 12, 2022

Webrevs

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Apr 12, 2022
@openjdk
Copy link

openjdk bot commented Apr 12, 2022

@valeriepeng has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@valeriepeng please create a CSR request for issue JDK-8284553. This pull request cannot be integrated until the CSR request is approved.

@mlbridge
Copy link

mlbridge bot commented Apr 12, 2022

Mailing list message from Michael StJohns on security-dev:

On 4/11/2022 9:34 PM, Valerie Peng wrote:

Hi Valerie -

I think deprecating DEFAULT? is wrong.? RFC8017 still has this definition:

RSAES-OAEP-params ::= SEQUENCE {
hashAlgorithm [0] HashAlgorithm DEFAULT sha1,
maskGenAlgorithm [1] MaskGenAlgorithm DEFAULT mgf1SHA1,
pSourceAlgorithm [2] PSourceAlgorithm DEFAULT pSpecifiedEmpty
}

and DEFAULT is what you should be getting if you see an empty sequence
in the param field.? It's DEFAULT in ASN1 terms, not DEFAULT in terms of
what you should use going forward? to create signatures, and the ASN1
DEFAULT won't change.

In any event, I can't find where RFC8017 says anything about deprecating
the defaults.? AFAICT, although there's general guidance to go away from
SHA1,? the math suggests that SHA1 is still sufficient for OAEP,? and
there's no guidance specific to OAEP's use of SHA1 that I can find other
than the requirement in NIST SP800-56B rev 2 to use "Approved Hash
Functions" for OAEP. If there's a section in 8017 that you're looking at
for this guidance that I missed, you may want to update your comment to
point to it.

Take care - Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20220411/aaa5d423/attachment.htm>

@seanjmullan
Copy link
Member

I think deprecating DEFAULT? is wrong.? RFC8017 still has this definition:

RSAES-OAEP-params ::= SEQUENCE {
hashAlgorithm [0] HashAlgorithm DEFAULT sha1,
maskGenAlgorithm [1] MaskGenAlgorithm DEFAULT mgf1SHA1,
pSourceAlgorithm [2] PSourceAlgorithm DEFAULT pSpecifiedEmpty
}

and DEFAULT is what you should be getting if you see an empty sequence in the param field.? It's DEFAULT in ASN1 terms, not DEFAULT in terms of what you should use going forward? to create signatures, and the ASN1 DEFAULT won't change.

In any event, I can't find where RFC8017 says anything about deprecating the defaults.? AFAICT, although there's general guidance to go away from SHA1,? the math suggests that SHA1 is still sufficient for OAEP,? and there's no guidance specific to OAEP's use of SHA1 that I can find other than the requirement in NIST SP800-56B rev 2 to use "Approved Hash Functions" for OAEP. If there's a section in 8017 that you're looking at for this guidance that I missed, you may want to update your comment to point to it.

https://www.rfc-editor.org/rfc/rfc8017#appendix-A.2.1 (which defines the OAEP-params structure) says:

o hashAlgorithm identifies the hash function. It SHALL be an
algorithm ID with an OID in the set OAEP-PSSDigestAlgorithms. For
a discussion of supported hash functions, see Appendix B.1.

https://www.rfc-editor.org/rfc/rfc8017#appendix-B.1 is a long section discussing hash functions, but I think this text (last two paragraphs) is most relevant:

There have also been advances in the cryptanalysis of SHA-1.
Particularly, the results of Wang et al. [SHA1CRYPT] (which have
been independently verified by M. Cochran in his analysis [COCHRAN])
on using a differential path to find collisions in SHA-1, which
conclude that the security strength of the SHA-1 hashing algorithm is
significantly reduced. However, this reduction is not significant
enough to warrant the removal of SHA-1 from existing applications,
but its usage is only recommended for backwards-compatibility
reasons.

To address these concerns, only SHA-224, SHA-256, SHA-384, SHA-512,
SHA-512/224, and SHA-512/256 are RECOMMENDED for new applications.
As of today, the best (known) collision attacks against these hash
functions are generic attacks with complexity 2L/2, where L is the
bit length of the hash output. For the signature schemes in this
document, a collision attack is easily translated into a signature
forgery. Therefore, the value L / 2 should be at least equal to the
desired security level in bits of the signature scheme (a security
level of B bits means that the best attack has complexity 2B). The
same rule of thumb can be applied to RSAES-OAEP; it is RECOMMENDED
that the bit length of the seed (which is equal to the bit length of
the hash output) be twice the desired security level in bits.

Also, this is a proposed deprecation ... there is no plan to remove this field. The main intention of this deprecation is to alert users that SHA-1 is being used as it may not be apparent from the "DEFAULT" name, and to make sure that is ok or to think about using something stronger.

@valeriepeng I recommend making the link more specific in the deprecation text to point to https://www.rfc-editor.org/rfc/rfc8017#appendix-B.1

Comment on lines -75 to -81
* <p>Note: the OAEPParameterSpec.DEFAULT uses the following:
* <pre>
* message digest -- "SHA-1"
* mask generation function (mgf) -- "MGF1"
* parameters for mgf -- MGF1ParameterSpec.SHA1
* source of encoding input -- PSource.PSpecified.DEFAULT
* </pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should leave these lines in, so users can still see what the default parameters are. Nit, change:
"the OAEPParameterSpec.DEFAULT uses the following"
to:
"{@code OAEPParameterSpec.DEFAULT} uses the following default values:"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this in meaning we may have to duplicate the new wordings for the field here. The default values are covered in the paragraph above where the ASN.1 structure for RSAES-OAEP-params is defined. I can move this down to the javadoc field description for the DEFAULT field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@valeriepeng
Copy link
Author

Mailing list message from Michael StJohns on security-dev:

On 4/11/2022 9:34 PM, Valerie Peng wrote:

Hi Valerie -

I think deprecating DEFAULT? is wrong.? RFC8017 still has this definition:

RSAES-OAEP-params ::= SEQUENCE {
hashAlgorithm [0] HashAlgorithm DEFAULT sha1,
maskGenAlgorithm [1] MaskGenAlgorithm DEFAULT mgf1SHA1,
pSourceAlgorithm [2] PSourceAlgorithm DEFAULT pSpecifiedEmpty
}

and DEFAULT is what you should be getting if you see an empty sequence in the param field.? It's DEFAULT in ASN1 terms, not DEFAULT in terms of what you should use going forward? to create signatures, and the ASN1 DEFAULT won't change.

In any event, I can't find where RFC8017 says anything about deprecating the defaults.? AFAICT, although there's general guidance to go away from SHA1,? the math suggests that SHA1 is still sufficient for OAEP,? and there's no guidance specific to OAEP's use of SHA1 that I can find other than the requirement in NIST SP800-56B rev 2 to use "Approved Hash Functions" for OAEP. If there's a section in 8017 that you're looking at for this guidance that I missed, you may want to update your comment to point to it.

Take care - Mike

-------------- next part -------------- An HTML attachment was scrubbed... URL: https://mail.openjdk.java.net/pipermail/security-dev/attachments/20220411/aaa5d423/attachment.htm

Hi Mike,
Thanks for the comment/feedback. Deprecating the static field in javadoc does not mean we are changing its parameters with something other than those in RFC 8017. Providers can and should still follow the default ASN.1 values when the DER encoding omit the particular field(s). Besides what Sean mentioned already, this deprecation is an effort to moving toward the "explicitly specify what you want to use" direction. Hope this clarifies thing up?
I like the suggestion for using a more specific link and will change it to point to the particular section.

Regards,
Valerie

* Constructs a parameter set for OAEP padding as defined in
* the PKCS #1 standard using the default values.
*/
// disallowed
private OAEPParameterSpec() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just remove this ctor now that it is not used by DEFAULT.

@valeriepeng
Copy link
Author

Comment on lines 99 to 100
* to advances in cryptanalysis -- see the
* <a href="https://www.rfc-editor.org/rfc/rfc8017#appendix-B.1">Appendix B.1</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the URL text, you should still say this is from RFC 8017. Suggest changing these two lines to:

     *         to advances in cryptanalysis -- see
     *         <a href="https://www.rfc-editor.org/rfc/rfc8017#appendix-B.1">Appendix B.1 of RFC 8017</a>

*
* @deprecated This field uses the default values defined in the PKCS #1
* standard. Some of these defaults are no longer recommended due
* to advances in cryptanalysis -- see the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "the".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok~

* if {@code null} is specified, {@code null} will be returned by
* {@link #getMGFParameters()}
* @param pSrc the source of the encoding input P
* @exception NullPointerException if {@code mdName},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change @exception to the more accepted @throws tag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will change.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 20, 2022
@openjdk
Copy link

openjdk bot commented Apr 20, 2022

@valeriepeng This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8284553: Deprecate the DEFAULT static field of OAEPParameterSpec

Reviewed-by: mullan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 105 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 20, 2022
@valeriepeng
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Apr 20, 2022

Going to push as commit 15ce8c6.
Since your change was applied there have been 114 commits pushed to the master branch:

  • e8016f7: 8281006: Module::getResourceAsStream should check if the resource is open unconditionally when caller is null
  • 018017a: 8266247: Swing test bug7154030.java sometimes fails on macOS 11 ARM
  • e6c5f28: 8280594: Refactor annotation invocation handler handling to use Objects.toIdentityString
  • b4a85cd: 8284742: x86: Handle integral division overflow during parsing
  • 5291ec8: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task
  • 6c6d522: 8284758: [linux] improve print_container_info
  • 46b2e54: 8075816: Deprecate AliasLevel flag since it is broken
  • 1b71621: 8042381: Test javax/swing/JRootPane/4670486/bug4670486.java fails with Action has not been received
  • 0f81d8f: 8284933: Improve debug in jdk.crypto.cryptoki
  • 72726c4: 8284563: AArch64: bitperm feature detection for SVE2 on Linux
  • ... and 104 more: https://git.openjdk.java.net/jdk/compare/4d45c3ebc493bb2c85dab84b97840c8ba093ab1f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 20, 2022
@openjdk openjdk bot closed this Apr 20, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 20, 2022
@openjdk
Copy link

openjdk bot commented Apr 20, 2022

@valeriepeng Pushed as commit 15ce8c6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@valeriepeng valeriepeng deleted the JDK-8284553 branch April 20, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
2 participants