Closed Bug 359473 Opened 18 years ago Closed 17 years ago

PK11_GetAttributes returns OK for an object with no ID

Categories

(NSS :: Libraries, defect, P2)

3.11.3
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil.williams, Assigned: neil.williams)

Details

Attachments

(1 file, 2 obsolete files)

Pk12util -o sees SEC_ERROR_BAD_KEY when there are no user certs associated with the specified nickname. Drilling down into the cause shows it being set in PK11_MatchItem with this stack

  [1] PR_SetError(code = -8178, osErr = 0), line 58 in "prerror.c"
  [2] PORT_SetError(value = -8178), line 181 in "secport.c"
=>[3] PK11_MatchItem(slot = 0x80aa4b8, searchID = 4047854611U, matchclass = 3U), line 1483 in "pk11obj.c"
  [4] nssToken_IsPrivateKeyAvailable(token = 0x80aed78, c = 0x80b2a50, instance = 0x807d958), line 1718 in "devtoken.c"
  [5] NSSCertificate_IsPrivateKeyAvailable(c = 0x80b2a50, uhh = (nil), statusOpt = (nil)), line 769 in "certificate.c"
  [6] nssTrust_GetCERTCertTrustForCert(c = 0x80b2a50, cc = 0x80b18d0), line 622 in "pki3hack.c"
  [7] fill_CERTCertificateFields(c = 0x80b2a50, cc = 0x80b18d0, forced = 0), line 793 in "pki3hack.c"
  [8] stan_GetCERTCertificate(c = 0x80b2a50, forceUpdate = 0), line 850 in "pki3hack.c"
  [9] STAN_GetCERTCertificateOrRelease(c = 0x80b2a50), line 891 in "pki3hack.c"
  [10] PK11_FindCertsFromNickname(nickname = 0x8046c21 "clientCA", wincx = 0x80469c0), line 741 in "pk11cert.c"
  [11] P12U_ExportPKCS12Object(nn = 0x8046c21 "clientCA", outfile = 0x807e270 "x.pfx", inSlot = 0x80aa4b8, slotPw = 0x80469c0, p12FilePw = 0x80469b8), line 594 in "pk12util.c"
  [12] main(argc = 7, argv = 0x80469f4), line 958 in "pk12util.c"

At this point PK11_GetAttributes(objhandle) has returned CKR_OK but the value of  the CKA_ID attribute returned is zero. It seems either objhandle is invalid, in which case PK11_GetAttributes should complain, or the object itself is invalid because it must have an ID.
> At this point PK11_GetAttributes(objhandle) has returned CKR_OK but the 
> value of  the CKA_ID attribute returned is zero.

The LENGTH of the output CKA_ID attribute is zero.  
The address of that attribute value is non-NULL and points at memory 
that has been initialized with 0xdadadada, suggesting it is memory that is
either unallocated (freed), or allocated but uninitialized.

The other attribute value is also bogus.  It is the CKA_OBJECT_CLASS attribute.
It's output length is 4, and the address of the output value also points at
memory containing 0xdadadada.

So, there are several potential errors here, including;
a) the object handle ("searchID") was invalid, but PK11_GetAttributes failed
to detect/report that fact, and/or 
b) the object handle identified an improperly constructed object in the 
softoken (a bug in softoken), or 
c) a bug in GetAttributes caused the output values to be invalid, even though
the handle was valid and the true object attributes were valid.  

Neil, I'm asking you to investigate this further.  
One more question/observation.  As seen in the stack, the arguments to 
NSSCertificate_IsPrivateKeyAvailable appear that they might be invalid.  
Is that yet another bug?  
Assignee: nobody → neil.williams
Priority: -- → P2
Target Milestone: --- → 3.11.8
Target Milestone: 3.11.8 → Future
Target Milestone: Future → ---
OS: SunOS → All
Hardware: PC → All
PK11_MatchItem is explicitly for finding a "peer" of an object. A comment says "searchID is typically a privateKey or a certificate while the peer is the opposite". So this stack is not asking about an attribute of a key, it's asking about a cert. C_GetAttributeValue returns a staticNullAttr when the CKA_ID of a cert is requested. So this all appears correct.

Response to comment 1

a) handle was valid, validity is checked in C_GetAttributeValue.
b) the attribute returned is explicitly constructed.
c) it passes back what was returned to it--a special return value which means peer does not exist.

The 2 null arguemnts passed to NSSCertificate_IsPrivateKeyAvailable are OK: neither one is used.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → INVALID
I think you've uncovered several functions that are sloppy about 
how they say "no such beast was found".  

SEC_ERROR_BAD_KEY is nt a valid error return for function
PK11_FindCertsFromNickname.  Some error mapping is evidently missing.

Returning a non-zero length with a pointer to freed memory is a bug.

There is a special CKA_ID (value zero) that means "no such ID", but
its length is not zero.  It's length is the length of a CKA_ID.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Let's take each issue separately:

1. PK11_GetAttributes returns CKR_OK and bogus data: (The bogus data in this case is the CKA_OBJECT_CLASS, CKA_ID with zero length is fine). This would be a bug in softoken. If we failed to get the attributes, we should not return CKR_OK. If the object exists it should never return a bad CKA_OBJECT_CLASS and not return an error. (This is particularly surprising since CKA_OBJECT_CLASS is taken from the object handle in the dbm version of softoken).

2. Bigger question, where are we getting an object handle from if there are no certificates: F1455C13 is clearly a softoken handle for a certificate. If the certificate does not exist, how did we get it? (This may provide some answer to way we are seeing weirdness in case 1). This is also why we are getting into PK11_MatchItem to begin with. (we should have failed in the find).

3. SEC_ERROR_BAD KEY return from PK11_MatchItem: P11_MatchItem can take any type  of object as an input as long as the object has a CKA_ID. It will then look for the kind of item specified by matchclass. I see a couple possible solutions. 1) change MatchItem to pass in the objectclass of the parent object so we can us it to get the error code right, 2) try to guess the parent object class from the match class, 3) require callers of PK11_MatchItem to reset the error code. I think option 1 is probably best.

The primary failure is case 2. Why are we getting here? That should point out where the incorrect return case is for 1 (which is a separate issue). Case 3 is also a separte issue. Once case 2 is resolved, Case 1 and 3 should go away in this particular case.

Neil, what exactly are the steps you go through to reproduce this problem?

> There is a special CKA_ID (value zero) that means "no such ID", but
> its length is not zero.  It's length is the length of a CKA_ID.

That's not true. The special CKA_ID of zero was a cheat to mark certain certs as trusted before we had a way of doing that explicitly. If CKA_ID is not set, the length of zero is exactly what we want.
Here's what I was doing to reproduce the problem:

cd mozilla/tests_results/security/test.1
pk12util -o x.pfx -d alicedir -K nss -n TestCA

From comment 3 SEC_ERROR_BAD_KEY is not being returned by PK11_FindCertsFromNickname which successfully returns a cert list. That cert list is then filtered for user certs. If the result is a null list pk12util prints "no user certs from given nickname" and exits, which is pretty clear.

Bob, regarding comment 4:
1) PK11_GetAttributes returns CKA_CLASS as 1 (4 bytes). The 0xdada... appears in memory after that. No problem here. PK11_GetAttributes does allocate memory for attributes whose length is zero, but this seems to be a nit.

2) There is a valid certificate--just no private key for it.

3) Is it so bad to have SEC_ERROR_BAD KEY mean "the peer for the object you're trying to match does not exist?
In that case it is almost always wrong for PK11_MatchItem to return it. MatchItem is almost never dealing with the public keys (there are a few cases).
This is the only bug I could find the situation reported here. Bob, Nelson, you agree?
Attachment #267736 - Flags: review?(rrelyea)
Attachment #267736 - Attachment is obsolete: true
Attachment #267736 - Flags: review?(rrelyea)
Looks like the answer to my question is going to be no. How about this? In PK11_MatchItem check the requested match class and set the error based on it.
Attachment #267745 - Flags: review?(rrelyea)
I think we're tying to say "not found", rather than "invalid".
BAD_KEY says "invalid".
Unless and Until we can reproduce the invalid object class issue that 
I reported in comment 1, I agree that the only two remaining bugs to 
fix are the two that the patch attachment 267745 [details] [diff] [review] attempts to fix.
I see no error in secerr.h that means, or implies, key not found. Should one ne added?
Adding a key-not-found error seems entirely reasonable to me.
I have two issues that need to be answered about this:

1) This error is triggered if the cert does not have a CKA_ID. It's possible the key exists, but the cert's CKA_ID value was damanged. In that case, the error would be confusing.

2) If you put the error in PK11_MatchItem(), then you also have to deal with the fact the source object is a key, not a cert. In this case the key would exist, but doesn't have a matching cert (or it's CKA_ID value was damaged).

I'm not against the error value because it's better than what we have, and 1) is probably rare. I would prefer to see the error changed higher up on the stack, however, particulary in view of issue 2.

bob
IINM (Pls tell me if I am), the ONLY way that we find a private key for a 
cert is by searching for a key with the matching CKA_ID.  If the CKA_ID of 
either cert or key is damaged, we completely LOSE the ability to find the 
key for the cert, and so the key *is* lost (for all intents and purposes).

I agree that we should return an error code that correctly identifies the
type of missing or unfound object (key or cert).
Actually, we can find a private key with C_FindObjects looking for all private keys, but you are right in practice. We never really use a key unless there is a cert pointing to it. 
Comment on attachment 267745 [details] [diff] [review]
previous patch plus change error returned by PK11_MatchItem

r- = rrelyea.

The first part of the patch is fine (though I am a little worried about returning NULL pointers to the code above, it should be fine).

The second half of the patch though...

matchclass is the name of the class of object we are trying to find. The class of the object we are checking is not passed to this function. So we should check machclass == CKO_CERTIFICATE and return BAD_KEY in that case. If we are looking up a certificate, we are looking up the certificate either from the public key or the private key, so the object we are passing in is a key.

if matchclass is CK_PUBLIC_KEY or CKO_PRIVATE_KEY, then most likely the source object is a CERT (thought it could be a key). In either case NO_KEY is appropriate (the key we are trying to find doesn't exist).

(r+ for this patch with CKO_PUBLIC_KEY-> CKO_CERTIFICATE)
Attachment #267745 - Flags: review?(rrelyea) → review-
Your assessment in comment #17 makes good sense to me, Bob. I see only 1 place in NSS where PK11_MatchItem is called with CKO_CERTIFICATE and in that case, if INVALID_HANDLE is returned by MatchItem the error is explicity set to SSL_ERROR_NO_CERTIFICATE.

So, regarding your conditional r+ I'll check in this version.
Attachment #267745 - Attachment is obsolete: true
Checking in lib/pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.16; previous revision: 1.15
done
Checking in lib/pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.16; previous revision: 1.15
done
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: