316837 - crash in [@ strchr - nsNSSCertificateDB::getCertNames] on cert without nickname

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

NSS in a firefox trunk build from 20051113.

How:
nsCOMPtr<nsIX509CertDB> certDB (do_GetService (NS_X509CERTDB_CONTRACTID));
certDB->FindCertNicknames (NULL, nsIX509Cert::SERVER_CERT, &cnt, &list);

Trace:
#0  0xb6e57a63 in strchr () from /lib/tls/i686/cmov/libc.so.6
#1  0xb2c19235 in nsNSSCertificateDB::getCertNames (this=0x880e4e0, certList=0x880e598, type=8, _count=0xbf9012b4, _certNames=0xbf9012b0)
    at /opt/source/mozilla/trunk-ff/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:1235
#2  0xb2c193f2 in nsNSSCertificateDB::FindCertNicknames (this=0x880e4e0, aToken=0x0, aType=8, _count=0xbf9012b4, _certNames=0xbf9012b0)
    at /opt/source/mozilla/trunk-ff/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:197

(gdb) frame 1
#1  0xb2c19235 in nsNSSCertificateDB::getCertNames (this=0x880e4e0, certList=0x880e598, type=8, _count=0xbf9012b4, _certNames=0xbf9012b0)
    at /opt/source/mozilla/trunk-ff/mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:1235
1235            char *sc = strchr(namestr, ':');
Current language:  auto; currently c++
(gdb) list
1230          PR_FREEIF(dbkey);
1231          if (type == nsIX509Cert::EMAIL_CERT) {
1232            namestr = node->cert->emailAddr;
1233          } else {
1234            namestr = node->cert->nickname;
1235            char *sc = strchr(namestr, ':');
1236            if (sc) *sc = DELIM;
1237          }
1238          nsAutoString certname = NS_ConvertASCIItoUCS2(namestr);
1239          certstr.Append(PRUnichar(DELIM));
(gdb) p namestr
$1 = 0x0
(gdb) p *node->cert
$2 = {arena = 0x884f690, subjectName = 0x8850740 "CN=certs.synchroedit.com/signing,O=Alacrity Management Corporation,C=US,ST=California,L=Berkeley",
  issuerName = 0x88507a8 "CN=certs.synchroedit.com,O=Alacrity Management Corporation,C=US,ST=California,L=Berkeley", signatureWrap = {data = {
      type = siBuffer, data = 0x88501cc "0\202\002\026\uffff\003\002\001\002\002\004\177\uffffW\uffff0\r\006\t*\206H\206\uffff\r\001\001\004\005", len = 538},
    signatureAlgorithm = {algorithm = {type = siBuffer, data = 0x88503ea "*\206H\206\uffff\r\001\001\004\005", len = 9}, parameters = {type = siBuffer,
        data = 0x88503f3 "\005", len = 2}}, signature = {type = siBuffer,
      data = 0x88503f9 "w\u02f3\212~\uffff%\uffffH+\uffff\203\206\uffff\u0428I\uffff\003\uffff\215]\uffffm\uffff\201G~\022\uffffS8\uffff\uffff\uffff\212\206\206\2255\215\uffff&\uffffk\236hS\uffff\uffffUl\201\216N\uffffj\203\037\uffff\uffff\a\uffff\uffff\uffff\237<o\uffff*^\uffff?.=M\uffff+D\uffffN{>e\t\uffff\223\uffff\uffffQ\005\202\uffff\\XZa\217\uffff\uffffM\uffffN\230?\212vx\uffff\001\uffff\v\uffff\213\031\uffffc\u0537\uffff/\225\022\uffff\uffffh\0343\uffff\uffff\uffff\uffff\uffff\uffff\uffff\230\004\205\b\234\004\205\b\uffff\004\205\b\uffff\004\205\b\uffff\004\205\b",
      len = 1024}}, derCert = {type = siBuffer,
    data = 0x88501c8 "0\202\002\uffff0\202\002\026\uffff\003\002\001\002\002\004\177\uffffW\uffff0\r\006\t*\206H\206\uffff\r\001\001\004\005", len = 689}, derIssuer = {
    type = siBuffer,
    data = 0x88501ea "0\1771\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifornia1\v0\t\006\003U\004\006\023\002US1(0&\006\003U\004\n\023\037Alacrity Management Corporation1\0360\034\006\003U\004\003\023\025certs.synchroedit.com0\036\027\r050826201059Z\027\r131126201059Z0\201\2071\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifo"..., len = 129}, derSubject = {type = siBuffer,
    data = 0x885028b "0\201\2071\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifornia1\v0\t\006\003U\004\006\023\002US1(0&\006\003U\004\n\023\037Alacrity Management Corporation1&0$\006\003U\004\003\023\035certs.synchroedit.com/signing0\201\2370\r\006\t*\206H\206\uffff\r\001\001\001\005",
    len = 138}, derPublicKey = {type = siBuffer, data = 0x8850315 "0\201\2370\r\006\t*\206H\206\uffff\r\001\001\001\005", len = 162}, certKey = {type = siBuffer,
    data = 0x88506a0 "\177\uffffW\uffff0\1771\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifornia1\v0\t\006\003U\004\006\023\002US1(0&\006\003U\004\n\023\037Alacrity Management Corporation1\0360\034\006\003U\004\003\023\025certs.synchroedit.com\uffff\uffff\uffff\uffff\"\f'\2037\uffff\uffff\002x\206n\uffffa\uffff6eD`Y\uffff\uffff\uffff\uffffCN=certs.synchroedit.com/signing,O=Alacr"..., len = 133}, version = {type = siBuffer, data = 0x88501d4 "\002\002\004\177\uffffW\uffff0\r\006\t*\206H\206\uffff\r\001\001\004\005", len = 1},
  serialNumber = {type = siBuffer, data = 0x88501d7 "\177\uffffW\uffff0\r\006\t*\206H\206\uffff\r\001\001\004\005", len = 4}, signature = {algorithm = {type = siBuffer,
      data = 0x88501df "*\206H\206\uffff\r\001\001\004\005", len = 9}, parameters = {type = siBuffer, data = 0x88501e8 "\005", len = 2}}, issuer = {arena = 0x0,
    rdns = 0x8850480}, validity = {arena = 0x0, notBefore = {type = siUTCTime,
      data = 0x885026f "050826201059Z\027\r131126201059Z0\201\2071\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifornia1\v0\t\006\003U\004\006\023\002US1(0&\006\003U\004\n\023\037Alacrity Management Corporation1&0$\006\003U\004\003\023\035certs.synchroedit.com/signing0\201\2370\r\006\t*\206H\206\uffff\r\001\001\001\005", len = 13}, notAfter = {type = siUTCTime,
      data = 0x885027e "131126201059Z0\201\2071\0210\017\006\003U\004\a\023\bBerkeley1\0230\021\006\003U\004\b\023\nCalifornia1\v0\t\006\003U\004\006\023\002US1(0&\006\003U\004\n\023\037Alacrity Management Corporation1&0$\006\003U\004\003\023\035certs.synchroedit.com/signing0\201\2370\r\006\t*\206H\206\uffff\r\001\001\001\005", len = 13}}, subject = {arena = 0x0, rdns = 0x8850550}, subjectPublicKeyInfo = {arena = 0x0, algorithm = {algorithm = {type = siBuffer,
        data = 0x885031c "*\206H\206\uffff\r\001\001\001\005", len = 9}, parameters = {type = siBuffer, data = 0x8850325 "\005", len = 2}}, subjectPublicKey = {
      type = siBuffer, data = 0x885032b "0\201\211\002\201\201", len = 1120}}, issuerID = {type = siBuffer, data = 0x0, len = 0}, subjectID = {
    type = siBuffer, data = 0x0, len = 0}, extensions = 0x8850620, emailAddr = 0x0, dbhandle = 0x87ef310, subjectKeyID = {type = siBuffer,
    data = 0x8850728 "\uffff\"\f'\2037\uffff\uffff\002x\206n\uffffa\uffff6eD`Y\uffff\uffff\uffff\uffffCN=certs.synchroedit.com/signing,O=Alacrity Management Corporation,C=US,ST=California,L=Berkeley",
    len = 20}, keyIDGenerated = 0, keyUsage = 132, rawKeyUsage = 132, keyUsagePresent = 1, nsCertType = 16, keepSession = 0, timeOK = 0, domainOK = 0x0,
  isperm = 1, istemp = 0, nickname = 0x0, dbnickname = 0x0, nssCertificate = 0x8927c80, trust = 0x8850808, referenceCount = 1, subjectList = 0x0,
  authKeyID = 0x0, isRoot = 0, authsocketlist = 0x0, series = 0, slot = 0x87f0c80, pkcs11ID = 4087546326, ownSlot = 1}


The cert that this is crashing on used to be available when starting the demo ofrom the demo on http://www.synchroedit.com/ but I cannot find it there now (it's possible it's available once you create an account there). If there's a way to extract it from the cert8.db file, I could attach it; or I could provide the cert8.db file in private, if necessary.

Comment 1

[Wan-Teh Chang]

Kai, could you take a look at this crash?

Bob, is it legal for a CERTCertificate's "char *nickname"
field to be a NULL pointer?

Comment 2

[Wan-Teh Chang]

Here is the code in question:

1231       if (type == nsIX509Cert::EMAIL_CERT) {
1232         namestr = node->cert->emailAddr;
1233       } else {
1234         namestr = node->cert->nickname;
1235         char *sc = strchr(namestr, ':');
1236         if (sc) *sc = DELIM;
1237       }

In this crash, type is nsIX509Cert::SERVER_CERT (8).

If it is legal for node->cert->nickname to be NULL,
we can fix this crash by using node->cert->subjectName
instead if node->cert->nickname is NULL.

Also, we may want to have a case for nsIX509Cert::SERVER_CERT
to display its DNS name.  We will need to figure out how to
display a server cert with multiple DNS names in its subjectAltName
cert extension.  But an email cert has the same issue.

Comment 3

[Kai Engert (:KaiE:)]

independet of the crash, function nsNSSCertificateDB::getCertNames looks very obscure to me

if i read this code correctly:

      if (type == nsIX509Cert::EMAIL_CERT) {
        namestr = node->cert->emailAddr;
      } else {
        namestr = node->cert->nickname;
        char *sc = strchr(namestr, ':');
        if (sc) *sc = DELIM;
      }
      nsAutoString certname = NS_ConvertASCIItoUTF16(namestr);
      certstr.Append(PRUnichar(DELIM));
      certstr += certname;
      certstr.Append(PRUnichar(DELIM));
      certstr += keystr;
      tmpArray[i++] = ToNewUnicode(certstr);

for email certs will produce a certstr
  delim + email + delim + keystr 

for other certs, for nicknames without a colon ':' it will produce
  delim + nickname + delim + keystr

however, if the nickname does contain a colon it will produce
          
  delim + nickname-before-first-colon + delim + nickname-after-first-colon + delim + keystr


if the caller expects a pair of strings delimited using "delim" for each cert, nicknames containint a colon will disturb that...?

i will try to analyze further

Comment 4

[Kai Engert (:KaiE:)]

It seems our own code never calls exported interface nsIX509CertDB::findCertNicknames

First, we should make this function safe and check for nulls. I will attach an initial patch.

Second, should we skip certs not having a nickname that we can return? Or, should we rather return an empty string with its db-key?

Third, what should we do regarding the triple-delim when nickname contains a colon?

Comment 5

[Kai Engert (:KaiE:)]

Attachment: patch v1

Comment 6

[Robert Relyea]

Comment on attachment 228038 [details] [diff] [review]
patch v1

This is fine. It means certs with NULL nicknames will show up as blank on the list of name.

bob

Comment 7

[Kai Engert (:KaiE:)]

fixed on trunk

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 228038 [details] [diff] [review]
patch v1

proposing 1.8 branch inclusion of this simple null-check crash fix

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 228038 [details] [diff] [review]
patch v1

a=mconnor on behalf of drivers for 1.8 branch

Comment 10

[Kai Engert (:KaiE:)]

fixed1.8.1