Bug 545779

Hmm, this sounds like a difference in the sql versus dbm databases.

Try the following:

certutil -L -d /etc/pki/nssdb

and 

certutil -L -d sql:/etc/pki/nssdb

My guess is the former will not have the test root cert that the latter does.
This can happen if the cert was added to the old DB after the upgrade (which happens when you install NSS 3.12.4 or later).

Check to see if your tests import a root cert into /etc/pki/nssdb, and if 3.12 import it into both /etc/pki/nssdb and sql:/etc/pki/nssdb.

NOTE: There may be other reasons for this failure, but this seems the most likely. It would at leat be good to elliminate it as a possibility.

bob

(In reply to comment #1)
> certutil -L -d /etc/pki/nssdb
> certutil -L -d sql:/etc/pki/nssdb

Both commands above give me the same output:
$ certutil -L -d /etc/pki/nssdb

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

> My guess is the former will not have the test root cert that the latter does.
> This can happen if the cert was added to the old DB after the upgrade (which
> happens when you install NSS 3.12.4 or later).

curl does not need any certificates in the NSS db. It can read the bundle from /etc/pki/tls/certs/ca-bundle.crt using PEM reader.

> Check to see if your tests import a root cert into /etc/pki/nssdb, and if 3.12
> import it into both /etc/pki/nssdb and sql:/etc/pki/nssdb.

The second does not work for me. Am I doing anything wrong?
$ certutil -A -n root -t C,C,C -d sql:/etc/pki/nssdb < /etc/pki/tls/certs/ca-bundle.crt
certutil: could not add certificate to token or database: Error adding certificate to database.

> NOTE: There may be other reasons for this failure, but this seems the most
> likely. It would at leat be good to elliminate it as a possibility.

Any idea why the bug appears on i686 only?

> curl does not need any certificates in the NSS db. It can read the bundle from
> /etc/pki/tls/certs/ca-bundle.crt using PEM reader.

OK, thanks. This is good to know. This means there's probably some bug in the sql: path preventing the reading of other root tokens.

> Any idea why the bug appears on i686 only?  

Another indication that it's probably a bug. Usually this means it's probably a latent bug that will accidentally trip on some platform (like an uninitialized stack variable).

bob

Bad track actually. The bug itself is not specific to i686 at all. It always fails when the "sql:" prefix is used. The only difference between 32bit and 64bit is the result of NSS_VersionCheck("3.12.0"). It *sometimes* returns 0, which looks like yet another bug to me.

The good track - curl works again after:
# setup-nsssysinit.sh off

Ah, excellent. This means it's more likely an nsssysinit problem, not a sql db problem.

bob

It looks like a PEM reader bug. I am able to connect when using libnssckbi instead of libnsspem (even with libnssinit loaded):

SECMOD_LoadUserModule("library=libnssckbi.so", NULL, PR_FALSE);

[emaldona@rocinante nss-3.12.5]$ diff mozilla/security/nss/lib/ckfw/builtins/nssckbi.h mozilla/security/nss/lib/ckfw/pem/nssckbi.h 
78,79c78,79
< #define NSS_BUILTINS_LIBRARY_VERSION_MINOR 76
< #define NSS_BUILTINS_LIBRARY_VERSION "1.76"
---
> #define NSS_BUILTINS_LIBRARY_VERSION_MINOR 62
> #define NSS_BUILTINS_LIBRARY_VERSION "1.62"

Why does the pem module need to keep a copy of this header?

Created attachment 377540 [details]
(In reply to comment #7)

For nothing. Is it somehow related to this bug?

Rob, could you please review the attachment #377540 [details] ? Thanks in advance!

Created attachment 377548 [details]
gdb trace

In the failing case, pem_CreateObject() is called with nonsense pTemplate, thus has to fail with CKR_ATTRIBUTE_VALUE_INVALID.

+r on the patch.

I think this may be cruft left over from when the module was being built outside of NSS instead of within the tree.

Now one step closer, I am able to get it working again with the following patch:

--- a/mozilla/security/nss/lib/pk11wrap/pk11slot.c
+++ b/mozilla/security/nss/lib/pk11wrap/pk11slot.c
@@ -1913,6 +1913,8 @@ PK11_GetAllTokens(CK_MECHANISM_TYPE type, PRBool needRW, PRBool loadCerts,
                            PK11_AddSlotToList(loginList, slot);
                        }
                    } else {
+                       if (0==strcmp("PEM", slot->module->commonName))
+                         continue;
                        PK11_AddSlotToList(list, slot);
                    }
                }

The PEM reader just expects the slots to be ordered somehow. However the order does not look to be guaranteed by the surrounding framework.

Hmmm... this code prevents anything from seeing any pem slots.

The list is 'lightly' order (tokens not needing login first, followed by tokens which do).

What part of the PEM reader is failing here?

bob

(In reply to comment #13)
> Hmmm... this code prevents anything from seeing any pem slots.

True, however it does not change anything in the way I debug it. You can of course try the following patch instead:

--- a/mozilla/security/nss/lib/pk11wrap/pk11slot.c
+++ b/mozilla/security/nss/lib/pk11wrap/pk11slot.c
@@ -1906,7 +1906,8 @@ PK11_GetAllTokens(CK_MECHANISM_TYPE type, PRBool needRW, PRBool loadCerts,
                if (needRW &&  slot->readOnly) continue;
                if ((type == CKM_INVALID_MECHANISM)
                                        || PK11_DoesMechanism(slot, type)) {
-                   if (pk11_LoginStillRequired(slot,wincx)) {
+                   if ((0==strcmp("PEM", slot->module->commonName))
+                           || pk11_LoginStillRequired(slot,wincx)) {
                        if (PK11_IsFriendly(slot)) {
                            PK11_AddSlotToList(friendlyList, slot);
                        } else {

> The list is 'lightly' order (tokens not needing login first, followed by tokens
> which do).

The PEM reader obviously expects a bit more ;-) FYI there are no tokens requiring login, so that it does not change the order.

> What part of the PEM reader is failing here?

The first failure I've observed in pem_CreateObject() called unexpectedly as already stated in comment #10.

Created attachment 377629 [details]
magic patch for the PEM reader

It also helps, if the PEM reader does not claim it supports RSA. And believe me or not, I am still able to connect koji.fedoraproject.org with a RSA key protected by password using the attached patch! I am completely puzzled...

Oh, wait. It looks like the problem is in pk11import. We are trying to import a public key in the PEM module. The question is why? For RSA we should be importing into the default RSA device.... which should be softoken.

This explains why all the work arounds succeed. Taking RSA off the reader removes it from the list of eligible tokens which can do RSA. putting it on the list behind softoken makes softoken be selected before pem.

I think there is something about the nsssysinit initialization that is causing softoken not to be placed on the defaults list.

Your info has definately helped pin down this Kamil, I'll take a look at why this isn't happenning.

(On another note, technically we should have libpem support importing session public and private keys, but if we did, we would not have found this other issue.)

bob

Created attachment 377812 [details]
patch revealing incorrect nsssysinit initialization

Good point. The initialization is indeed broken:

#2  SECMOD_LoadModule ("library=\"libnsssysinit.so\" name=\"NSS Internal PKCS #11 Module\" NSS=\"Flags=internal,moduleDBOnly,critical trustOrder=75 cipherOrder=100 slotParams=(1={slotFlags=[RSA,DSA,DH,RC2,RC4,DES,RANDOM,SHA1,MD5,"..., parent=0x6450d0, recurse=1)

You are giving slotParams along with the flag "moduleDBOnly". This is not going to work well. Just look at the function secmod_LoadPKCS11Module(). It simply ignores the slotInfo whenever the "moduleDBOnly" flag is used. The slotParams stuff should go to the "NSS system database" instead, which has that slots actually. Try the attached patch ;-)

This patch should fix it....

--- nsssysinit.c	8 Oct 2009 17:08:36 -0000	1.1
+++ nsssysinit.c	11 Dec 2009 22:14:32 -0000
@@ -203,6 +203,7 @@
 {
     char **module_list = PORT_ZNewArray(char *, 4);
     char *userdb;
+    char *nssflags = "trustOrder=75 cipherOrder=100 slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM askpw=any timeout=30 ] }  ";
     int next = 0;
 
     /* can't get any space */
@@ -217,8 +218,10 @@
 	    "library= "
 	    "module=\"NSS User database\" "
 	    "parameters=\"configdir='sql:%s' %s\" "
-	    "NSS=\"flags=internal%s\"", 
-		userdb, stripped_parameters, getFIPSMode() ? ",FIPS" : "");
+	    "NSS=\"%sflags=internal%s\"", 
+		userdb, stripped_parameters, nssflags,
+		getFIPSMode() ? ",FIPS" : "");
+	nssflags = "";
 
 	/* now open the user's defined PKCS #11 modules */
 	/* skip the local user DB entry */
@@ -235,7 +238,7 @@
 	"library= "
 	"module=\"NSS system database\" "
 	"parameters=\"configdir='sql:%s' tokenDescription='NSS system database' flags=readonly\" "
-	"NSS=\"flags=internal,critical\"",filename);
+	"NSS=\"%sflags=internal,critical\"",filename, nssflags);
 
     /* that was the last module */
     module_list[next] = 0;

> You are giving slotParams along with the flag "moduleDBOnly". This is not going
> to work well. Just look at the function secmod_LoadPKCS11Module(). It simply
> ignores the slotInfo whenever the "moduleDBOnly" flag is used. The slotParams
> stuff should go to the "NSS system database" instead, which has that slots
> actually. Try the attached patch ;-)  

So the problem is the slotParams are NSS flags, which aren't passed on the the 'db module' (the one returning more PKCS #11 strings). nsssysinit then passes it's own truncated version of the NSS flags.

bob

(In reply to comment #18)
Please post non-trivial patches as attachment instead. It's merely impossible to apply them this way.

> This patch should fix it....

It does not in my case. It somehow works only for the system database, not for the user's one.

> +    char *nssflags = "trustOrder=75 cipherOrder=100
> slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM
> askpw=any timeout=30 ] }  ";

What's the purpose of having those flags at two places? Once in pkcs11.txt, which are going to be ignored, and then in nsssysinit.c, hard wired?

> + nssflags = "";

After removing this line it starts to work in that simple case.

> What's the purpose of having those flags at two places? Once in pkcs11.txt,
> which are going to be ignored, and then in nsssysinit.c, hard wired?

The problem is we can't easily "see" what was originally passed from pkcs11.txt.
As far has 'hardwired'. The slotParams pretty much need to be. The others should be under 'administrator control'. nsssysinit is really a placeholder for an administration interface that configures everything on the fly. It's waiting for the system admin interface to be defined.

> > + nssflags = "";

> After removing this line it starts to work in that simple case.  

Ah, of course, you probably don't have any user databases defined. Adding the slotParams on both calls should not cause any problems.

(In reply to comment #21)
> As far has 'hardwired'. The slotParams pretty much need to be. The others
> should be under 'administrator control'. nsssysinit is really a placeholder for
> an administration interface that configures everything on the fly. It's waiting
> for the system admin interface to be defined.

That's finally your choice. I am only solving the bug ;-)

> Ah, of course, you probably don't have any user databases defined. Adding the

How can I define one, so that nsssysinit is able to find it?

> slotParams on both calls should not cause any problems.  

Definitely better than letting curl always fail with the default setup...

[OT] Have you looked at the second part of attachment #377812 [details] ? I marked the place where should go the fix for #540387. If you think it is good (or perhaps not so good) idea, please reply to bug #540387.

Created attachment 378123 [details]
Bob's patch: Fix nsssysinit to set the default flags on the crypto module

> > Ah, of course, you probably don't have any user databases defined. Adding the

> How can I define one, so that nsssysinit is able to find it?

certutil -N -d sql:/etc/pki/nssdb

or even 

certutil -L -w -d sql:/etc/pki/nssdb


Basically nsssysinit will create the directory tree for the new database, so you only need to do something that will open that db read/write instead of read only.

Of course it's good that you didn't have a database because we would not have found this bug.

bob

nss-3.12.5-1.fc12.10 from koji works

Could you please submit that as an update as the bug seems to break fedora-
packager setup for every packager that has updates-testing installed (even if 
the update is being unpushed now because of negative karma - the damage is done).

Thanks.

nss-3.12.5-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/nss-3.12.5-2.fc12

nss-3.12.5-2.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0262

nss-3.12.5-2.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0262

nss-3.12.5-7.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/nss-3.12.5-7.fc12

nss-3.12.5-7.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0685

*** Bug 557859 has been marked as a duplicate of this bug. ***

The bug seems to be back again in nss-3.12.5-7.fc12.

Created attachment 386227 [details]
fix for regression from nss-3.12.5-7.fc12

Patch ready for review ;-)

I was having really hard time to debug it, again:

    if (filename && !userIsRoot() && 0
        /* This doesn't actually work. If we register
           both this and the sysdb (in either order)
           then only one of them actually shows up */) {
        module_list[next++] = PR_smprintf(
              "library= "
              "module=\"NSS database\" "
              "parameters=\"configdir='sql:%s' tokenDescription='NSS database sql:%s'\" "
              "NSS=\"%sflags=internal\"",filename, filename, nssflags);
    }

Do we need the dead code above?  Anyway the coding style above is really awkward!  I wasn't aware of the "&& 0" until I ran it through debugger.  If you really need it there, then please use #if 0.

> Do we need the dead code above? 

My bad, I asked Elio to keep it in in his last patch. We eventually want to get that code working, but it needs to be done upstream.

It's probably better to just remove it from our code (or ifdef it at least).

bob

Let's remove this completely. It's making the code hard to read when we are trying to debug. Of course, once we temporarily drop something then subsequent patches are affected.  We can always add something like this later with
#if (0)
 {
  /* all the code related (including the setting of filename to null)
  inside this block.*/
   ...
  }
#endif 

Here is a commented list of all the patches and how we could about disabling them and bringing them back in as we make progress.

533125-ammend - must keep it, this is ssl related, needed to allow clients to
renegotiate with old servers. May go away or be replaced when nss 3.12.6 comes 
out. It doesn't affect nsssysinit testing at all.

540387 - to prevent and infinite recursion caused by user misunderstanding
of the pkcs11 module configuration instrctions. This is safe but
need as the 545779 fix was verified while this oen was in. 

545779 - Must keep it and get it to worl, this is the first part what we want to get working so we don't break curl.

546221 -This is intended as a work-around so we can simultaneously open application-specific databases and the system one for applications that weren't
shared db aware much less nss db aware. It's not currently working, the code
is commented out. Bob will like to get it to work eventually but it maybe getting in the way. If not execution-wise at least visually. As Bob sugested we should drop it for the time being.

547860 - This is to fix the bug where root couldn't add certs to the system db.
Postpone applying until we get curl working again (545779). To get in back in
change it to work just after 545779 (it's currently applied after 546221 which we want to drop)

553638 - This was originally intended to fix a null pointer exception cased by 546221. It also has some changes Kamil asked for to better document our code for contributors. This can be addressed by a separate bug or even incorporated into 545779 once we get it working and have retested.

Thanks for summary!

As for the dead code, I am fine with #if 0.

> #if (0)

The above looks wrong to me.  The parenthesis should *not* be there since '#if 0' is a common pattern recognized by vast majority of editors.  If you enable syntax-highlighting, such a block of code is marked as comment.

> Here is a commented list of all the patches and how we could about disabling
> them and bringing them back in as we make progress.

What's actually the state of getting them upstream?

(In reply to comment #33)
> Created an attachment (id=386227) [details]
> fix for regression from nss-3.12.5-7.fc12
> Patch ready for review ;-)

Good catch Kamil. I r+'ed and ran these tests:

1. curl -svo/dev/null https://bugzilla.redhat.com which passes.

2. sudo certutil -E -d sql:/etc/pki/nssdb -i newca.cert -t "C,C,C" -n newca
does add the ca cert to the system db.

2. certutil -E -d sql:/etc/pki/nssdb -i newca.cert -t "C,C,C" -n newca
which fails. It doesn't add it to ny user db either which is supposed to do.

It seems that with this change 545779 if fixed again, and 547860 continues to work. At least we aren't regressing but there is more to do.
If anyone needs to do some testing the scratch build I used is at
http://koji.fedoraproject.org/koji/taskinfo?taskID=1939559

(In reply to comment #38) 
certutil -E -d sql:/etc/pki/nssdb -i $HOME/newca.crt -n newca -t "CT,c,c"
After this command the new cert should have been added tu the user's database at $HOME/.pki/nssdb and it doesn't happen. 

The odd thing is that I have seen the test succeed once or twice. The problem is that when I try to repeat it after properly cleaning everything I don't get to work again.

We do have a fix for the regression and I have implemented the proper #ifdef'ing or David's patch so that the code is readable and his disabled code is all in one place. It's actually a revision to the 553638 patch but I'll submit here.

Created attachment 386687 [details]
Fixes regression due to typo and cleans up code

Once reviewed I'll copy this patch to the 533638 bug where it properly belongs. It fixes the regression, cleans up code placing all of David's disabled code in one location with an obvious #ifdef 0.

Comment on attachment 386687 [details]
Fixes regression due to typo and cleans up code

Let me just note the indentation is mangled again, mixed tabs with spaces, etc.

nss-3.12.5-8.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/nss-3.12.5-8.fc12

nss-3.12.5-8.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-1127

nss-3.12.5-8.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.