273624 - NSS_Initialize diagnostics aren't sufficiently precise; regression from 3.3.x to 3.9.x

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Chris Newman]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021104 Chimera/0.6
Build Identifier: NSS 3.9.4

Now that the database filenames are opaque, it is important for NSS_Initialize
with the NSS_INIT_READONLY flag to clearly distingush an error because the
certificate databases haven't been created from other errors.  In NSS 3.3.x the
SEC_ERROR_BAD_DATABASE return code did approximately this.  However, the new
architecture in NSS 3.9.x throws away this detailed error code generated by the
softoken and replaces it with a generic SEC_ERROR_IO error which is not appropriate.

This is a serious problem because reporting a bogus error will generate customer
calls for service which are expensive.  But if I change the caller to avoid
logging SEC_ERROR_IO errors by default, that will make other substantive errors
very difficult to diagnose.

Reproducible: Always
Steps to Reproduce:
1. Call NSS_Initialize with NSS_INIT_READONLY flag when no certificate DBs are
present.
2.
3.

Actual Results:  
returns generic SEC_ERROR_IO error

Expected Results:  
Return SEC_ERROR_BAD_DATABASE as it used to.

Comment 1

[Chris Newman]

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

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The expected error code is set inside softoken, but then when softoken returns
from NSC_Initialize, another error code gets set.  Here are two stack traces,
showing the two events.  Note, I have obscured the actual directory names
for the cert DB in these stacks.

-
=>[1] PR_SetError(code = -8174, osErr = 0), line 54 in "prerror.c"
  [2] PORT_SetError(value = -8174), line 178 in "secport.c"
  [3] nsslowcert_OpenPermCertDB(handle = 0x41658, readOnly = 1, appName =
(nil), prefix = 0x4d7e8 "", namecb = 0xfd6898c8 =
&`libsoftokn3.so`dbinit.c`pk11_certdb_name_cb(void *arg, int dbVersion), cbarg
= 0x4a9c0), line 4168 in "pcertdb.c"
  [4] nsslowcert_OpenCertDB(handle = 0x41658, readOnly = 1, appName = (nil),
prefix = 0x4d7e8 "", namecb = 0xfd6898c8 =
&`libsoftokn3.so`dbinit.c`pk11_certdb_name_cb(void *arg, int dbVersion), cbarg
= 0x4a9c0, openVolatile = 0), line 4630 in "pcertdb.c"
  [5] pk11_OpenCertDB(configdir = 0x46e98 "/xxxxxxxxxxxxx", prefix =
0x4d7e8 "", readOnly = 1, certdbPtr = 0x157170), line 171 in "dbinit.c"
  [6] pk11_DBInit(configdir = 0x46e98 "/xxxxxxxxxxxxxx", certPrefix =
0x4d7e8 "", keyPrefix = 0x4d7a8 "", readOnly = 1, noCertDB = 0, noKeyDB = 0,
forceOpen = 0, certdbPtr = 0x157170, keydbPtr = 0x157174), line 239 in
"dbinit.c"
  [7] PK11_SlotInit(configdir = 0x46e98 "/xxxxxxxxxxxxxxx", params =
0x14760c, moduleIndex = 0), line 2556 in "pkcs11.c"
  [8] nsc_CommonInitialize(pReserved = 0xffbed710, isFIPS = 0), line 2816 in
"pkcs11.c"
  [9] NSC_Initialize(pReserved = 0xffbed710), line 2835 in "pkcs11.c"
  [10] secmod_ModuleInit(mod = 0x147940), line 108 in "pk11load.c"
  [11] SECMOD_LoadPKCS11Module(mod = 0x147940), line 263 in "pk11load.c"
  [12] SECMOD_LoadModule(modulespec = 0x1476c0 "library= name="NSS Internal
PKCS #11 Module" parameters="configdir='/xxxxxxxxxxxxxxx' certPrefix=''
keyPrefix='' secmod='secmod.db' flags=readOnly
cryptoTokenDescription='Internal (Software) Database     '
dbTokenDescription='Internal (Software) Token        ' minPS=0"
NSS="Flags=internal,critical trustOrder=75 cipherOrder=100
slotParams=(1={slotFlags=[RSA,DSA,DH,RC2,RC4,DES,RANDOM,SHA1,MD5,MD2,SSL,TLS,AES,SHA256,SHA512]
askpw=any timeout=30})"", parent = 0x147368, recurse = 1), line 322 in
"pk11pars.c"
  [13] SECMOD_LoadModule(modulespec = 0x1336e8 "name="NSS Internal Module"
parameters="configdir='/xxxxxxxxxxxxxxxxxxxx' certPrefix='' keyPrefix=''
secmod='secmod.db' flags=readOnly  cryptoTokenDescription='Internal (Software)
Database     ' dbTokenDescription='Internal (Software) Token        ' minPS=0"
NSS="flags=internal,moduleDB,moduleDBOnly,critical"", parent = (nil), recurse =
1), line 337 in "pk11pars.c"
  [14] nss_Init(configdir = 0xffbee038 "/xxxxxxxxxxxxxxxxxxx", certPrefix =
0x4d8a8 "", keyPrefix = 0x4d8a8 "", secmodName = 0xff324f1c "secmod.db",
readOnly = 1, noCertDB = 0, noModDB = 0, forceOpen = 0, noRootInit = 0,
optimizeSpace = 0), line 456 in "nssinit.c"
  [15] NSS_Initialize(configdir = 0xffbee038 "/xxxxxxxxxxxxxxxx",
certPrefix = 0x4d8a8 "", keyPrefix = 0x4d8a8 "", secmodName = 0xff324f1c
"secmod.db", flags = 1U), line 531 in "nssinit.c"

Second error code being set

=>[1] PR_SetError(code = -8192, osErr = 0), line 54 in "prerror.c"
  [2] PORT_SetError(value = -8192), line 178 in "secport.c"
  [3] secmod_ModuleInit(mod = 0x147940), line 117 in "pk11load.c"
  [4] SECMOD_LoadPKCS11Module(mod = 0x147940), line 263 in "pk11load.c"
  [5] SECMOD_LoadModule(modulespec = 0x1476c0 "library= name="NSS Internal PKCS
#11 Module" parameters="configdir='/xxxxxxxxxxxxxxxxxxx' certPrefix=''
keyPrefix='' secmod='secmod.db' flags=readOnly
cryptoTokenDescription='Internal (Software) Database     '
dbTokenDescription='Internal (Software) Token        ' minPS=0"
NSS="Flags=internal,critical trustOrder=75 cipherOrder=100
slotParams=(1={slotFlags=[RSA,DSA,DH,RC2,RC4,DES,RANDOM,SHA1,MD5,MD2,SSL,TLS,AES,SHA256,SHA512]
askpw=any timeout=30})"", parent = 0x147368, recurse = 1), line 322 in
"pk11pars.c"
  [6] SECMOD_LoadModule(modulespec = 0x1336e8 "name="NSS Internal Module"
parameters="configdir='/xxxxxxxxxxxxxxxxxxxx' certPrefix='' keyPrefix=''
secmod='secmod.db' flags=readOnly  cryptoTokenDescription='Internal (Software)
Database     ' dbTokenDescription='Internal (Software) Token        ' minPS=0"
NSS="flags=internal,moduleDB,moduleDBOnly,critical"", parent = (nil), recurse =
1), line 337 in "pk11pars.c"
  [7] nss_Init(configdir = 0xffbee038 "/xxxxxxxxxxxxxxxxxxxxx", certPrefix =
0x4d8a8 "", keyPrefix = 0x4d8a8 "", secmodName = 0xff324f1c "secmod.db",
readOnly = 1, noCertDB = 0, noModDB = 0, forceOpen = 0, noRootInit = 0,
optimizeSpace = 0), line 456 in "nssinit.c"
  [8] NSS_Initialize(configdir = 0xffbee038 "/xxxxxxxxxxxxxxxxx", certPrefix
= 0x4d8a8 "", keyPrefix = 0x4d8a8 "", secmodName = 0xff324f1c "secmod.db",
flags = 1U), line 531 in "nssinit.c"

Comment 3

[Wan-Teh Chang]

This is an interesting problem.

The best solution I can come up with is to
call PORT_SetError(0) before the C_Initialize
call in secmod_ModuleInit, and call
PORT_SetError(PK11_MapError(crv)) only if
PORT_GetError() is still 0.  The assumption
is that if the PKCS #11 module's C_Initialize
function sets the PORT (NSPR) error code, we
trust that that error code is more accurate
than trying to map the 'crv'.

What do you think of this solution?  The only
other idea I have is to add a new CKR macro
that will be mapped to SEC_ERROR_BAD_DATABASE,
but I don't know whether we can add new CKR
macros that make sense to the softoken only.

Comment 4

[Julien Pierre]

I don't think adding proprietary CKR return codes would be an option, because
the NSS softoken is also getting used with non-NSS applications, such as JDK
1.5, and they wouldn't understand proprietary error codes.

Comment 5

[Wan-Teh Chang]

Looking at the code closer, I found that the softoken
returns a 'crv' of CKR_CERTDB_FAILED in this case, but
that macro is defined in lib/softoken/dbinit.c like this:

  /* for now... we need to define vendor specific codes here.
   */
  #define CKR_CERTDB_FAILED       CKR_DEVICE_ERROR
  #define CKR_KEYDB_FAILED        CKR_DEVICE_ERROR

So there seems to be an unfinished attempt to do the
right thing...

Comment 6

[Wan-Teh Chang]

Bob said we can define CKR_CERTDB_FAILED and CKR_KEYDB_FAILED
in lib/softoken/pkcs11n.h.  JDK needs to be able to handle
gracefully a CRK in the vendor-defined range (CKR_VENDOR_DEFINED
or higher) returned by the softoken.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I htink that our softoken should return standard PKCS11 error codes (CRVs) 
to the public PKCS11 functions.  

Here's a variation on Wan-Teh's idea.

Call PORT_SetError with zero before calling C_*, and then when C_* returns,
look at the CRV value returned by the C_* function, and IF it returns a 
non-zero CRV error code, and IF that error code is one we know is pretty
meaningless (e.g. one that maps to SEC_ERROR_IO), then don't call 
PORT_SetError again with the mapped value, and just use whatever error code 
was left over by the C_* function.  

Comment 8

[Wan-Teh Chang]

Attachment: Bob's proposed patch (checked in)

I implemented the solution that Bob proposed.
I define two new vendor-defined CKR's for the
softoken: CKR_NETSCAPE_CERTDB_FAILED and
CKR_NETSCAPE_KEYDB_FAILED.

I also have to prevent NSS from calling C_Initialize
again on the softoken with a NULL pInitArgs (which
will fail with CKR_ARGUMENTS_BAD).  Otherwise I will
lose the 'crv' returned by the first C_Initialize call.
We do this by specifically testing for the two
CKR_NETSCAPE_XXX values.

Comment 9

[Wan-Teh Chang]

Comment on attachment 168361 [details] [diff] [review]
Bob's proposed patch (checked in)

Using 'dbtest', I verified that this patch makes
the softoken's C_Initialize return the right CKR
value if either cert8.db or key3.db is missing,
and NSS_Initialize fails with the SEC_ERROR_BAD_DATABASE
error.

I will attempt to implement Nelson's suggestion
next.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I'm inclined to give this patch r+.  It appears to correctly implement two new
vendor-defined PKCS11 CKR_ error codes, to be returned only from C_Initialize.
For programs that initializez NSS's softoken via NSS_Init (or any of NSS's
other initialization functions), this should work just dandy.  
But I still have one concern.

What about programs that use NSS solely for its softoken, that do not use 
any higher-layer aspects of NSS?  IINM, recent versions of JDK now have the 
ability to use any PKCS11 module directly, without needing (say) JSS.  
How will programs using that interface to Softoken behave if/when C_Initialize
returns a vendor-defined error code?  

Glen, can you shed any light on that question?

Do we know of any other programs that use softoken directly?

Since NSS's softoken requires an extrga initializaiton string that (AFAIK)
no other PKCS11 module requires, it is possible that programs that work 
directly with NSS's softoken already have code that treats NSS's softken as
a special case, at least for C_Initialize.  So, maybe it wouldn't be hard to
sell the idea of special casing the error codes, too.  

If we don't break programs horribly by this change, then I'll sr+ it.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

target 3.10

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 168361 [details] [diff] [review]
Bob's proposed patch (checked in)

Basec on feedback from Andreas Sterbenz of Sun, (Thanks!, Andreas), I'm giving
this bug r+, and passing the sr reuqest on to Bob Relyea.  I think one of us
should request that one or more new official CRV values get created for the
next PKCS11 revision, defining errors that are specifically relevant to
software modules, such as a CRV that means "required host file cannot be
opened", and then migrate softoken to use those new official CRVs when they are
available.

Comment 13

[Robert Relyea]

Comment on attachment 168361 [details] [diff] [review]
Bob's proposed patch (checked in)

sr=relyea

Comment 14

[Robert Relyea]

Question on the proposal.

It seams reasonable to me to add a 'required host file could not be openned' CRV
in PKCS #11. My only question is do we want to loose the distinction between the
failure to open the cert db and the failure to open the key db in going to the
common error code? (I think that it's possible that we would)

bob

Comment 15

[Wan-Teh Chang]

Comment on attachment 168361 [details] [diff] [review]
Bob's proposed patch (checked in)

Thanks for the code review, Nelson and Bob.
I checked in this patch on the NSS trunk for NSS 3.10.

Checking in lib/pk11wrap/pk11err.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11err.c,v	<--  pk11err.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/pk11wrap/pk11load.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11load.c,v  <--  pk11load.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/softoken/dbinit.c;
/cvsroot/mozilla/security/nss/lib/softoken/dbinit.c,v  <--  dbinit.c
new revision: 1.24; previous revision: 1.23
done
Checking in lib/softoken/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11n.h,v	<--  pkcs11n.h
new revision: 1.11; previous revision: 1.10
done

Per my observation in comment 8 that CKR_ARGUMENTS_BAD
is mapped to SEC_ERROR_IO, should PK11_MapError map it
to SEC_ERROR_INVALID_ARGS instead?  Note: this change
may have unforseen consequences if some apps depend on
specific error codes to be returned.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Chris Newman, Could you please answer Wan-Teh's question in comment 15 ?
Does your software depend on the error code SEC_ERROR_IO?  Thanks.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

We've heard nothing form teh bug reporter for 2 months on this.
I think we can mark it fixed, and am doing so.

Comment 18

[Chris Newman]

Sorry I didn't respond sooner -- my bugzilla mail goes to a folder I don't read
often enough.  The code I work with does not depend on SEC_ERROR_IO at all so
the suggested mapping sounds like a good idea to me.