444367 - NSS 3.12 softoken returns the certificate type of a certificate object as CKC_X_509_ATTR_CERT.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Joe Orton reported in the mozilla.dev.tech.crypto newsgroup:

  For a test suite I'm importing a PKCS#12 cert into a fresh
  database as follows:

   rm -rf nssdb
   echo foobar > nssdb.pw

   ${CERTUTIL} -d nssdb -N -f nssdb.pw
   ${PK12UTIL} -d nssdb -K foobar -W '' -i unclient.p12

  and then using that database with the softokn PKCS#11
  module.

  With NSS 3.11, doing a FindObjects search for an object
  in this database with a CKA_CLASS of CKO_CERTIFICATE
  would return one object, which would have a
  CKA_CERTIFICATE_TYPE of CKC_X_509.  This matched how other
  hardware tokens would work.

  With NSS 3.12, I'm seeing that the certificate type of
  the single object with class of CKO_CERTIFICATE has changed
  to CKC_X_509_ATTR_CERT.  Is this expected behaviour?

Since NSS doesn't support X.509 attribute certificates, this
is definitely a bug.  Looking at the NSS 3.12 source code
(http://lxr.mozilla.org/security/search?string=CKC_X_509_ATTR_CERT)
I can't see where the bug is.  Since the value of
CKC_X_509_ATTR_CERT is 1, we may have incorrectly returned some
other constant whose value is also 1.  In particular, LG_CERT
is defined as 1 in mozilla/security/nss/lib/softoken/legacydb/lgfind.c
(http://lxr.mozilla.org/security/source/security/nss/lib/softoken/legacydb/lgfind.c#90)

Comment 1

[Elio Maldonado]

It's been a while since it was entered. Any comments on this bug?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Let's ask Joe.
Joe, Were you saying that this problem is peculiar to a particular 
certificate or PKCS#12 file you were using?  Or do you believe it is 
common to all certificates found in an NSS 3.12 DB by C_FindObjects?

If it is peculiar to a particular cert or file, Would you be willing to 
provide a PKCS#12 file with which the problem can be reproduced?

Obviously the 3.11 test was done with the old cert8.db files, because
we didn't have cert9 in 3.11.  Was your 3.12 test using cert8 or cert9?

I presume this problem was seen on Linux.  Is that right?

Comment 3

[Joe Orton]

1) This is reproducible with PKCS#12 test certs created by OpenSSL.  I can't say whether this is something specific to the way OpenSSL creates PKCS#12 certs.

2) I will attach the test file my test case creates.

3) On Fedora 12, with nss-3.12.6-4.fc12.x86_64, the contents of the NSS directory after running the certutil/pk12util commands was:

-rw-------. 1 root root 65536 2010-07-14 16:33 cert8.db
-rw-------. 1 root root 16384 2010-07-14 16:33 key3.db
-rw-------. 1 root root 16384 2010-07-14 16:33 secmod.db

4) Yes, I've reproduced on Fedora and RHEL.

Comment 4

[Joe Orton]

Attachment: PKCS#12 client cert

Comment 5

[Elio Maldonado]

Attachment: test application to try to reproduce the bug

I tried to reproduce the bug with this application but couldn't. It shows the correct type. Using nss 3.12.8. I'm not sure if the application is correct. Will attach a convenience makefile.

Comment 6

[Elio Maldonado]

Attachment: convinience makefile for testing

It uses a legacy db as in the bug report. I tried also with a shared db and it worked.

Comment 7

[Joe Orton]

That code doesn't look right, am I missing something?

    CK_OBJECT_HANDLE hObject = 0, o = 0;
...
    rv = pFunctionList->C_GetAttributeValue(hSession, o, pTemplate, 1);

is trying to retrieve an attribute of the wrong object - should be hObject not "o".  Is that function returning success, rv isn't checked?  Why call C_GetAttributeValue twice?

Comment 8

[Joe Orton]

I can't reproduce the CKC_X_509_ATTR_CERT-specific failure any more with NSS.  It's possible I was screwing something up.

I debugged this some more.  I think the failure I see is due to this change to lgutil.c:lg_GetULongAttribute --

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=lgutil.c&branch=&root=/cvsroot&subdir=mozilla/security/nss/lib/softoken/legacydb&command=DIFF_FRAMESET&rev1=1.2&rev2=1.3

The failure is pass a search template to FindObjectsInit like: 

   CK_CERTIFICATE_TYPE type = CKC_X_509
   CK_ATTRIBUTE template = { CKA_CERTIFICATE_TYPE, &type, sizeof(type) };

on 64-bit platforms with nss 3.11 this was working.

with 64-bit nss 3.12 this now fails (FindObjects finds no objects) because of the referenced change; if I replace sizeof(type) with 4 it works again.

AFAICT this template is correct use of the API - it's used in the PKCS#11 spec.

Comment 9

[Joe Orton]

Attachment: updated test case

This is an update of the test case which reproduces the failure.

Comment 10

[Wan-Teh Chang]

Joe: thanks a lot for tracking down the change to lg_GetULongAttribute in comment 8.  I believe lg_GetULongAttribute was sftk_GetULongAttribute in NSS 3.11:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11u.c&rev=1.69.2.5&mark=2047-2064#2047

I suspect we changed to copy byte by byte to avoid unaligned attribute->pValue.  But the byte-by-byte copy also results in a network-to-host byte order conversion.  It assumes the source, attribute->pValue, is in network byte order (big endian).  I am not sure if that was intentional.

Without knowing what the code is intended to do, I don't know how to fix this bug.

Bob, do you remember?

Comment 11

[Robert Relyea]

So what is sent across the PKCS #11 boundary is supposed to be host order and native ulong length. What is stored in the database needs to be network order and 4 bytes (so the database can be moved from machine to machine). Before the lg code gets the data, it should have been converted to network order and 4 bytes by sftk_ULong2SDBULong in sftkdb_fixupTemplateIn(). This is because that database interface is supposed to allow the DB's to take the bytes at this point and just pass them on.

The value in the template that arrives to the lg_GetULongAttribute should be SBD_ULONG_SIZE (which is 4 bytes) and network byte order. lg_GetULongAttribute should probably use the latter define.

> if I replace sizeof(type) with 4 it works again.

This seems to indicate that the internal representation is correct. Since CKC_X_509 is 0, then byte order doesn't matter, a size of 4 won't be converted
by the fixup code because it's not size of ULONG, so the correct value is accidentally passed into the lg code. This seems to point to somehow 
sftkdb_fixupTemplateIn() isn't being called or isn't doing it's job.

bob

Comment 12

[Elio Maldonado]

Attachment: Fixed test case

Reorganized code and Bob fixed a bug where certType wasn't initialized.
I now get
$ ./findobject
FindObjects got 1 objects.
type=CKC_X_509

Joe, Could you try this one?

Comment 13

[Robert Relyea]

Elio's working cases were all using sdb.
NSS 3.11 did not validate CKA_CERTIFICATE_TYPE in the big presearch case. The 3.12 bug was there. Attaching patch.

Comment 14

[Robert Relyea]

Attachment: When validating CKA_CERTIFICATE_TYPE, actually fetch the certType, not the class!

Comment 15

[Robert Relyea]

Comment on attachment 485180 [details] [diff] [review]
When validating CKA_CERTIFICATE_TYPE, actually fetch the certType, not the class!

other reviewers welcome as well.

bob

Comment 16

[Joe Orton]

Ah, sorry for sending you done another blind alley there guys :(

Comment 17

[Elio Maldonado]

Comment on attachment 485180 [details] [diff] [review]
When validating CKA_CERTIFICATE_TYPE, actually fetch the certType, not the class!

Looks like the right thing to do and I verified it fixes the problem. 

I reproduced the bug and verified the fix on fedora 13 with builds from TRUNK as well as a fedora 13 build for nss-softokn 3.12.8.

Comment 18

[Wan-Teh Chang]

Comment on attachment 485180 [details] [diff] [review]
When validating CKA_CERTIFICATE_TYPE, actually fetch the certType, not the class!

r=wtc.  I'm glad you found the bug.  I tried to debug
this for two hours the other day but couldn't track it
down.

I examined all lg_GetULongAttribute calls and verified
that there is no other mistake of this kind.

Bob, please review this function and see if more 'break'
statements should be added after we detect an error.
I suspect we need a break statement at all of these places:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/lgfind.c&rev=1.5&mark=748,758,768,796,#746

Comment 19

[Robert Relyea]

> Ah, sorry for sending you done another blind alley there guys

'tis ok, at least the test lead us to the real bug.

> Bob, please review this function and see if more 'break'
> statements should be added after we detect an error.

Yes, those should get breaks. Fortunately there isn't really a bug since the loop never adds bits, just clears them, but we really should break out once we know we can't go further. (and if the bool length isn't correct, we really shouldn't dereference the pointer -- even though bool is only 1 byte).

The KRL case may a actually be a bug. Fortunately KRL's are FORTEZZA things. I haven't seen one in a very long time...

bob

Comment 20

[Robert Relyea]

obs-laptop(149) cvs commit
cvs commit: Examining .
? patch
Checking in lgfind.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgfind.c,v  <--  lgfind.c
new revision: 1.6; previous revision: 1.5
done
bobs-laptop(150)

Comment 21

[Wan-Teh Chang]

Patch checked in on the NSS_3_12_BRANCH (NSS 3.12.9).

Checking in lgfind.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgfind.c,v  <--  lgfind.c
new revision: 1.4.62.1; previous revision: 1.4
done