8026976: ECParameters, Point does not match field size #2958

kurashige23 · 2024-10-21T07:15:55Z

Hi all,

This is a backport of JDK-8026976: ECParameters, Point does not match field size

Original patch apply cleanly to 11u.

Testing: jdk/sun/security/pkcs11 tests on RHEL9, GHA testing

Thanks.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
JDK-8026976 needs maintainer approval

Issue

JDK-8026976: ECParameters, Point does not match field size (Bug - P4 - Approved)

Reviewers

Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2958/head:pull/2958
$ git checkout pull/2958

Update a local copy of the PR:
$ git checkout pull/2958
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2958/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2958

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2958.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-21T07:16:13Z

👋 Welcome back kurashige23! 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 · 2024-10-21T07:16:51Z

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

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

8026976: ECParameters, Point does not match field size

Reviewed-by: sgehwolf

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-10-21T07:17:10Z

This backport pull request has now been updated with issue from the original commit.

openjdk · 2024-10-21T07:17:52Z

⚠️ @kurashige23 This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

mlbridge · 2024-10-21T07:21:44Z

Webrevs

kurashige23 · 2024-10-21T07:24:44Z

/approval request Clean backport. The risk is low because the fix to the source is only for P11ECKeyFactory.java and it is just addition of switching by UseEcX963Encoding. pkcs11 tests on RHEL9 and GHA tests pass.

openjdk · 2024-10-21T07:25:04Z

@kurashige23
8026976: The approval request has been created successfully.

kurashige23 · 2024-11-05T00:37:02Z

Could anyone review this backport please?

kurashige23 · 2024-11-27T04:16:52Z

Could anyone review this backport please?

bridgekeeper · 2024-12-25T09:17:10Z

@kurashige23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

kurashige23 · 2025-01-06T10:54:53Z

Comment to avoid automatic close.

bridgekeeper · 2025-02-03T14:07:22Z

@kurashige23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

kurashige23 · 2025-02-05T23:41:37Z

@gnu-andrew

Sorry for the sudden mention, if possible, could you please review this PR?

jerboaa · 2025-02-20T16:39:36Z

@martinuy @franferrax Could you please take a look at this backport and see if that makes sense to bring to 11u at this stage of where 11u is currently? Thanks!

franferrax · 2025-02-24T14:59:07Z

@jerboaa: in my view this is a minor and clean backport. 11u already contains e6e820c, which is partial/incomplete without this change.

I agree that the risk is low considering this affects SunPKCS11, a security provider that is disabled by default.

The test removed from ProblemList.txt (sun/security/pkcs11/ec/TestKeyFactory.java) is now passing (I checked this in a local slowdebug build of this PR code). This test fails with the current version of NSS and without this PR change, meaning this bug may be being hit by users:

java.security.spec.InvalidKeySpecException: Could not parse key
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.implGetPublicKeySpec(P11ECKeyFactory.java:300)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11KeyFactory.engineGetKeySpec(P11KeyFactory.java:94)
	at java.base/java.security.KeyFactory.getKeySpec(KeyFactory.java:433)
	at TestKeyFactory.testPublic(TestKeyFactory.java:83)
	at TestKeyFactory.test(TestKeyFactory.java:117)
	at TestKeyFactory.main(TestKeyFactory.java:146)
	at PKCS11Test.premain(PKCS11Test.java:907)
	at PKCS11Test.testNSS(PKCS11Test.java:605)
	at PKCS11Test.main(PKCS11Test.java:254)
	at TestKeyFactory.main(TestKeyFactory.java:124)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.io.IOException: Point does not match field size
	at java.base/sun.security.util.ECUtil.decodePoint(ECUtil.java:48)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.decodePoint(P11ECKeyFactory.java:89)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.implGetPublicKeySpec(P11ECKeyFactory.java:297)
	... 15 more

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11ECKeyFactory.java

jerboaa · 2025-02-24T15:23:23Z

@jerboaa: in my view this is a minor and clean backport. 11u already contains e6e820c, which is partial/incomplete without this change.

I agree that the risk is low considering this affects SunPKCS11, a security provider that is disabled by default.

The test removed from ProblemList.txt (sun/security/pkcs11/ec/TestKeyFactory.java) is now passing (I checked this in a local slowdebug build of this PR code). This test fails with the current version of NSS and without this PR change, meaning this bug may be being hit by users:

java.security.spec.InvalidKeySpecException: Could not parse key
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.implGetPublicKeySpec(P11ECKeyFactory.java:300)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11KeyFactory.engineGetKeySpec(P11KeyFactory.java:94)
	at java.base/java.security.KeyFactory.getKeySpec(KeyFactory.java:433)
	at TestKeyFactory.testPublic(TestKeyFactory.java:83)
	at TestKeyFactory.test(TestKeyFactory.java:117)
	at TestKeyFactory.main(TestKeyFactory.java:146)
	at PKCS11Test.premain(PKCS11Test.java:907)
	at PKCS11Test.testNSS(PKCS11Test.java:605)
	at PKCS11Test.main(PKCS11Test.java:254)
	at TestKeyFactory.main(TestKeyFactory.java:124)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.io.IOException: Point does not match field size
	at java.base/sun.security.util.ECUtil.decodePoint(ECUtil.java:48)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.decodePoint(P11ECKeyFactory.java:89)
	at jdk.crypto.cryptoki/sun.security.pkcs11.P11ECKeyFactory.implGetPublicKeySpec(P11ECKeyFactory.java:297)
	... 15 more

OK, thanks. That's helpful.

kurashige23 · 2025-03-07T05:02:32Z

@jerboaa

I recognize that JDK-8026976 requires a maintainer approval.
I'm sorry to bother you, but could you approve?

Thanks.

kurashige23 · 2025-03-07T05:30:27Z

Oh, I noticed that JDK-8026976 is labeled "jdk11u-defer-next."
As explained in https://wiki.openjdk.org/display/JDKUpdates/JDK11u#JDK11u-Commonjdk11uJBSlabels, "jdk11u-defer-next" will be cleared when development of the next release is started, and then approved for JDK-8026976?

jerboaa

Looks good.

jerboaa · 2025-03-10T10:18:52Z

As explained in https://wiki.openjdk.org/display/JDKUpdates/JDK11u#JDK11u-Commonjdk11uJBSlabels, "jdk11u-defer-next" will be cleared when development of the next release is started, and then approved for JDK-8026976?

Yes, next development cycle started. I've cleared those. Please merge latest master which should get cleaner GHA runs. Thanks!

kurashige23 · 2025-03-11T06:45:27Z

I merged latest branch and runned GHA.
I think that failure in macos-x64 is not related to this PR's change.

kurashige23 · 2025-03-12T00:00:09Z

Thank you for your review.

/integrate

openjdk · 2025-03-12T00:01:09Z

@kurashige23
Your change (at version 04dd235) is now ready to be sponsored by a Committer.

jerboaa · 2025-03-12T09:18:36Z

/sponsor

openjdk · 2025-03-12T09:19:05Z

Going to push as commit 8785172.

openjdk · 2025-03-12T09:19:13Z

@jerboaa @kurashige23 Pushed as commit 8785172.

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

gnu-andrew · 2025-03-14T23:25:53Z

As explained in https://wiki.openjdk.org/display/JDKUpdates/JDK11u#JDK11u-Commonjdk11uJBSlabels, "jdk11u-defer-next" will be cleared when development of the next release is started, and then approved for JDK-8026976?

Yes, next development cycle started. I've cleared those. Please merge latest master which should get cleaner GHA runs. Thanks!

No it didn't. 11u-dev is currently closed for commits. This should not have been pushed until the freeze was lifted. Re-assigned manually to 11.0.28.

kurashige23 · 2025-03-18T01:39:42Z

Sorry for the inconvenience and thanks for fixing JDK-8351828.
Is there anything else I should do?

Backport 55f5542

bb5ed9c

openjdk bot changed the title ~~Backport 55f5542ca2104df91e14693534cc7b3c36e81953~~ 8026976: ECParameters, Point does not match field size Oct 21, 2024

openjdk bot added backport clean labels Oct 21, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 21, 2024

openjdk bot added the approval label Oct 21, 2024

franferrax reviewed Feb 24, 2025

View reviewed changes

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11ECKeyFactory.java Outdated Show resolved Hide resolved

kurashige23 and others added 2 commits February 26, 2025 08:26

Merge branch 'openjdk:master' into JDK-8026976

131868b

Fix copyright year

f019f2d

jerboaa approved these changes Mar 10, 2025

View reviewed changes

Merge branch 'openjdk:master' into JDK-8026976

04dd235

openjdk bot added ready Pull request is ready to be integrated and removed approval labels Mar 11, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 12, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 12, 2025

openjdk bot closed this Mar 12, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 12, 2025

kurashige23 mentioned this pull request May 7, 2025

8026976: ECParameters, Point does not match field size openjdk/jdk8u-dev#643

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8026976: ECParameters, Point does not match field size #2958

8026976: ECParameters, Point does not match field size #2958

kurashige23 commented Oct 21, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024 •

edited

Loading

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

mlbridge bot commented Oct 21, 2024 •

edited

Loading

kurashige23 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

kurashige23 commented Nov 5, 2024

kurashige23 commented Nov 27, 2024

bridgekeeper bot commented Dec 25, 2024

kurashige23 commented Jan 6, 2025

bridgekeeper bot commented Feb 3, 2025

kurashige23 commented Feb 5, 2025

jerboaa commented Feb 20, 2025

franferrax commented Feb 24, 2025

jerboaa commented Feb 24, 2025

kurashige23 commented Mar 7, 2025

kurashige23 commented Mar 7, 2025 •

edited

Loading

jerboaa left a comment

jerboaa commented Mar 10, 2025

kurashige23 commented Mar 11, 2025

kurashige23 commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

jerboaa commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

gnu-andrew commented Mar 14, 2025

kurashige23 commented Mar 18, 2025

8026976: ECParameters, Point does not match field size #2958

8026976: ECParameters, Point does not match field size #2958

Conversation

kurashige23 commented Oct 21, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024 • edited Loading

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

mlbridge bot commented Oct 21, 2024 • edited Loading

Webrevs

kurashige23 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

kurashige23 commented Nov 5, 2024

kurashige23 commented Nov 27, 2024

bridgekeeper bot commented Dec 25, 2024

kurashige23 commented Jan 6, 2025

bridgekeeper bot commented Feb 3, 2025

kurashige23 commented Feb 5, 2025

jerboaa commented Feb 20, 2025

franferrax commented Feb 24, 2025

jerboaa commented Feb 24, 2025

kurashige23 commented Mar 7, 2025

kurashige23 commented Mar 7, 2025 • edited Loading

jerboaa left a comment

Choose a reason for hiding this comment

jerboaa commented Mar 10, 2025

kurashige23 commented Mar 11, 2025

kurashige23 commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

jerboaa commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

openjdk bot commented Mar 12, 2025

gnu-andrew commented Mar 14, 2025

kurashige23 commented Mar 18, 2025

kurashige23 commented Oct 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 21, 2024 •

edited

Loading

mlbridge bot commented Oct 21, 2024 •

edited

Loading

kurashige23 commented Mar 7, 2025 •

edited

Loading