1568886 - PK11_ReadRawAttribute support for PK11_TypeCert

Status: RESOLVED WONTFIX

(NSS :: Test, task, P3)

Description

[Marcus Burghardt]

When compiled with ASAN, the function PK11_ReadRawAttribute() return SECFailure.

This was catch during the creating of new gtests for NSSCKBI and, consequently to this function. The code sent to try-server shows a SEGV due to the way the EXPECT and ASSERTs are set in the gtest.cc. So:

• The problem is not really the SEGV, but why the PK11_ReadRawAttribute() is failing.
• It returns fail to read both new fields and other existing fields, like "CKA_NSS_MOZILLA_CA_POLICY".
• Tracking the error, this is the path:
  1 - PK11_ReadRawAttribute() calls PK11_ReadAttribute().
  2 - We stop at line 106 of pk11obj.c.
  - https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11obj.c#106
  - At this point, the crv value is "0x00000130", which is defined in "CKA_PRIME", "CKM_DES2_KEY_GEN" and in "CKR_DOMAIN_PARAMS_INVALID".
  - I didnt't find yet the definition of the function which return this value (C_GetAttributeValue).
  - The expected return of this value is "0x00000000" or "CKR_OK".
• Whitout ASAN flags everything works fine.

Here are the codes for this tests:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=8f995b91c72f

Comment 1

[Marcus Burghardt]

This problem appeared during the development of gtests for the feature created for BUG 1465613.
After some deeper analysis, it was concluded that this function was not designed to work with some certain types of handles, like handles of slots used for certificates.

One possible solution would be update the case of function PK11_ReadRawAttribute() to support PK11_TypeCert in a similar way that PK11_TypePrivKey and PK11_TypePubKey. However, as this looked to be made this way by design, we should discuss any change here.

To not block the bug 1465613 anymore, a different approach was made there.

Comment 2

[Marcus Burghardt]

Attachment: Bug 1568886 - PK11_ReadRawAttribute support for PK11_TypeCert. r=kjacobs

Comment 3

[Marcus Burghardt]

Firstly, this is nothing urgent. : )

I just pushed a patch to facilitate the discussion.
This function is only used to read the attributes and continues not possible to modify them with this patch.
However, with this patch we can facilitate the reading of all Certificate's attributes for the clients.

Any thoughts about risks of this patch?
For sure should exist a purpose of left this out when the initial code was written, but I don't know this motivations to analyse if the same worries of that time is sill valid nowadays.

Thanks,

Comment 4

[Kevin Jacobs [:kjacobs]]

I don't feel like I have enough historical knowledge to give a strong opinion on the change itself. While looking into this approach for bug 1465613, I was told that we intentionally restrict this capability (for reasons that weren't entirely clear, but having to do with it being too powerful or dangerous).

This hasn't been supported (via this function, at least) since at least 2004 and we were able to implement the distrust patch without it, so it seems we don't have any real need for it.

Comment 5

[Ryan Sleevi]

(In reply to Marcus Burghardt from comment #3)

For sure should exist a purpose of left this out when the initial code was written, but I don't know this motivations to analyse if the same worries of that time is sill valid nowadays.

(In reply to Kevin Jacobs [:kjacobs] from comment #4)

While looking into this approach for bug 1465613, I was told that we intentionally restrict this capability (for reasons that weren't entirely clear, but having to do with it being too powerful or dangerous).

Just chiming in from the peanut gallery: That's effectively correct. The historic design of NSS was to provide abstractions for each layer. Dealing with CERTCertificates or SECKEYPub/PrivKey would not need to know about the PKCS#11 layer under the hood. This is somewhat documented - the crypto wrapper / high key / high cert interfaces were all meant to hide PKCS#11.

Unfortunately, as with all projects that have things like "ABI guarantees" and "lots of users", ideals and practice are rarely aligned, and you can see throughout that things have bled through various abstractions. For example, the lack of opaque pointers through much of the NSS API, much like its contemporaneous competitor of OpenSSL (which also, until recently, wasn't very opaque), meant that implementation details bled through.

Does that help?

Comment 6

[Kevin Jacobs [:kjacobs]]

Yes, that does help. Thanks for the additional context!

Comment 7

[Marcus Burghardt]

Hi,

With this context, I read once more this mentioned documentation, and in the part about pkcs#11 and certhi is clear about that idea of abstraction.
So, as the intention of this BUG was just open a discussion and not a real necessity, I will close it bug for now.

Thanks Kevin and Ryan.