JDK-6676643: Improve current C_GetAttributeValue native implementation #3709

valeriepeng · 2021-04-27T02:41:12Z

Anyone can help review this somewhat trivial fix? The main change is inside src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c. This is to help better troubleshooting by reporting the type of unavailable attributes in PKCS11 exception message when C_GetAttributeValue(...) call failed. The java file changes are just cleanup for consolidating the CKR_* constants definition into PKCS11Exception class.

Thanks,
Valerie

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-6676643: Improve current C_GetAttributeValue native implementation

Reviewers

@SalusaSecondus (no known github.com user name / role) ⚠️ Review applies to a08ce8b
Xue-Lei Andrew Fan (@XueleiFan - Reviewer) ⚠️ Review applies to 285196b

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3709

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

Using diff file

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

Include mechanism info into PKCS11 exception message and remove duplicated constant definitions in PKCS11Constants and PKCS11Exception classes.

bridgekeeper · 2021-04-27T02:42:22Z

👋 Welcome back valeriep! 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 · 2021-04-27T02:44:15Z

@valeriepeng 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 · 2021-04-27T02:47:45Z

Webrevs

SalusaSecondus · 2021-04-27T04:27:10Z

src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c

+
+    if (rv != CKR_OK) {
+        if (rv == CKR_ATTRIBUTE_SENSITIVE || rv == CKR_ATTRIBUTE_TYPE_INVALID) {
+            msg = malloc(80); // should be more than sufficient


I'm worried about asserting that 80 is sufficient especially as extremely large numbers of attributes could be retrieved and the limit check later on seems a bit lax.

80 should be sufficient, as we only include attribute type of those UNAVAILABLE ones. For example, there may be 6 attributes specified, but realistically only one or two being unavailable. Are you aware of any case which a large number of attributes are unavailable? I added the limit check to ensure no out-of-bound writes. In the worst case, the reported types may be incomplete, i.e. up to 80 bytes. Otherwise, we may need two passes, one to find out the number of unavailable attributes, and one to allocate and set the content. Or, do you have other suggestion?

SalusaSecondus · 2021-04-27T04:28:26Z

src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c

+            // format msg w/ attribute(s) whose value is unavailable
+            temp1 = msg;
+            temp2 = msg + 80;
+            for (i = 0; i < ckAttributesLength && temp1 < temp2; i++) {


I think that this loop will append at most 11 bytes to the string each time (is this correct?), if so, we can make the check temp1 < temp2 - 12 to count the final null and avoid running off the end with a buffer overflow.

I apologize. This counting code is correct and doesn't have any of the issues I claimed. snprintf takes care of everything and I just missed it last night.

It's alright, thanks for checking and review.

SalusaSecondus · 2021-04-27T04:31:42Z

src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_objmgmt.c

-    if (ckAssertReturnValueOK(env, rv) != CK_ASSERT_OK) {
+
+    if (rv != CKR_OK) {
+        if (rv == CKR_ATTRIBUTE_SENSITIVE || rv == CKR_ATTRIBUTE_TYPE_INVALID) {


According to the PKCS#11v2.40 spec, CKR_BUFFER_TOO_SMALL should be handled in the same special ways as these too (in that it isn't a "true error").

For this particular call, the pValue field is null, it's meant to query the exact length of the specified attribute. Thus, CKR_BUFFER_TOO_SMALL should not be returned.
Afterwards, we then allocate the buffer based on this queried result, so CKR_BUFFER_TOO_SMALL should also not occur.
So, based on the current API usage, CKR_BUFFER_TOO_SMALL should not happen.

All my concerns are addressed then. So, while my review doesn't count towards acceptance of this change, everything LGTM.

Thanks for your review still. Good to have an extra pair of eyes to look over the changes...

SalusaSecondus · 2021-04-27T04:32:59Z

src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c

- * CKR_OK. Otherwise, it returns the returnValue as a jLong.
+ * This function generates a PKCS#11Exception with the returnValue as the
+ * errorcode. If the returnValue is not CKR_OK. The functin returns 0, if the
+ * returnValue is CKR_OK. Otherwise, it returns the returnValue as a jLong.
 *
 * @param env - used to call JNI funktions and to get the Exception class


NitPick: here and above some German seems to have slipped in. I think we want "functions"

I didn't notice the German, just copy-n-paste. Will fix the typos.

Other PKCS#11 JNI sources have the same German typos, I prefer to leave them out of this. Will fix them when they are touched in other fixes.

XueleiFan

Looks good to me except a few trivial comments.

XueleiFan · 2021-05-13T20:53:05Z

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

+    };
+
+    private static void register(String name, long errorCode) {
+        errorMap.put(errorCode, name);


I may check the put() return value and throw error for duplicated items, just in case duplicated copy-and-past register.

XueleiFan · 2021-05-13T21:08:15Z

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

+        // for unknown PKCS11 return values, just use hex as its string
+        if (res == null) {
+            res = "0x" + Functions.toFullHexString((int)errorCode);
+            errorMap.put(errorCode, res);


It would be nice if the errorMap is immutable once the known error codes get registered.

XueleiFan · 2021-05-13T21:12:24Z

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

+        register("CKR_FUNCTION_REJECTED", 0x00000200L);
+        register("CKR_TOKEN_RESOURCE_EXCEEDED", 0x00000201L);
+        register("CKR_OPERATION_CANCEL_FAILED", 0x00000202L);
+        register("CKR_VENDOR_DEFINED", 0x80000000L);


See below comment, I may have the hash map unmodifiable after the registration.

openjdk · 2021-05-13T21:44:57Z

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

6676643: Improve current C_GetAttributeValue native implementation

Reviewed-by: xuelei

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

be0a655: 8254598: StringDedupTable should use OopStorage
360928d: 8260046: Assert left >= right in pointer_delta() methods
5eda812: 8267180: Typo in copyright header for HashesTest
e90388b: 8266461: tools/jmod/hashes/HashesTest.java fails: static @test methods
599d07c: 8263006: Add optimization for Max()Node and Min()Node
16ca370: 8265694: Investigate test StressHiddenClasses.java
af4cd04: 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem
ebcf399: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0()
644f28c: 8266810: Move trivial Matcher code to cpu-specific header files
88907bb: 8266904: Use function pointer typedefs in OopOopIterateDispatch
... and 304 more: https://git.openjdk.java.net/jdk/compare/b524a81ad20871d5113ebd8373facaa6a74cfd35...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.

XueleiFan · 2021-05-14T00:15:09Z

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

+        long value() {
+            return value;
+        }


I guess it is safe to use the 'value' variable directly for enum. The value() method may be not necessary. Just a personal preference, ignore it if you like the current way.

Sure, I like it too. Will update.

Thank you. I have no more comment.

valeriepeng · 2021-05-14T23:03:12Z

/integrate

openjdk · 2021-05-14T23:04:58Z

@valeriepeng Since your change was applied there have been 315 commits pushed to the master branch:

28f1c7a: 8249395: (macos) jpackage tests timeout on MacPro5_1 systems
be0a655: 8254598: StringDedupTable should use OopStorage
360928d: 8260046: Assert left >= right in pointer_delta() methods
5eda812: 8267180: Typo in copyright header for HashesTest
e90388b: 8266461: tools/jmod/hashes/HashesTest.java fails: static @test methods
599d07c: 8263006: Add optimization for Max()Node and Min()Node
16ca370: 8265694: Investigate test StressHiddenClasses.java
af4cd04: 8266291: (jrtfs) Calling Files.exists may break the JRT filesystem
ebcf399: 8266622: Optimize Class.descriptorString() and Class.getCanonicalName0()
644f28c: 8266810: Move trivial Matcher code to cpu-specific header files
... and 305 more: https://git.openjdk.java.net/jdk/compare/b524a81ad20871d5113ebd8373facaa6a74cfd35...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7ab6dc8.

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

JDK-6676643: Improve current C_GetAttributeValue native implementation

a08ce8b

Include mechanism info into PKCS11 exception message and remove duplicated constant definitions in PKCS11Constants and PKCS11Exception classes.

openjdk bot added the rfr Pull request is ready for review label Apr 27, 2021

openjdk bot added the security security-dev@openjdk.org label Apr 27, 2021

SalusaSecondus reviewed Apr 27, 2021

View reviewed changes

SalusaSecondus approved these changes Apr 27, 2021

View reviewed changes

Fix typos, e.g. funktion, lsit, functin.

285196b

XueleiFan approved these changes May 13, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 13, 2021

PKCS11Exception class refactoring - use enum instead of HashMap

c08f62f

XueleiFan reviewed May 14, 2021

View reviewed changes

Use the final field "value" directly and remove value() method.

057c3df

openjdk bot closed this May 14, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 14, 2021

valeriepeng deleted the JDK-6676643 branch May 14, 2021 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-6676643: Improve current C_GetAttributeValue native implementation #3709

JDK-6676643: Improve current C_GetAttributeValue native implementation #3709

valeriepeng commented Apr 27, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

mlbridge bot commented Apr 27, 2021 •

edited

Loading

SalusaSecondus Apr 27, 2021

valeriepeng Apr 27, 2021

SalusaSecondus Apr 27, 2021

SalusaSecondus Apr 27, 2021

valeriepeng Apr 27, 2021

SalusaSecondus Apr 27, 2021

valeriepeng Apr 27, 2021

SalusaSecondus Apr 27, 2021

valeriepeng Apr 29, 2021

SalusaSecondus Apr 27, 2021

valeriepeng Apr 27, 2021

valeriepeng Apr 27, 2021

XueleiFan left a comment

XueleiFan May 13, 2021

XueleiFan May 13, 2021

XueleiFan May 13, 2021

openjdk bot commented May 13, 2021 •

edited

Loading

XueleiFan May 14, 2021

valeriepeng May 14, 2021

XueleiFan May 14, 2021

valeriepeng commented May 14, 2021

openjdk bot commented May 14, 2021

JDK-6676643: Improve current C_GetAttributeValue native implementation #3709

JDK-6676643: Improve current C_GetAttributeValue native implementation #3709

Conversation

valeriepeng commented Apr 27, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

mlbridge bot commented Apr 27, 2021 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XueleiFan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriepeng commented May 14, 2021

openjdk bot commented May 14, 2021

valeriepeng commented Apr 27, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 27, 2021 •

edited

Loading

openjdk bot commented May 13, 2021 •

edited

Loading