136279 - NSS3.4 rc2, can not see CA certs on Builtin Object Token

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[miodrag]

With NSS3.4 rc2 I can not list "Builtin Object Token" CA certs. This is the same 
code as used with NSS3.3.x. The problem seems to be with the call to 

PK11_GetAllTokens(CKM_RSA_PKCS, PR_FALSE, PR_FALSE, NULL);

It does not return the "Builtin Object Token".

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bob.

Julien, is this a duplicate of bug 116315?

Comment 2

[Julien Pierre]

Wan-Teh,

No, it is not the same bug.
We don't use PK11_GetAllTokens in the 6.x web server admin code . We use 
PK11_ListCerts. This was changed for a number of reasons when web server moved 
from NSS 2.8 to 3.2.1 and the root cert module was installed. The main ones were 
problems with trust and duplicate cert entries.

Comment 3

[Ian McGreer]

Is libnssckbi.so in LD_LIBRARY_PATH, or installed in secmod.db?

Comment 4

[miodrag]

It's installed along with secmod.db in the same directory for both Unix and NT.  
But the same problem exists on all platforms.

You can stop by at my cube and we can single-step through the NSS code. That 
will probably be the easiest way to diagnose the problem.

Comment 5

[Julien Pierre]

Miodrag,

Can you do a modutil -list and show the output ?
I'd like to see s libnssckbi.so loaded from an absolute pathname or relative 
pathname.

Comment 6

[miodrag]

Here is the output on Solaris8

1) Check that libnssckbi.so is in the same directory as secmod.db

>> ls admin-serv/config/*.??
admin-serv/config/libnssckbi.so  admin-serv/config/secmod.db

2) List modules
>> shared/bin/modutil -dbdir admin-serv/config -nocertdb -list
Using database directory admin-serv/config...

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services Version 3.4                
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services                  
        token: NSS Certificate DB
-----------------------------------------------------------

Comment 7

[Julien Pierre]

Miodrag,

What version of modutil were you using ?

This output shows that the root cert module is not installed inside secmod.db. 
So it isn't surprising that PK11_GetAllTokens isn't listing it.

The output should look like this if the module was in secmod.db .

(strange)/export/home/jpierre/60/SunOS_5.8_depend/ns/server/work/B1/SunOS5.8_DBG
.OBJ/alias{165} modutil -dbdir . -nocertdb -list
Using database directory ....

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services Version 3.4                
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services                  
        token: NSS Certificate DB

  2. Root Certs
        library name: 
/export/home/jpierre/60/SunOS_5.8_depend/ns/server/work/B1/SunOS5.8_DBG.OBJ/alia
s/libnssckbi.so
         slots: 1 slot attached
        status: loaded

         slot: 
        token: Builtin Object Token
-----------------------------------------------------------
(strange)/export/home/jpierre/60/SunOS_5.8_depend/ns/server/work/B1/SunOS5.8_DBG
.OBJ/alias{166}

Try doing a modutil -add to see if that works.
There is some code to automatically load the root cert module to secmod.db . It 
will only work if the db is open read/write, however. Admin server may not do 
that. The web server admin server does, and everything is fine.

Comment 8

[miodrag]

I was using the NSS_3_4_RTM from the integration area.
But if I modutil 3.3.2 on the same directory (secmod.db created with NSS3.4) I 
get the expected output:

Listing of PKCS #11 Modules
-----------------------------------------------------------
  1. NSS Internal PKCS #11 Module
         slots: 2 slots attached
        status: loaded

         slot: NSS Internal Cryptographic Services Version 3.2
        token: NSS Generic Crypto Services

         slot: NSS User Private Key and Certificate Services
        token: NSS Certificate DB

  2. Root Certs
        library name: ./libnssckbi.so
         slots: 1 slot attached
        status: loaded

         slot: 
        token: Builtin Object Token


Which module should I try to add using -add option? Can you give me the exact 
command parameters. 

Comment 9

[Julien Pierre]

The problem was with the set of parameters passed to NSS_Initialize.
The configdir is supposed to be the path where secmod.db and libnssckbi.so 
both live.
But instead, configdir was two levels above that location. secmod.db was found 
because the secmodprefix had a path in it. Basically the code in NSS changed and 
is trying to load configdir/libnssckbi.so instead of 
configdir/path(secmodprefixdir)/libnssckbi.so in 3.3.2 ...
This could be worked around by passing the correct set of paths to 
NSS_Initialize.

Comment 10

[Wan-Teh Chang]

This seems like a semantic change that breaks backward
compatibility with NSS 3.3.2.

Comment 11

[Robert Relyea]

There wasn't an intent to change the semantics (sigh). I think the 3.4 semantics
are more correct, but we should keep this open and allow the old semantics to
work as well.

NOTE: this problem only occurs in applications which depend on the builtins
automatically being loaded, and store secmod.db in another directly other than
the configdir (which is the case with the admin server).

Mioadrag, the work around is to put a link to the libnssckbi.so from the config
directory to the secmod.db directory.

bob

Comment 12

[Julien Pierre]

The link wouldn't work for NT.
The workaround I suggested to Miodrag was to specify the full path to 
secmod.db in configdir, then use relative paths in the keydbprefix and 
certdbprefix, eg ../../alias . This worked when we tried it.

Comment 13

[miodrag]

The workaround works (on NT can copy the DLL to the config dir) but just let me 
know if this bug will be fixed (3.3 behavior restored) for 3.4.1. If the bug 
does not get fixed, then I will need to change NSS_Initialize AS and 
also adminsdk component. 

Comment 14

[Julien Pierre]

I am having difficulty reproducing the problem right now. The problem might be 
related to http://bugzilla.mozilla.org/show_bug.cgi?id=139291 .

Miodrag, could you please paste the exact argument values you are passing to 
NSS_Initialize, as well as a brief summary of your directory structure - eg. 
where your libnssckbi.so, secmod.db, and cert/key DBs live.

Thanks.

Comment 15

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 16

[Julien Pierre]

Attachment: Proposed patch to try both old and new locations for root certs

Comment 17

[Julien Pierre]

Attachment: new patch

Comment 18

[Wan-Teh Chang]

Assigned the bug to Julien.

Comment 19

[Wan-Teh Chang]

Bob, could you please review Julien's new patch?  Thanks.

Comment 20

[Julien Pierre]

Bob, is this patch ok to check in ?

Comment 21

[Robert Relyea]

Yes, the new patch looks good.

bob

Comment 22

[Julien Pierre]

Checked in to 3.4 branch :

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.42.2.2; previous revision: 1.42.2.1
done

And tip :

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.45; previous revision: 1.44
done

Comment 23

[Wan-Teh Chang]

Set target milestone to NSS 3.4.2.

Comment 24

[Wan-Teh Chang]

Miodrag verified the fix works on NT.

Marked the bug verified.

Comment 25

[Wan-Teh Chang]

The 32-bit HP-UX optimized version of certutil (built on the
machine "sbshp1") dumps core with this fix, so I am reopening
this bug.  I will recompile with debug symbols and try to get
a stack trace.

Comment 26

[Wan-Teh Chang]

This is the stack trace in the core file:

Core was generated by `certutil'.
Program terminated with signal 11, Segmentation fault.

warning: The shared libraries were not privately mapped; setting a
breakpoint in a shared library will not work until you rerun the program.

#0  0xc01780d8 in mallinfo () from /usr/lib/libc.2
(gdb) where
#0  0xc01780d8 in mallinfo () from /usr/lib/libc.2
#1  0xc0174ccc in malloc () from /usr/lib/libc.2
#2  0xc015cc7c in calloc () from /usr/lib/libc.2
#3  0xc13605bc in PR_Calloc ()
   from /u/wtc/tmp/nss-3.4.2-beta2/mozilla/dist/HP-
UXB.11.00_OPT.OBJ/lib/libnspr4.sl
#4  0x9b7e8 in PORT_ZAlloc () at secport.c:137
#5  0x9b94c in PORT_NewArena () at secport.c:182
#6  0x36560 in secmod_NewModule () at pk11pars.c:58
#7  0x36678 in SECMOD_CreateModule () at pk11pars.c:114
#8  0x48c20 in SECMOD_AddNewModuleEx () at pk11util.c:429
#9  0x48d50 in SECMOD_AddNewModule () at pk11util.c:476
#10 0x1d258 in nss_FindExternalRoot () at nssinit.c:333
#11 0x1d508 in nss_Init () at nssinit.c:435
#12 0x1d678 in NSS_Initialize () at nssinit.c:481
#13 0x1b208 in main () at certutil.c:2649
(gdb)

I can't print the values of any local variables in gdb.

Comment 27

[Wan-Teh Chang]

Attachment: Patch to null-terminate 'path' and 'oldpath'

The cause for the coredump is that 'path' is not null-terminated,
which screws up the PORT_Strcpy(oldpath, path) call.

I believe that the PORT_Strncpy() call does not store any null
character into 'oldpath' and leaves 'oldpath' without a terminating
null character, which will screw up the PORT_Strcat(oldpath, dllname)
call.  The second part of the patch fixes that.  It also changes
the PORT_Strncpy() call to a PORT_Memcpy() call to make it clear
that it is not storing a null character.

Comment 28

[Wan-Teh Chang]

Comment on attachment 81101 [details] [diff] [review]
new patch

We should also add a comment to explain what 'oldpath' is,
something like:
  'oldpath' is the external root path in NSS 3.3.x or older.
  For backward compatibility we try to load the root certs
  module with the old path first.  (See bug 136279.)

Comment 29

[Wan-Teh Chang]

Attachment: Incremental patch

This patch makes the code handling 'oldpath' more
symmetric to the code handling 'path'.	It also has
a comment explaining what 'oldpath' is for.

Comment 30

[Wan-Teh Chang]

Comment on attachment 82238 [details] [diff] [review]
Incremental patch

In the interest of time, I've checked this patch into
the tip and NSS_3_4_BRANCH of NSS.  I'd still like Bob
or Julien to review this patch.  Thanks.

Comment 31

[Robert Relyea]

Comment on attachment 81101 [details] [diff] [review]
new patch

On closer review, we have bug in the original. We should be calling
PORT_ZAlloc() because we are depending on the buffer to be zero filled for us.
Neither the PORT_Memcpy, nor the addition of FILE_SEP add a zero, but then we
use PORT_Strcpy on path expecting it to be NULL terminated.

Also, if it turns out that oldpath and path are identical, we should only
return path (this is the normal case for most apps).

bob

Comment 32

[Wan-Teh Chang]

Bob, the original code does not have this bug.  The buffer
does not need to be zero filled; it just needs to be NULL
terminated.

You wrote:
> Neither the PORT_Memcpy, nor the addition of FILE_SEP
> add a zero, but then we use PORT_Strcpy on path expecting
> it to be NULL terminated.

Yes, and that's fine because PORT_Strcpy adds the terminating
NULL character to path.

I will attach yet another patch that handles the case where
path and oldpath are identical.

Comment 33

[Robert Relyea]

Your right: the original code is OK, because path is only used as a target
(PORT_Strcyp(&patch[path_len], dllname);

The new code, however, uses path as the source:
> PORT_Strcpy(oldpath,path);
before the subsequent Strcpy can add the NULL. If we get unlucky, we can have
quite a long string with garbage in oldpath (and our tests will never find it).

Comment 34

[Wan-Teh Chang]

Attachment: Incremental patch #2: return oldpath only if it is different from path

Bob, is this what you want?

Comment 35

[Robert Relyea]

Comment on attachment 82292 [details] [diff] [review]
Incremental patch #2: return oldpath only if it is different from path

Yes, It also fixes the other error I was talking about (we are no longer trying
use a non-NULL terminated string in the source of a Strcpy).

Looks good.

bob

Comment 36

[Wan-Teh Chang]

Attachment: Incremental patch #2: return oldpath only if it is different from path

Now that we return oldpath only if it is different from path,
oldpath will be NULL most of the time, so the original code
would be making an unnecessary call to SECMOD_HasRootCerts()
most of the time.  This version of the patch only calls
SECMOD_HasRootCerts() when oldpath is not NULL.

Comment 37

[Wan-Teh Chang]

Comment on attachment 82727 [details] [diff] [review]
Incremental patch #2: return oldpath only if it is different from path

This incremental patch has been checked into both the
NSS_3_4_BRANCH and the tip of NSS.

Comment 38

[Wan-Teh Chang]

Marked the bug fixed.  Would appreciate a code review of
the latest patch.

Comment 39

[Robert Relyea]

Comment on attachment 82727 [details] [diff] [review]
Incremental patch #2: return oldpath only if it is different from path

looks good

Comment 40

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks!