8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation #16687

rebarbora-mckvak · 2023-11-16T12:06:26Z

This fixes the defect described at https://bugs.openjdk.org/browse/JDK-8313367

If the process does not have write permissions, the store is opened as read-only (instead of failing).

Please note that permissions to use a certificate in a local machine store must be granted - in a management console, select a certificate, right-click -> All tasks... -> Manage Private Keys... -> add Full control to user.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation (Bug - P3)

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16687/head:pull/16687
$ git checkout pull/16687

Update a local copy of the PR:
$ git checkout pull/16687
$ git pull https://git.openjdk.org/jdk.git pull/16687/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16687

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16687.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-16T12:07:35Z

Hi @rebarbora-mckvak, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rebarbora-mckvak" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2023-11-16T12:09:45Z

@rebarbora-mckvak 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 · 2023-11-20T23:38:42Z

Webrevs

bridgekeeper · 2023-12-19T05:59:23Z

@rebarbora-mckvak 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!

rebarbora-mckvak · 2023-12-19T11:52:01Z

This is a very trivial change fixing rather annoying bug. Can someone review it and let it merge?

wangweij · 2023-12-19T13:43:48Z

@macarte, can you take a look at this fix? Thanks.

macarte · 2023-12-19T17:36:02Z

Apologies I wasn't aware of the JBS issue until I saw this github notification.

At a glance the fix seems trivial, but I'll need to test it. We were planning on looking at supporting the ability to open a keystore with READONLY access and I emailed security-dev to this effect in May

Considering this change, I'd suggest that when the store is opened with read-only permissions that some warning is output (if this can't be detected then we may have to attempt to open with write-privileges and the fall back to read-only (CERT_STORE_READONLY_FLAG).

@rebarbora-mckvak - what testing was done with a NON-elevated user opening a keystore with (CERT_STORE_MAXIMUM_ALLOWED_FLAG) and then attempting write-operations on the keystore?

wangweij · 2023-12-19T18:31:46Z

Also, when a keystore is read-only. What happens when one tries to write into it? Ideally a KeyStoreException should be thrown with a clear and precise message.

bridgekeeper · 2024-01-16T22:07:10Z

@rebarbora-mckvak 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!

rebarbora-mckvak · 2024-01-17T15:35:13Z

@rebarbora-mckvak - what testing was done with an elevated user opening a keystore with (CERT_STORE_MAXIMUM_ALLOWED_FLAG) and then attempting write-operations on the keystore?

I have not tested any write operations yet. My use case has always been: administrators put private keys in the store and give a service (my java app) permissions to use it e.g. to sign data.

Also, when a keystore is read-only. What happens when one tries to write into it? Ideally a KeyStoreException should be thrown with a clear and precise message.

I guess you get the same "Access denied" error now as before.

wangweij · 2024-01-17T17:20:12Z

Oh, I re-read the bug report. It looks like a KeyStoreException was thrown. That's good.

macarte · 2024-01-24T00:41:15Z

Please enable github actions so that minimal tier1 jtreg tests are run; as you're changing the behavior of KeyStore.load (for SunMSCAPI) you should really test that all available write operations fail as expected (as they did before this change).

rebarbora-mckvak · 2024-01-25T18:11:03Z

I enabled gh actions and started build of windows platforms (all other options left empty/default). Hopefully github lets you to look at the results - https://github.com/rebarbora-mckvak/jdk/actions/runs/7658564889

bridgekeeper · 2024-02-22T18:12:21Z

@rebarbora-mckvak 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!

MustavData · 2024-03-04T21:55:41Z

@macarte : In your comment on JDK-8313367, you suggest that perhaps "this is a feature request". What do you mean by that?

JDK-6782021 provides the Microsoft CNG interface for an OpenJDK application, such as jarsigner.exe, to function as a native Windows application when integrated with something like Azure Key Vault. On a secure system, by the principle of least privilege, the following accesses would inhibit a bad actor from committing code signing forgery:

Builder user access:
a) Code signing authorization
b) Read-only access to the Local Computer keystore.
c) No membership in the local Administrators group.
System admin access:
a) No code signing authorization
b) Read-write access to the Local Computer keystore.
c) Membership in the local Administrators group

These accesses provide complete, unambiguous separation of roles. A user with dual roles must remain vigilant to accurately simulate a production environment when verifying this fix. Code signing authorization would be assigned, for example, by a key vault admin. A system admin privilege is not also needed. So, for this test, an elevation prompt should be seen as a red flag.

wangweij · 2024-03-04T22:29:16Z

For "this is a feature request", I think Mat means the type of this JBS issue should be an "Enhancement" instead of a "Bug".

macarte · 2024-03-04T23:55:50Z

For clarification I've edited the comment in the JBS issue, replacing "feature request" with "enhancement" so that it properly matches the terminology used in JBS.

MustavData · 2024-03-06T21:55:47Z

@macarte : Thank you for the clarification. But why do you think this issue should be an Enhancement? It appears to be a minor scope, high impact defect that would block an application from production deployment.

Omission of a secure environment test for security Enhancement JDK-6782021 could not have been intentional. Its underlying requirement, like a lock on a car door, is implicit. And, even if the fix is broader than the elegant change proposed by @rebarbora-mckvak, a documentation change should be unnecessary.

Given the threat of forgery, security around code signing is not optional. Windows can be part of a secure platform for activities such as this, but not when applications leave issues like this unresolved.

rebarbora-mckvak · 2024-03-06T22:13:09Z

I have encountered a related problem on my customer's system. Depending on how private keys are imported in the store, either signCngHash or signHash is used. signHash fails to find the key, because it does not look at local machine's store. I will commit a fix for that soon in this PR.

MustavData · 2024-03-13T14:42:09Z

@stepan-atypon-com : This looks like a helpful comment. But as your first comment on Github, no one can see it until you return to that page and check the box indicated you agree to the terms of use. Then everyone can view your posting. :-)

…

On Tue, Mar 5, 2024 at 2:54 AM Stepan Schejbal ***@***.***> wrote: I have encountered another related problem on one of my customer systems - signing with some private keys fails when non-administrator account is used. I will add another patch after some more testing. — Reply to this email directly, view it on GitHub <#16687 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFUF6UIAATCKRDBLNIR3LK3YWWB3JAVCNFSM6AAAAAA7OBIQAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZYGI2DQOBSGI> . You are receiving this because you commented.Message ID: ***@***.***>

macarte · 2024-03-13T16:57:17Z

I welcome your contribution and feel that it will be a worthwhile improvement; and I'm happy to give feedback (and have done already), but as an author I'm not able to sponsor this change.

The original enhancement has gone through a review process and has not had any security related bugs raised against it. The scenario this change targets is a valid one, but is not a security vulnerability as the original enhancement does not circumvent security. The choice to deploy to a less-secure environment to use the feature is a user choice.

While the change in this PR is trivial, it is still classed as an enhancement as it's not addressing a bug; ie. the original change functions as expected. That you have identified a scenario and supplied a patch is much appreciated; however the change will need to go through review as it changes functionality; again we'll need to consider informing the user as the change could lead to unexpected deployment issues (it may also require documentation changes [CSR]).

wangweij · 2024-03-13T18:17:11Z

@macarte If you find the change ok, you can always add yourself as a reviewer even if the OpenJDK bot might not count you as a Reviewer.

wangweij · 2024-03-13T18:24:07Z

A CSR is needed if there will be a spec or doc change, and I don't see we need it anywhere. It is also needed if there is a non-trivial compatibility issue. While there is some behavior change here I don't think it's a compatibility issue. Those worked still work and those didn't work now somehow work. To inform the user this behavior change it looks like a release note is enough. See https://openjdk.org/guide/#release-notes.

rebarbora-mckvak · 2024-10-15T08:29:28Z

@wangweij, please see the release note above.

wangweij · 2024-10-15T17:04:53Z

@rebarbora-mckvak I've added the release note as https://bugs.openjdk.org/browse/JDK-8340661 last month.

I didn't approve because of Mat's Aug 14 comment:

I think there's at least two issues that need to be addressed (see above comments)

@macarte Is this still your current opinion?

wangweij · 2024-10-15T17:06:45Z

@rebarbora-mckvak BTW, I have a small comment on the test.

wangweij · 2024-10-15T17:07:31Z

test/jdk/sun/security/mscapi/AllTypes.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.


christophvw · 2024-10-29T08:54:27Z

Any news on this issue? I would like to use certificates from the windows cert store for our tomcat servers, because these will get automatically renewed by AD cert templates.

The problem is: it currently works only when the tomcat server is running as LocalSystem.

I need a solution which works with a gMSA - without admin rights.

macarte · 2024-10-29T15:34:37Z

@rebarbora-mckvak I've added the release note as https://bugs.openjdk.org/browse/JDK-8340661 last month.

I didn't approve because of Mat's Aug 14 comment:

I think there's at least two issues that need to be addressed (see above comments)

@macarte Is this still your current opinion?

Yes I believe these are two potential issues that should at least be reviewed (the issues are outlined in the comments); IIRC they should be trivial to address

christophvw · 2024-10-29T15:52:21Z

@macarte that's a lot to read. could you give a short summary of the issues?

@wangweij

So my concern is that inside Windows-MY-LOCALMACHINE, this entry actually contains a private key. But because of user privilege missing, the private key is not available and it shows as a certificate entry.

That's by design. When the user needs access to the private key, assign the proper permissions.
Select the certificate in the MMC Certs snap-in -> right click -> All tasks -> Manage private keys -> give the user read permissions to the private key

macarte · 2024-10-29T18:01:15Z

@macarte that's a lot to read. could you give a short summary of the issues?

@wangweij

So my concern is that inside Windows-MY-LOCALMACHINE, this entry actually contains a private key. But because of user privilege missing, the private key is not available and it shows as a certificate entry.

That's by design. When the user needs access to the private key, assign the proper permissions. Select the certificate in the MMC Certs snap-in -> right click -> All tasks -> Manage private keys -> give the user read permissions to the private key

christophvw · 2024-10-29T19:12:44Z

ah, I expected to find the comments in the chat and didn't look at the code... You are right about the open issues - but I personally don't care about the signHash changes. When the author doesn't fix this - what about just merging the initial fix e1a0a22 and the test case 1eff576?

rebarbora-mckvak · 2024-10-29T19:52:29Z

Wierd, I do not see those comments in github. Anyway it is fixed now.

macarte · 2024-10-29T20:06:02Z

Wierd, I do not see those comments in github. Anyway it is fixed now.

@rebarbora-mckvak - change looks good to me

@wangweij - this commit addresses the two issues I highlighted, I don't have any other concerns

wangweij · 2024-10-30T21:00:33Z

Thanks for the new commit. I'll run some tests and probably approve it tomorrow.

@macarte, the screenshot you posted shows the comments in "Pending" mode which sounds like you added them to a review session but has never submitted it.

macarte · 2024-08-14T21:32:45Z

src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp

@@ -798,20 +798,16 @@ JNIEXPORT jbyteArray JNICALL Java_sun_security_mscapi_CSignature_signHash
            ::CryptGetProvParam((HCRYPTPROV)hCryptProv, PP_CONTAINER, //deprecated
                (BYTE *)pbData, &cbData, 0);

+            DWORD keysetType;
+            ::CryptGetProvParam((HCRYPTPROV)hCryptProv, PP_KEYSET_TYPE, //deprecated
+                (BYTE*)&keysetType, &cbData, 0);


I think this is a bug, cbData starts of initialized to 256 (for the length of pbData), however a sideeffect of the previous call can change that (going in to the call, informs the method of the number of bytes that can be stored in pbData, and coming out indicates the number of bytes actually store)

for this second call to CryptGetProvParam cbData should be the number of bytes in the DWORD keysetType.

macarte · 2024-08-14T21:38:00Z

src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp

@@ -798,20 +798,16 @@ JNIEXPORT jbyteArray JNICALL Java_sun_security_mscapi_CSignature_signHash
            ::CryptGetProvParam((HCRYPTPROV)hCryptProv, PP_CONTAINER, //deprecated
                (BYTE *)pbData, &cbData, 0);

+            DWORD keysetType;


This should be initialized explicitly to zero (0) in case the method returns an error, we want 0 (through failure or because the CRYPT_KEYMACHINE_KEYSET flag was not used when the key was created), or we want this set by the CryptGetProvParam to CRYPT_KEYMACHINE_KEYSET

macarte · 2024-10-30T21:15:42Z

src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp

@@ -798,11 +798,15 @@ JNIEXPORT jbyteArray JNICALL Java_sun_security_mscapi_CSignature_signHash
            ::CryptGetProvParam((HCRYPTPROV)hCryptProv, PP_CONTAINER, //deprecated
                (BYTE *)pbData, &cbData, 0);

+            DWORD keysetType = 0;
+            DWORD keysetTypeLen = sizeof(keysetType);
+            ::CryptGetProvParam((HCRYPTPROV)hCryptProv, PP_KEYSET_TYPE, //deprecated


This addresses the previous concerns

wangweij · 2024-11-09T23:17:22Z

@rebarbora-mckvak If you are ready to integrate this fix, please issue the /integrate command, and someone will /sponsor it. See https://openjdk.org/guide/#life-of-a-pr.

rebarbora-mckvak · 2024-11-11T10:31:23Z

/integrate

openjdk · 2024-11-11T10:32:13Z

@rebarbora-mckvak
Your change (at version 582ef82) is now ready to be sponsored by a Committer.

wangweij · 2024-11-28T13:11:18Z

/sponsor

openjdk · 2024-11-28T13:12:50Z

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

edfe285: 8344306: RISC-V: Add zicond
d33ad07: 8334493: Remove SecurityManager Permissions infrastructure from DiagnosticCommands
56f1e4e: 8344093: Implement JEP 501: Deprecate the 32-bit x86 Port for Removal
d791f4b: 8341585: Test java/foreign/TestUpcallStress.java should mark as /native
e096660: 8345043: [ASAN] methodMatcher.cpp report reading from a region of size 0 [-Werror=stringop-overread]
1033385: 8344967: Some tests in TestFill do not use the test parameter
81c44e5: 8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs
ce9d543: 8345119: Some java/foreign tests wrongly assume aligned memory
1a07d54: 8343703: Symbol name cleanups after JEP 479
a0df0a5: 8340731: Cleanup remaining IA64 references in hotspot code
... and 4884 more: https://git.openjdk.org/jdk/compare/c36ec2ca70248c2e4676fd725fbb132c3b929908...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-28T13:13:02Z

@wangweij @rebarbora-mckvak Pushed as commit db535c8.

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

christophvw · 2024-12-09T11:39:43Z

could you please backport the fix to Java 17 LTS or at least 21 LTS?
I would like to use certificates from the Windows cert store for our tomcat deployments on Windows.

8313367: Local Computer store is opened with max. allowed permissions

e1a0a22

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 16, 2023

openjdk bot added the security security-dev@openjdk.org label Nov 16, 2023

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Nov 20, 2023

openjdk bot added the rfr Pull request is ready for review label Nov 20, 2023

rebarbora-mckvak changed the title ~~8313367: Local Computer store is opened with max. allowed permissions~~ 8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation Nov 21, 2023

Hakky54 mentioned this pull request Jan 23, 2024

Loading the keystore takes a very long time in some rare cases Hakky54/sslcontext-kickstart#444

Closed

wangweij reviewed Oct 15, 2024

View reviewed changes

copyright fixed

add89a3

8313367: getting PP_KEYSET_TYPE parameter fixed

582ef82

macarte reviewed Oct 30, 2024

View reviewed changes

wangweij approved these changes Oct 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 11, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 28, 2024

openjdk bot closed this Nov 28, 2024

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 Nov 28, 2024

8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation #16687

8313367: SunMSCAPI cannot read Local Computer certs w/o Windows elevation #16687

Conversation

rebarbora-mckvak commented Nov 16, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 16, 2023

openjdk bot commented Nov 16, 2023

mlbridge bot commented Nov 20, 2023 • edited Loading

Webrevs

bridgekeeper bot commented Dec 19, 2023

rebarbora-mckvak commented Dec 19, 2023

wangweij commented Dec 19, 2023

macarte commented Dec 19, 2023 • edited Loading

wangweij commented Dec 19, 2023

bridgekeeper bot commented Jan 16, 2024

rebarbora-mckvak commented Jan 17, 2024

wangweij commented Jan 17, 2024

macarte commented Jan 24, 2024

rebarbora-mckvak commented Jan 25, 2024

bridgekeeper bot commented Feb 22, 2024

MustavData commented Mar 4, 2024

wangweij commented Mar 4, 2024

macarte commented Mar 4, 2024

MustavData commented Mar 6, 2024

rebarbora-mckvak commented Mar 6, 2024

MustavData commented Mar 13, 2024 via email • edited Loading

macarte commented Mar 13, 2024 • edited Loading

wangweij commented Mar 13, 2024 • edited Loading

wangweij commented Mar 13, 2024

rebarbora-mckvak commented Oct 15, 2024

wangweij commented Oct 15, 2024 • edited Loading

wangweij commented Oct 15, 2024

wangweij Oct 15, 2024

Choose a reason for hiding this comment

christophvw commented Oct 29, 2024 • edited by bridgekeeper bot Loading

macarte commented Oct 29, 2024

christophvw commented Oct 29, 2024 • edited Loading

macarte commented Oct 29, 2024

christophvw commented Oct 29, 2024

rebarbora-mckvak commented Oct 29, 2024

macarte commented Oct 29, 2024

wangweij commented Oct 30, 2024

macarte Aug 14, 2024

Choose a reason for hiding this comment

macarte Aug 14, 2024

Choose a reason for hiding this comment

macarte Oct 30, 2024

Choose a reason for hiding this comment

wangweij commented Nov 9, 2024

rebarbora-mckvak commented Nov 11, 2024

openjdk bot commented Nov 11, 2024

wangweij commented Nov 28, 2024

openjdk bot commented Nov 28, 2024

openjdk bot commented Nov 28, 2024

christophvw commented Dec 9, 2024

rebarbora-mckvak commented Nov 16, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 20, 2023 •

edited

Loading

macarte commented Dec 19, 2023 •

edited

Loading

MustavData commented Mar 13, 2024 via email •

edited

Loading

macarte commented Mar 13, 2024 •

edited

Loading

wangweij commented Mar 13, 2024 •

edited

Loading

wangweij commented Oct 15, 2024 •

edited

Loading

christophvw commented Oct 29, 2024 •

edited by bridgekeeper bot

Loading

christophvw commented Oct 29, 2024 •

edited

Loading