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

RESOLVED FIXED in 3.4.2

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: miodrag, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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".
(Reporter)

Updated

15 years ago
Priority: -- → P1

Comment 1

15 years ago
Assigned the bug to Bob.

Julien, is this a duplicate of bug 116315?
Assignee: wtc → relyea
(Assignee)

Comment 2

15 years ago
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

15 years ago
Is libnssckbi.so in LD_LIBRARY_PATH, or installed in secmod.db?
(Reporter)

Comment 4

15 years ago
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.

(Assignee)

Comment 5

15 years ago
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.

(Reporter)

Comment 6

15 years ago
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
-----------------------------------------------------------
(Assignee)

Comment 7

15 years ago
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.

(Reporter)

Comment 8

15 years ago
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. 
(Assignee)

Comment 9

15 years ago
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

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

Comment 11

15 years ago
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
(Assignee)

Comment 12

15 years ago
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.
(Reporter)

Comment 13

15 years ago
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. 
(Assignee)

Comment 14

15 years ago
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

15 years ago
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
(Assignee)

Comment 16

15 years ago
Created attachment 81080 [details]
Proposed patch to try both old and new locations for root certs
(Assignee)

Comment 17

15 years ago
Created attachment 81101 [details] [diff] [review]
new patch
(Assignee)

Updated

15 years ago
Attachment #81080 - Attachment is obsolete: true

Comment 18

15 years ago
Assigned the bug to Julien.
Assignee: relyea → jpierre

Comment 19

15 years ago
Bob, could you please review Julien's new patch?  Thanks.
(Assignee)

Comment 20

15 years ago
Bob, is this patch ok to check in ?

Updated

15 years ago
Attachment #81101 - Flags: review+

Comment 21

15 years ago
Yes, the new patch looks good.

bob
(Assignee)

Comment 22

15 years ago
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 23

15 years ago
Set target milestone to NSS 3.4.2.
Target Milestone: --- → 3.4.2

Comment 24

15 years ago
Miodrag verified the fix works on NT.

Marked the bug verified.
Status: RESOLVED → VERIFIED

Comment 25

15 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 26

15 years ago
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

15 years ago
Created attachment 82172 [details] [diff] [review]
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

15 years ago
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

15 years ago
Created attachment 82238 [details] [diff] [review]
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.
Attachment #82172 - Attachment is obsolete: true

Comment 30

15 years ago
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

15 years ago
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

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #82238 - Flags: review+

Comment 33

15 years ago
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

15 years ago
Created attachment 82292 [details] [diff] [review]
Incremental patch #2: return oldpath only if it is different from path

Bob, is this what you want?

Comment 35

15 years ago
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
Attachment #82292 - Flags: review+

Comment 36

15 years ago
Created attachment 82727 [details] [diff] [review]
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.
Attachment #82292 - Attachment is obsolete: true

Comment 37

15 years ago
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

15 years ago
Marked the bug fixed.  Would appreciate a code review of
the latest patch.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 39

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

looks good
Attachment #82727 - Flags: review+

Updated

15 years ago
Blocks: 145836

Comment 40

15 years ago
adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks! 
Keywords: adt1.0.1+, mozilla1.0.1, nsbeta1+
Whiteboard: [adt2 RTM]

Updated

15 years ago
Keywords: adt1.0.1+, mozilla1.0.1 → fixed1.0.1
You need to log in before you can comment on or make changes to this bug.