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

[rebarbora-mckvak]

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)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

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

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

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[bot]]

@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[bot]]

Webrevs

• 04: Full - Incremental (1300248d)
• 03: Full - Incremental (ab60267b)
• 02: Full (7f5c04fe)
• 01: Full - Incremental (04a47361)
• 00: Full (e1a0a22e)

[bridgekeeper[bot]]

@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]

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

[wangweij]

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

[macarte]

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]

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[bot]]

@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]

@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]

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

[macarte]

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]

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[bot]]

@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]

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

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

2. 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]

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]

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]

@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]

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]

@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 https://github.com/openjdk/jdk/pull/16687#issuecomment-1978248822, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFUF6UIAATCKRDBLNIR3LK3YWWB3JAVCNFSM6AAAAAA7OBIQAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZYGI2DQOBSGI . You are receiving this because you commented.Message ID: @.***>

[macarte]

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]

@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]

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.

[macarte]

If its decided that when the store is opened with readonly access that no warning is output then I'll go with the majority (please raise this discussion in the mailing list).

However, before "signing off" I'd like to see the output of manually testing the keystore API when opened in readonly mode using this method.

I believe that with this change we can modify the original jtreg test as the test will be able to enumerate the keystore and load it without an access denied exception

[openjdk[bot]]

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

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

Reviewed-by: weijun

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

• a4582a8957d604b50249e1f59679393966456a14: 8334166: Enable binary check
• 7baddc202a9ab2b85401aa05f827678b514ebf55: 8334339: Test java/nio/file/attribute/BasicFileAttributeView/CreationTime.java fails on alinux3
• eb110bdc6e8bcb87b9b8b24ac66eb9b4c57106fd: 8334180: NMT gtests introduced with 8312132 should be labeled as NMT
• 652784c803863f40ee3d81695a19e705365cb800: 8334392: Switch RNG in NMT's treap
• 72ca7bafcd49a98c1fe09da72e4e47683f052e9d: 8334708: FFM: two javadoc problems
• 7e55ed3b106ed08956d2d38b7c99fb81704667c9: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS
• 1ff5acdafff1ccd3e64c70eebbfbff75e0d783eb: 8332099: since-checker - Add @ since to package-info in jdk.jsobject
• 689cee3d0950e15e88a1f6738bfded00655dca9c: 8334509: Cancelling PageDialog does not return the same PageFormat object
• 8e1d2b091c9a311d98a0b886a803fb18d4405d8a: 8334441: Mark tests in jdk_security_infra group as manual
• 93d98027649615afeeeb6a9510230d9655a74a8f: 8334715: [riscv] Mixed use of tab and whitespace in riscv.ad
• ... and 2751 more: https://git.openjdk.org/jdk/compare/c36ec2ca70248c2e4676fd725fbb132c3b929908...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.

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

[wangweij]

I'm trying out this on my Windows Server 2016 box as a domain user. The command prompt window always has "Administrator:" in its title. What can I do?

[wangweij]

I finally found a toy Windows 11 Home Edition machine and this code change works. The original AllTypes.java test also passed after I removed the detectIfRunningWithAdminPrivileges check.

However, I see something different from the bug report. After creating the entry as shown in step 2, it is listed as a private key entry in Windows-MY-LOCALMACHINE when user has admin privilege. Otherwise, it's just a cert entry, which means the private key is not visible. In fact, when I tried the signtool utility there, it also does not work and says "no certificate were found that met all the given criteria".

So what I observed is even lower than the "non-elevated command prompt" described in the bug report, where the private key is at least visible.

I don't know if it's possible to detect this situation. If an entry was created as a self-signed cert with a private key but only the cert is visible, I wonder if we should show it as a cert entry to the user. Will this confuse users?

[wangweij]

I also noticed a different problem. No matter if privileged or unprivileged, keytool -genkeypair -storetype Windows-My-LOCALMACHINE works successfully but the entries are actually created in Windows-MY-CURRENTUSER. This is unrelated to this code change and I filed https://bugs.openjdk.org/browse/JDK-8328184.

[openjdk[bot]]

@rebarbora-mckvak Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[rebarbora-mckvak]

First, I am sorry I had to force-push the branch, because I accidentally clicked on some github UI and it merged master branch without asking for any confirmation.

I added another commit which fixes signing with some certificates. It seems it matters how the keys are imported in the local machine store and sometimes it means signHash is used and produced Keyset does not exist error.