8286908: ECDSA signature should not return parameters #8758

wangweij · 2022-05-17T19:56:22Z

Let ECDSA's engineGetParameters() always return null. At the same time, remove the remembered sigParams field. One behavior change is that after calling setParameter(), one can call init() again with a key using different parameters. I think this should be allowed since we are reusing the signature object with a brand new key.

setParameter is kept unchanged to be able to deal with certificates still having parameters after the signature algorithm object identifier. See https://bugs.openjdk.java.net/browse/JDK-8225745.

Also added SHA1withECDSA to the no-NULL list in KnownOIDs.

All security-related tests passed.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request to be approved
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issues

JDK-8286908: ECDSA signature should not return parameters
JDK-8286982: ECDSA signature should not return parameters (CSR)

Reviewers

Anthony Scarpino (@ascarpino - Reviewer) ⚠️ Review applies to b13de5a9
Hai-May Chao (@haimaychao - Committer) ⚠️ Review applies to b13de5a9
Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to aceeb4a7

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8758

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

Using diff file

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

bridgekeeper · 2022-05-17T19:56:56Z

👋 Welcome back weijun! 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 · 2022-05-17T20:00:54Z

@wangweij 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.

mlbridge · 2022-05-17T20:05:06Z

Webrevs

jnimeh · 2022-05-17T20:27:41Z

Do the behavioral changes you've cited in the PR description warrant a CSR, or do you feel this behavioral change is still consistent with the current Signature API documentation?

wangweij · 2022-05-17T20:37:15Z

Do the behavioral changes you've cited in the PR description warrant a CSR, or do you feel this behavioral change is still consistent with the current Signature API documentation?

I think so. In fact, after this change, there's simply no parameters for the signature, so calling setParameter should not change any internal state and thus has no effect on other calls.

While this is a behavior change, I don't think it has any negative impact to users.

ascarpino · 2022-05-17T21:22:18Z

Looks good to me.
The changes seem within the spec to me.

openjdk · 2022-05-17T21:23:36Z

@wangweij 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:

8286908: ECDSA signature should not return parameters

Reviewed-by: ascarpino, hchao, valeriep

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 28 new commits pushed to the master branch:

4587337: 8286858: Remove dead code in sun.reflect.misc.MethodUtil
6d56caf: 8285962: NimbusDefaults has a typo in a L&F property
d5d19f5: 8287013: StringConcatFactory: remove short and byte mixers/prependers
828dc89: 8286893: G1: Recent card set coarsening statistics wrong
6569666: 8286943: G1: With virtualized remembered sets, maximum number of cards configured is wrong
40e99a1: 8285308: Win: Japanese logical fonts are drawn with wrong size
890771e: 8285517: System.getenv() returns unexpected value if environment variable has non ASCII character
de74e0e: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable
9f562ef: 8286872: Refactor add/modify notification icon (TrayIcon)
b089229: 8271078: jdk/incubator/vector/Float128VectorTests.java failed a subtest
... and 18 more: https://git.openjdk.java.net/jdk/compare/2a2d54e8a6e24da49d57c0648a2af41c98b78ea4...master

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.

seanjmullan · 2022-05-18T15:29:07Z

src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSASignature.java

+        if (params == null) {
+            return;
+        }
+        if (!(params instanceof ECParameterSpec ecparams)) {
            throw new InvalidAlgorithmParameterException("No parameter accepted");


This is an odd error message since it does accept ECParameterSpec. Suggest changing the message to "Parameters must be of type ECParameterSpec".

valeriepeng

Looks fine.

wangweij · 2022-05-18T22:48:32Z

/csr

haimaychao · 2022-05-18T22:49:05Z

src/java.base/share/classes/sun/security/util/SignatureUtil.java

-                }
+                // Some certificates have params in an ECDSA algorithmID,
+                // they are useless and should be ignored.
+                return null;


Suggest adding the RFC reference for this change.

openjdk · 2022-05-18T22:49:29Z

@wangweij this pull request will not be integrated until the CSR request JDK-8286982 for issue JDK-8286908 has been approved.

haimaychao · 2022-05-19T19:20:43Z

CSR and updated webrev look good.

wangweij · 2022-05-23T13:32:46Z

/integrate

openjdk · 2022-05-23T13:34:39Z

Going to push as commit 8040aa0.
Since your change was applied there have been 46 commits pushed to the master branch:

689f80c: 8287153: Whitespace typos in HeapRegion class
c906591: 8286391: Address possibly lossy conversions in jdk.compiler
88018c4: 8287150: Remove HeapRegion::block_start_const declaration without definition
81f128b: 8287154: java/nio/channels/FileChannel/LargeMapTest.java does not compile
7becf13: 8286825: Linker naming cleanup
a0042de: 8276549: Improve documentation about ContainerPtr encoding
89a1d05: 8286715: Generalize MemorySegment::ofBuffer
cb08b4e: 8287024: G1: Improve the API boundary between HeapRegionRemSet and G1CardSet
01916e1: 8287053: Avoid redundant HashMap.containsKey calls in ZoneInfoFile.getZoneInfo0
414265b: 8189669: Deduplicate VerifyOption documentation
... and 36 more: https://git.openjdk.java.net/jdk/compare/2a2d54e8a6e24da49d57c0648a2af41c98b78ea4...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-23T13:34:56Z

@wangweij Pushed as commit 8040aa0.

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

the fix

b13de5a

openjdk bot added the rfr Pull request is ready for review label May 17, 2022

openjdk bot added the security security-dev@openjdk.org label May 17, 2022

ascarpino approved these changes May 17, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 17, 2022

haimaychao approved these changes May 17, 2022

View reviewed changes

seanjmullan reviewed May 18, 2022

View reviewed changes

will not read params for ECDSA

aceeb4a

valeriepeng approved these changes May 18, 2022

View reviewed changes

haimaychao reviewed May 18, 2022

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label May 18, 2022

openjdk bot removed the ready Pull request is ready to be integrated label May 18, 2022

wangweij added 2 commits May 18, 2022 21:56

Merge branch 'master' into 8286908

652bbc1

add reference to RFCs

e5043a9

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels May 20, 2022

openjdk bot added the integrated Pull request has been integrated label May 23, 2022

openjdk bot closed this May 23, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 23, 2022

wangweij deleted the 8286908 branch May 30, 2022 22:26

8286908: ECDSA signature should not return parameters #8758

8286908: ECDSA signature should not return parameters #8758

Uh oh!

Conversation

wangweij commented May 17, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 17, 2022

Uh oh!

openjdk bot commented May 17, 2022

Uh oh!

mlbridge bot commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jnimeh commented May 17, 2022

Uh oh!

wangweij commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascarpino commented May 17, 2022

Uh oh!

openjdk bot commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanjmullan May 18, 2022

Choose a reason for hiding this comment

Uh oh!

valeriepeng left a comment

Choose a reason for hiding this comment

Uh oh!

wangweij commented May 18, 2022

Uh oh!

haimaychao May 18, 2022

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 18, 2022

Uh oh!

haimaychao commented May 19, 2022

Uh oh!

wangweij commented May 23, 2022

Uh oh!

openjdk bot commented May 23, 2022

Uh oh!

openjdk bot commented May 23, 2022

Uh oh!

Uh oh!

wangweij commented May 17, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 17, 2022 •

edited

Loading

wangweij commented May 17, 2022 •

edited

Loading

openjdk bot commented May 17, 2022 •

edited

Loading