620908 - certutil -T -d "sql:." dumps core

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[u238590]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.9.2.12) Gecko/20101027 Firefox/3.6.12
Build Identifier: 3.12.8

certutil dumps core.

Reproducible: Always

Steps to Reproduce:
1.$/share/builds/components/security/SECURITY_3.12.8_20100916/SunOS5.10_DBG.OBJ/bin/certutil -N -d "sql:."
Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.

Enter new password:
Re-enter password:
(do not enter any password)

2. $ls
cert9.db    key4.db     pkcs11.txt

3. $/share/builds/components/security/SECURITY_3.12.8_20100916/SunOS5.10_DBG.OBJ/bin/certutil -T -d "sql:."
Illegal Instruction (core dumped)
Actual Results:  
core dump

$mdb core
::stack
0xa4428(a0390, 2, 1, 1, ff002a00, 0)
libsoftokn3.so`sftkdb_ResetKeyDB+0x30(a0390, 0, 24058, 0, ff3f42f0, 0)
libsoftokn3.so`NSC_InitToken+0x2d0(2, 55f10, 0, ffbfecec, ff0000, 80808080)
libnss3.so`PK11_ResetToken+0x110(a0f08, 55f10, ff23e1a8, ff23e1ac, ff23e1b0, 0)
certutil_main+0x26b8(4, ffbfef44, 1, 4, ff002a00, fedb645c)
main+0x1c(4, ffbfef44, ffbfef58, 53c00, ff000100, 0)
_start+0x108(0, 0, 0, 0, 0, 0)

Expected Results:  
certutil should work.

Comment 1

[u238590]

The root cause of the problem is sdb_Reset was not set in the code :

$dbx certutil
...
(dbx) stop in sftkdb_ResetDB
(dbx) run -T -d "sql:."
Running: certutil -T -d sql:.
(process id 26063)
...
t@1 (l@1) signal ILL (illegal opcode) in (unknown) at 0xa4410
0x000a4410:     illtrap  0xa5660
Current function is sftkdb_ResetDB
 1527       crv = (*db->sdb_Reset)(db);
(dbx) p db
db = 0x9fed8
(dbx) p *db
*db = {
    private               = 0x900c0
    version               = 216
    sdb_type              = SDB_SQL
    sdb_flags             = 12
    app_private           = 0xa0378
    sdb_FindObjectsInit   = 0xfeb64400 = &sdb_FindObjectsInit()
    sdb_FindObjects       = 0xfeb64840 = &sdb_FindObjects()
    sdb_FindObjectsFinal  = 0xfeb649a8 = &sdb_FindObjectsFinal()
    sdb_GetAttributeValue = 0xfeb64f78 = &sdb_GetAttributeValue()
    sdb_SetAttributeValue = 0xfeb64ff8 = &sdb_SetAttributeValue()
    sdb_CreateObject      = 0xfeb656f8 = &sdb_CreateObject()
    sdb_DestroyObject     = 0xfeb65c80 = &sdb_DestroyObject()
    sdb_GetMetaData       = 0xfeb664a8 = &sdb_GetMetaData()
    sdb_PutMetaData       = 0xfeb66858 = &sdb_PutMetaData()
    sdb_Begin             = 0xfeb65ed0 = &sdb_Begin()
    sdb_Commit            = 0xfeb663c8 = &sdb_Commit()
    sdb_Abort             = 0xfeb66438 = &sdb_Abort()
    sdb_Reset             = 0xa4410
    sdb_Close             = 0xfeb66d68 = &sdb_Close()
    sdb_SetForkState      = 0xfeb66f20 = &sdb_SetForkState()
}
(dbx) l 1514
 1514   static CK_RV
 1515   sftkdb_ResetDB(SFTKDBHandle *handle)
 1516   {
 1517       CK_RV crv = CKR_OK;
 1518       SDB *db;
 1519       if (handle == NULL) {
 1520           return CKR_TOKEN_WRITE_PROTECTED;
 1521       }
 1522       db = SFTK_GET_SDB(handle);
 1523       crv = (*db->sdb_Begin)(db);
 1524       if (crv != CKR_OK) {
 1525           goto loser;
 1526       }
 1527       crv = (*db->sdb_Reset)(db);

file mozilla/security/nss/lib/softoken/sftkdb.c

Note sdb_reset points to a vague location and is not a proper function pointer.


Looking at the code sdb_Reset was not set :

1896     sdb = (SDB *) malloc(sizeof(SDB));
1897     sdb_p = (SDBPrivate *) malloc(sizeof(SDBPrivate));
1898
1899     /* invariant fields */
1900     sdb_p->sqlDBName = PORT_Strdup(dbname);
1901     sdb_p->type = type;
1902     sdb_p->table = table;
1903     sdb_p->cacheTable = cacheTable;
1904     sdb_p->lastUpdateTime = now;
1905     /* set the cache delay time. This is how long we will wait be     fore we
1906      * decide the existing cache is stale. Currently set to 10 se     c */
1907     sdb_p->updateInterval = PR_SecondsToInterval(10);
1908     sdb_p->dbMon = PR_NewMonitor();
1909     /* these fields are protected by the lock */
1910     sdb_p->sqlXactDB = NULL;
1911     sdb_p->sqlXactThread = NULL;
1912     sdb->private = sdb_p;
1913     sdb->sdb_type = SDB_SQL;
1914     sdb->sdb_flags = flags | SDB_HAS_META;
1915     sdb->sdb_FindObjectsInit = sdb_FindObjectsInit;
1916     sdb->sdb_FindObjects = sdb_FindObjects;
1917     sdb->sdb_FindObjectsFinal = sdb_FindObjectsFinal;
1918     sdb->sdb_GetAttributeValue = sdb_GetAttributeValue;
1919     sdb->sdb_SetAttributeValue = sdb_SetAttributeValue;
1920     sdb->sdb_CreateObject = sdb_CreateObject;
1921     sdb->sdb_DestroyObject = sdb_DestroyObject;
1922     sdb->sdb_GetMetaData = sdb_GetMetaData;
1923     sdb->sdb_PutMetaData = sdb_PutMetaData;
1924     sdb->sdb_Begin = sdb_Begin;
1925     sdb->sdb_Commit = sdb_Commit;
1926     sdb->sdb_Abort = sdb_Abort;
1927     sdb->sdb_Close = sdb_Close;
1928     sdb->sdb_SetForkState = sdb_SetForkState;
1929
1930     if (inTransaction) {
1931     sqlerr = sqlite3_exec(sqlDB, COMMIT_CMD, NULL, 0, NULL);
1932     if (sqlerr != SQLITE_OK) {
1933         error = sdb_mapSQLError(sdb_p->type, sqlerr);
1934         goto loser;
1935     }
1936     inTransaction = 0;
1937     }

file "security/nss/lib/softoken/sdb.c"

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

confirmed on Windows NT.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Meena, 
Thanks for finding this!  Please submit a patch to fix it and request review 
from me or rrelyea.  I suggest you add your name and email address to the 
file's list of contributors in that patch.  Fame and glory await!  :-)

Comment 4

[u238590]

Attachment: adding the missing function pointer for reset function

Adding the missing function pointer for reset function in file mozilla/security/nss/lib/softtoken/sdb.c

This patch was generated as shown below : 

$export CVSROOT=:pserver:[e-mail]:/cvsroot
$cvs co -r NSPR_4_8_6_RTM NSPR
$cvs co -r NSS_3_12_8_RTM NSS
$cd mozilla/
$gpatch -p 0 < patch.txt
$cd security/nss
$gmake nss_build_all

I have tested "certutil -T" manually. 

$cd mozilla/security/nss/tests/
$HOST=<myhostname> DOMSUF=india.sun.com ./all.sh

SUMMARY:
========
NSS variables:
--------------
HOST=<myhostname>
DOMSUF=india.sun.com
BUILD_OPT=
USE_64=
NSS_CYCLES=""
NSS_TESTS=""
NSS_SSL_TESTS="crl bypass_normal normal_bypass fips_normal normal_fips iopr"
NSS_SSL_RUN="cov auth stress"
NSS_AIA_PATH=
NSS_AIA_HTTP=
NSS_AIA_OCSP=
IOPR_HOSTADDR_LIST=
PKITS_DATA=

Tests summary:
--------------
Passed:             3695
Failed:             7
Failed with core:   0
Unknown status:     0

three failures are : 
36544 TIMESTAMP ssl BEGIN: Thu Dec 23 14:51:15 IST 2010
36545 ssl.sh: SSL tests ===============================
36546 ssl.sh: CRL SSL Client Tests   ===============================
36547 ssl.sh: TLS Request don't require client auth (client does not provide auth) --      --
36548 selfserv starting at Thu Dec 23 14:51:20 IST 2010
36549 selfserv -D -p 8443 -d ../server -n <myhostname>.india.sun.com  \
36550           -w nss -r -i ../tests_pid.17571  &
36551 selfserv: PR_Bind returned error -5982:
36552 Local Network address is in use.
36553 trying to connect to selfserv at Thu Dec 23 14:51:20 IST 2010
36554 tstclnt -p 8443 -h <myhostname>.india.sun.com  -q \
36555         -d ../client -v < /export1/certutil-bug/mozilla/security/nss/tests/ssl/      sslreq.dat
36556 tstclnt: connecting to <myhostname>.india.sun.com:8443 (address=xxx.xxx.xx.xxx)
36557 kill -0 27283 >/dev/null 2>/dev/null
36558 ssl.sh: Exit: 10 Fatal - selfserv process not detectable - FAILED
36559 ssl.sh: #1549: 10 Fatal - selfserv process not detectable - FAILED
36560 ./init.sh: line 178: kill: (27283) - No such process
36561 TIMESTAMP ssl END: Thu Dec 23 14:51:22 IST 2010


46548 chains.sh: Verifying certificate(s)  PayPalEE.cert with flags  -d AllDB    -o O      ID.2.16.840.1.113733.1.7.23.6
46549 vfychain -d AllDB -pp -vv     -o OID.2.16.840.1.113733.1.7.23.6  /export1/certu      til-bug/mozilla/security/nss/tests/libpkix/certs/PayPalEE.cert
46550 Chain is bad, -8181 = Peer's Certificate has expired.
46551 PROBLEM WITH THE CERT CHAIN:
46552 CERT 0. PayPalEE :
46553   ERROR -8181: Peer's Certificate has expired.
46554 Returned value is 1, expected result is pass
46555 chains.sh: #2146: RealCerts: Verifying certificate(s)  PayPalEE.cert with flags        -d AllDB    -o OID.2.16.840.1.113733.1.7.23.6  - FAILED


55534 ssl.sh: CRL SSL Client Tests   ===============================
55535 ssl.sh: TLS Request don't require client auth (client does not provide auth) --      --
55536 selfserv starting at Thu Dec 23 14:59:34 IST 2010
55537 selfserv -D -p 8443 -d ../server -n <myhostname>.india.sun.com  \
55538           -w nss -r -i ../tests_pid.17571  &
55539 selfserv: PR_Bind returned error -5982:
55540 Local Network address is in use.
55541 trying to connect to selfserv at Thu Dec 23 14:59:34 IST 2010
55542 tstclnt -p 8443 -h <myhostname>.india.sun.com  -q \
55543         -d ../client -v < /export1/certutil-bug/mozilla/security/nss/tests/ssl/      sslreq.dat
55544 tstclnt: connecting to <myhostname>.india.sun.com:8443 (address=xxx.xxx.xxxx.xxx)
55545 kill -0 21531 >/dev/null 2>/dev/null
55546 ssl.sh: Exit: 10 Fatal - selfserv process not detectable - FAILED
55547 ssl.sh: #2529: 10 Fatal - selfserv process not detectable - FAILED
55548 ./init.sh: line 178: kill: (21531) - No such process
55549 TIMESTAMP ssl END: Thu Dec 23 14:59:34 IST 2010

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 499488 [details] [diff] [review]
adding the missing function pointer for reset function

r+=nelson for the trunk for the 3.13 release.
It will need a second review (IINM) to be included in a 3.12.x release, 
so I'm requesting that from Bob.

Comment 6

[Wan-Teh Chang]

Comment on attachment 499488 [details] [diff] [review]
adding the missing function pointer for reset function

r=wtc.

I verified that all the other function pointers are set.  I found
that the 'version' field of struct SDBStr (defined in sdb.h) is
unused.  Please remove it.  I also recommend initializing
sdb->app_private to NULL (in both sdb_init and lg_init) as
defensive programming.

MXR shows that sdb_Reset is unreachable code without this patch.
So sdb_Reset has never been used before.  Bob should take the
opportunity to review sdb_Reset...

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, Can this now be checked into the 3.12 branch?
(I don't know the present state of the tree for trunk and 3.12 branch with 
respect to openness and what changes to softoken code (such as this) are 
being accepted on the branch, so please advise.)

Comment 8

[Wan-Teh Chang]

It's OK to make softoken changes on the 3.12 branch, but
the 3.12 branch may be locked down for the 3.12.9 release.
I don't know what the current status is.

The trunk is open right now.

Comment 9

[u238590]

Attachment: made changes as suggested by WTC

Changes include : 
Removed 'version' field of struct SDBStr (defined in sdb.h)  
Initialized sdb->app_private to NULL (in both sdb_init and lg_init) 
Added missing pointer to rest function.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 501691 [details] [diff] [review]
made changes as suggested by WTC

I disagree with WTC's request to remove the version field.  
Binary compatibility is an issue if we remove it.
IMO, we should USE it.

Comment 11

[Wan-Teh Chang]

Attachment: Patch v3, by Meena Vyas

Nelson is right.  We need to keep the version field
and should use it.  I simply initialize it to the
dummy value 0 for now.  I also list Meena Vyas as a
contributor in sdb.c, as patch v1 does.

Patch checked in on the NSS trunk (NSS 3.13) and
NSS_3_12_BRANCH (NSS 3.12.9).

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.20; previous revision: 1.19
done
Checking in legacydb/lginit.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lginit.c,v  <--  lginit.c
new revision: 1.16; previous revision: 1.15
done

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.16.6.2; previous revision: 1.16.6.1
done
Checking in legacydb/lginit.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lginit.c,v  <--  lginit.c
new revision: 1.14.22.1; previous revision: 1.14
done