487736 - libpkix passes wrong argument to DER_DecodeTimeChoice and crashes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

While working on my CRL cache patch, I ran into the following crashing stack while running vfychain :

(dbx) where
current thread: t@1
  [1] __lwp_kill(0x1, 0x6), at 0xfeb0a067
  [2] _thr_kill(0x1, 0x6), at 0xfeb057c4
  [3] raise(0x6), at 0xfeab1da3
  [4] abort(0xfed5e7b8, 0xfeca36b9, 0x8045d90, 0xfed46e3b, 0xfed4e48c, 0xfed4e490), at 0xfea91bed
  [5] PR_Assert(s = 0xfed4e48c "0", file = 0xfed4e490 "sectime.c", ln = 179), line 562 in "prlog.c"
  [6] DER_DecodeTimeChoice_Util(output = 0x8045df8, input = 0x807f408), line 179 in "sectime.c"
=>[7] PKIX_PL_CRL_VerifyUpdateTime(crl = 0x80b0d70, date = 0x807f408, pResult = 0x8045e74, plContext = 0x807da50), line 725 in "pkix_pl_crl.c"
  [8] pkix_CRLSelector_DefaultMatch(selector = 0x80b0380, crl = 0x80b0d70, pMatch = 0x8045efc, plContext = 0x807da50), line 498 in "pkix_crlselector.c"
  [9] pkix_pl_Pk11CertStore_GetCRL(store = 0x8099fc0, selector = 0x80b0380, pNBIOContext = 0x8045f68, pCrlList = 0x8045f6c, plContext = 0x807da50), line 845 in "pkix_pl_pk11certstore.c"
  [10] pkix_CrlChecker_CheckExternal(cert = 0x809d738, issuer = 0x80a0fb8, date = 0x807f408, checkerObject = 0x809a388, procParams = 0x8099bd0, methodFlags = 1U, pRevStatus = 0x8045fdc, pReasonCode = 0x80aa940, pNBIOContext = 0x8046018, plContext = 0x807da50), line 428 in "pkix_crlchecker.c"
  [11] PKIX_RevocationChecker_Check(cert = 0x809d738, issuer = 0x80a0fb8, revChecker = 0x8093620, procParams = 0x8099bd0, chainVerificationState = 1, testingLeafCert = 1, pRevStatus = 0x8046080, pReasonCode = 0x80aa940, pNbioContext = 0x80460b8, plContext = 0x807da50), line 422 in "pkix_revocationchecker.c"
  [12] pkix_CheckChain(certs = 0x80ac8e8, numCerts = 3U, anchor = 0x80ab118, checkers = 0x80aafd0, revChecker = 0x8093620, removeCheckedExtOIDs = 0x80ad870, procParams = 0x8099bd0, pCertCheckedIndex = 0x80aa92c, pCheckerIndex = 0x80aa930, pRevChecking = 0x80aa950, pReasonCode = 0x80aa940, pNBIOContext = 0x8046180, pFinalSubjPubKey = 0x804618c, pPolicyTree = 0x8046188, pVerifyTree = (nil), plContext = 0x807da50), line 797 in "pkix_validate.c"
  [13] pkix_Build_ValidateEntireChain(state = 0x80aa910, anchor = 0x80ab118, pNBIOContext = 0x804620c, pValResult = 0x8046234, verifyNode = 0x80ad108, plContext = 0x807da50), line 1335 in "pkix_build.c"
  [14] pkix_BuildForwardDepthFirstSearch(pNBIOContext = 0x8046350, state = 0x80aa910, pValResult = 0x8046300, plContext = 0x807da50), line 2522 in "pkix_build.c"
  [15] pkix_Build_InitiateBuildChain(procParams = 0x8099bd0, pNBIOContext = 0x80463fc, pState = 0x8046404, pBuildResult = 0x8046400, pVerifyNode = 0x8046450, plContext = 0x807da50), line 3533 in "pkix_build.c"
  [16] PKIX_BuildChain(procParams = 0x8099bd0, pNBIOContext = 0x8046464, pState = 0x8046460, pBuildResult = 0x8046468, pVerifyNode = 0x8046450, plContext = 0x807da50), line 3706 in "pkix_build.c"
  [17] CERT_PKIXVerifyCert(cert = 0x80989a0, usages = 2LL, paramsIn = 0x807bc10, paramsOut = 0x807bcf0, wincx = 0x807a0d0), line 2161 in "certvfypkix.c"
  [18] main(argc = 12, argv = 0x8046664, envp = 0x8046698), line 721 in "vfychain.c"
(dbx)

(dbx) frame 6
Current function is DER_DecodeTimeChoice_Util
  179               PORT_Assert(0);
(dbx) p input
input = 0x807f408
(dbx) p *input
*input = {
    type = 1213836335
    data = 0x4672a "<bad address 0x4672a>"
    len  = 89U
}
(dbx) up
Current function is PKIX_PL_CRL_VerifyUpdateTime
  725           status = DER_DecodeTimeChoice(&timeToCheck, &(date->nssTime));
(dbx) p date
date = 0x807f408
(dbx) p *date
*date = {
    nssTime = 1239331206966319LL
}
(dbx)

The problem is that the input date is PKIX_PL_Date, whose nssTime field is of type PRTime, not SECItem*. The code in PKIX_PL_CRL_VerifyUpdateTime can't possibly have ever worked.
What's really puzzling is that when I run the same test on a tree that doesn't include my changes (in crl.c), this stack never even runs. The first common function is PKIX_RevocationChecker_Check . Nothing above that runs.
I don't know how my CRL cache changes affect that execution path and I haven't tried to find that out yet. However, this previously untested PKIX code is now being exercised, and needs to be fixed.

Comment 1

[Julien Pierre]

In fact, this is detected as a warning by the Studio compiler. Unfortunately, C lets you get away with murder.

cc -o SunOS5.10_i86pc_DBG.OBJ/pkix_pl_crl.o -c -g -KPIC -DSVR4 -DSYSV -D__svr4 -D__svr4__ -DSOLARIS -D_REENTRANT -Di386 -DSOLARIS2_10 -D_SVID_GETTOD  -xs -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_jp96085 -DNSS_ENABLE_ECC -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -I/usr/dt/include -I/usr/openwin/include -I../../../../../../dist/SunOS5.10_i86pc_DBG.OBJ/include -I../../../../../../dist/public/nss -I../../../../../../dist/private/nss -I../../../../../../dist/public/dbm  pkix_pl_crl.c
"pkix_pl_crl.c", line 725: warning: argument #2 is incompatible with prototype:
        prototype: pointer to const struct SECItemStr {enum  {siBMPString(15), siUTF8String(14), siVisibleString(13), siGeneralizedTime(12), siUTCTime(11), siUnsignedInteger(10), siDEROID(9), siAsciiString(8), siAsciiNameString(7), siEncodedNameBuffer(6), siDERNameBuffer(5), siEncodedCertBuffer(4), siDERCertBuffer(3), siCipherDataBuffer(2), siClearDataBuffer(1), siBuffer(0)} type, pointer to unsigned char data, unsigned int len} : "../../../../../../dist/public/nss/secder.h", line 200
        argument : pointer to long long

Comment 2

[Julien Pierre]

Attachment: Fix crash

Comment 3

[Alexei Volkov]

Comment on attachment 372003 [details] [diff] [review]
Fix crash

Patch is correct. I've missed the place when created a patch for bug 482795. r=alexei

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 372003 [details] [diff] [review]
Fix crash

Alexei gave this patch r+.  That's good enough for me.

Comment 5

[Julien Pierre]

Thanks, Alexei. I checked this patch in.

Checking in pkix_pl_crl.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c,v  <--  pkix_pl_crl.c
new revision: 1.11; previous revision: 1.10
done

Comment 6

[Julien Pierre]

I think I know why I ended up testing this part of the code which was never running. I had a bug in my CRL cache patch that caused DPCache_Lookup to failed when the new emptyCache argument was NULL . And my 1-line change to libpkix was calling it with NULL. This is what must have caused libpkix to take previously untested code paths.