8294906: Memory leak in PKCS11 NSS TLS server

[djelinski]

C_DeriveKey with mechanisms CKM_*_KEY_AND_MAC_DERIVE always returns mac keys, even if macBits is zero. These keys must be free'd when no longer needed.

Verified that:

• SSL server configured with PKCS11-NSS provider leaks memory without this patch, does not leak memory with this patch
• The same server continues to function correctly
• Existing tier1-3 tests continue to pass with NSS; did not test any other PKCS11 providers
• new tests for AES-128-GCM-SHA256 and AES-256-GCM-SHA384 key derivation pass

---

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-8294906: Memory leak in PKCS11 NSS TLS server

Reviewers

• Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to fdbe559f

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10594

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

Using diff file

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

[bridgekeeper]

👋 Welcome back djelinski! 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]

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

Webrevs

• 02: Full - Incremental (4bb0db7d)
• 01: Full - Incremental (fdbe559f)
• 00: Full (66f529ae)

[valeriepeng]

I will take a look, thanks~

[mcpowers]

Would this fix cause problems for non-NSS providers when trying to destroy a mac key that was never allocated? Or does C_DestroyObject silently return if there's nothing to destroy?

[djelinski]

I don't know if other providers allocate mac keys here or not; NSS is the only provider I could test.
NSS's C_DestroyObject fails when asked to destroy invalid object.

The spec says:

The two MACing keys ("client_write_MAC_secret" and "server_write_MAC_secret") (if present) [..]

which suggests that the keys may be absent; it does not define how we can check if keys were generated, but it does declare that zero (CK_INVALID_HANDLE) is never a valid object handle. I'll add a check for zero.

[valeriepeng]

Changes look fine, just have one nit.
Thanks for finding and fixing this!

[valeriepeng]

BTW, if this NSS behavior is still present in newer NSS releases, we should raise the priority of this and backport it.

[openjdk]

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

8294906: Memory leak in PKCS11 NSS TLS server

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

• cb62f1c: 8295218: New KeepAliveTest.java has invalid copyright notice
• 9cf6651: 8294238: ZGC: Move CLD claimed mark clearing
• adaff7d: 8294900: Refactor ZObjArrayAllocator
• 86ec158: 8291226: Create Test Cases to cover scenarios for JDK-8278067
• 94ec729: 8263044: jdk/jfr/jvm/TestDumpOnCrash.java timed out
• bdb4ed0: 8294580: frame::interpreter_frame_print_on() crashes if free BasicObjectLock exists in frame
• 461d903: 8294841: G1: Refactor predict_base_elapsed_time_ms
• 392f35d: 8295033: hsdis configure error when cross-compiling with --with-binutils-src
• 16e0c59: 8294950: print_sys_devices_cpu_info doesn't use the provided buffer
• 5ad126f: 8295012: Arena should not derive from CHeapObj
• ... and 82 more: https://git.openjdk.org/jdk/compare/8ebebbce32c7021cc0983dece4bb2644c79e64aa...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.

[djelinski]

Thanks for the review! Comment added.

I was unable to find any working Windows NSS binaries that are less than 3 years old; apparently there are no official builds. I tried using Firefox's NSS, but for some reason Java couldn't load them.
I'll see if I can find a Linux distro with something more recent.

[mcpowers]

On Linux you might be able to use brew to get an older version of nss:

% brew tap-new $USER/local-nss
% brew extract --version=3.35 nss $USER/local-nss
% brew install nss@3.35

[djelinski]

Verified that the behavior is still present in NSS 3.82. JBS updated.
Thanks for the review!

/integrate

[openjdk]

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

• 03e63a2: 8295225: [JVMCI] codeStart should be cleared when entryPoint is cleared
• 26ac836: 8291638: Keep-Alive timeout of 0 should close connection immediately
• 6ae7e4d: 8293984: Unnecessary Vector usage in PropertyEditorSupport
• cd1357b: 8295229: Try to verify gtest version
• 90fb9a0: 8295102: Always load @lambda-form-invoker lines from default classlist
• ac19414: 8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
• 1961e81: 8289509: Improve test coverage for XPath Axes: descendant, descendant-or-self, following, following-sibling
• 5699041: 8294772: Remove workaround in os::dll_address_to_library_name
• d125265: 8277970: Test jdk/sun/security/ssl/SSLSessionImpl/NoInvalidateSocketException.java fails with "tag mismatch"
• 0475c34: 8292386: jvmti/thread/SuspendThread/suspendthrd03 failed with "FAIL: Status is 2"
• ... and 96 more: https://git.openjdk.org/jdk/compare/8ebebbce32c7021cc0983dece4bb2644c79e64aa...master

Your commit was automatically rebased without conflicts.

[openjdk]

@djelinski Pushed as commit 94caecb.

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