Last Comment Bug 390381 - libpkix rejects cert chain when root CA cert has no basic constraints
: libpkix rejects cert chain when root CA cert has no basic constraints
Status: RESOLVED FIXED
PKIX NSS312B1
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 normal (vote)
: 3.12
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
http://lxr.mozilla.org/security/sourc...
: 422717 (view as bug list)
Depends on:
Blocks: 390233 422717
  Show dependency treegraph
 
Reported: 2007-07-31 15:43 PDT by Alexei Volkov
Modified: 2008-03-19 13:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
cert with netscape cert type extension (1.46 KB, application/octet-stream)
2007-07-31 15:43 PDT, Alexei Volkov
no flags Details
Verify EKU (2.17 KB, patch)
2008-03-03 15:14 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
intermediate CA cert for above server cert (903 bytes, application/octet-stream)
2008-03-04 02:39 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details
Correct computation of nsCertType & recompute it when trust changes, v1 (2.67 KB, patch)
2008-03-04 19:57 PST, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Splinter Review
log correct error info (2.17 KB, patch)
2008-03-13 13:40 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
Correct computation of nsCertType, v2 (6.50 KB, patch)
2008-03-13 21:54 PDT, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: review+
rrelyea: superreview+
Details | Diff | Splinter Review

Description Alexei Volkov 2007-07-31 15:43:33 PDT
Created attachment 274690 [details]
cert with netscape cert type extension

While testing libpkix verify cert wrapper with live ssl sites, I've run into the failure with the symptom that the library fails to match eku. The cert has 
Netscape Cert Type and nss successfully validates it. But the cert does not have a supported extended key usage.
 
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            5b:ca:d5:7e:5e:04:9d:79:6b:04:ad:8d:3b:6c:12:f1
        Signature Algorithm: PKCS #1 MD5 With RSA Encryption
        Issuer: "OU=www.verisign.com/CPS Incorp.by Ref. LIABILITY LTD.(c)97 V
            eriSign,OU=VeriSign International Server CA - Class 3,OU="VeriSig
            n, Inc.",O=VeriSign Trust Network"
        Validity:
            Not Before: Tue Jan 09 00:00:00 2007
            Not After : Sat Feb 02 23:59:59 2008
        Subject: "CN=www.fastnet.com.ph,OU="Member, VeriSign Trust Network",O
            U="Authenticated by mySecureSign, Inc.",OU=Terms of use at www.my
            securesign.com/rpa (c) 01,OU=Technical Support Department,O=Equit
            able PCI Bank,L=Makati City,ST=Metro Manila,C=PH"
        Subject Public Key Info:
            Public Key Algorithm: PKCS #1 RSA Encryption
            RSA Public Key:
                Modulus:
                    e0:f4:86:65:8e:39:7a:7b:5f:65:3a:57:9b:95:b0:ff:
                    e9:01:7e:e2:93:82:48:b5:5a:8e:39:a5:d6:9d:9b:09:
                    ff:c2:34:44:b7:3c:d3:e8:a3:69:ce:9f:07:b8:65:73:
                    b4:c4:ae:07:48:9b:cf:84:d6:17:a4:0e:86:dc:f0:4b:
                    2d:6f:bc:55:4e:19:d6:c7:27:f0:4c:19:8e:04:40:bb:
                    97:e1:83:27:9e:6a:b6:29:3e:52:f7:ac:06:2f:7f:0d:
                    20:0c:2b:f0:63:9e:db:59:9c:e8:c0:62:60:a9:92:62:
                    86:72:60:84:28:b1:f3:93:0c:34:1c:99:c1:a2:9f:87
                Exponent: 65537 (0x10001)
        Signed Extensions:
            Name: Certificate Basic Constraints
            Data: Is not a CA.

            Name: Certificate Policies (Obsolete)
            Data: Sequence {
                Sequence {
                    Sequence {
                        Verisign User Notices
                        Sequence {
                            "This certificate incorporates by reference, and 
                            its use is strictly subject to, the VeriSign Cert
                            ification Practice Statement (CPS), available at:
                             https://www.verisign.com/CPS; by E-mail at CPS-r
                            equests@verisign.com; or by mail at VeriSign, Inc
                            ., 2593 Coast Ave., Mountain View, CA 94043 USA T
                            el. +1 (415) 961-8830 Copyright (c) 1996 VeriSign
                            , Inc.  All Rights Reserved. CERTAIN WARRANTIES D
                            ISCLAIMED and LIABILITY LIMITED."
                            [0]: {
                                OID.2.16.840.1.113733.1.7.1.1.1
                            }
                            [1]: {
                                OID.2.16.840.1.113733.1.7.1.1.2
                            }
                            Sequence {
                                Sequence {
                                    "https://www.verisign.com/repository/CPS "
                                }
                            }
                        }
                    }
                }
            }

            Name: Certificate Type
            Data: <SSL Server>

            Name: Extended Key Usage
                Strong Crypto Export Approved
                Microsoft SGC SSL server

    Signature Algorithm: PKCS #1 MD5 With RSA Encryption
    Signature:
        63:ce:bc:47:a4:0a:a9:67:2e:c2:0a:34:78:a4:a4:8e:
        85:88:71:83:ca:98:97:b7:c1:5a:f4:f3:be:11:e0:2a:
        56:7f:55:10:3d:2f:cb:d7:fe:38:8c:89:b0:56:2b:8c:
        5d:85:e1:36:fd:5e:7e:7f:9c:78:08:56:9c:27:66:da:
        21:02:08:db:93:a0:8b:86:a9:4b:5c:70:eb:80:35:36:
        34:ef:3f:b2:5d:90:76:06:b8:b6:12:0e:57:c5:4d:5e:
        80:8a:43:e2:a5:85:b8:6e:87:ea:e6:4b:9b:08:30:f0:
        c6:b7:b8:5e:44:6a:57:5f:5f:c0:5e:7b:4e:40:5f:c0
    Fingerprint (MD5):
        6B:0C:BC:86:F7:0C:F1:0D:EA:18:12:38:42:58:3C:EB
    Fingerprint (SHA1):
        1B:FC:60:25:0F:02:B0:06:F5:92:10:1F:48:37:F1:49:FF:D0:71:69

    Certificate Trust Flags:
        SSL Flags:
        Email Flags:
        Object Signing Flags:
Comment 1 Nelson Bolyard (seldom reads bugmail) 2007-07-31 22:23:27 PDT
Alexei, Notice that the server cert has EKU for SSL "Step-Up"
and for Microsoft's SGC.  Both of those EKUs imply SSL server.
Any server capable of SSL step-up or SGC is also an ordinary 
SSL server.  NSS supports Step-Up instead of SGC, but neither 
of those matter any more.  NSS can talk ordinary SSL to either 
type of server (Step-Up or SGC) just fine.

libPKIX should recognize either of those EKU OIDs as implying 
SSL server EKU.  The older cert code does that.  I don't think 
it's very important that libPKIX support the old Netscape cert 
type (although, I wouldn't object if it did), but I think it is
far more important that NSS should recognize those two other 
widely used EKU OIDs for SSL service.
Comment 2 Alexei Volkov 2007-08-07 13:53:29 PDT
So we should add those two eku to the libPKIX array of known ekus(See file pkix_pl_ekuchecker.c). 
Comment 3 Nelson Bolyard (seldom reads bugmail) 2007-08-07 16:32:08 PDT
http://lxr.mozilla.org/security/source/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_ekuchecker.c#46
One has to wonder where that list came from.  I just LOVE ivory tower software.  
It's sad that we have YA list of known OIDs.
IMO, we should not have to convert them from ASCII dotted decimal form to 
DER form every time we want to check them.

NSS has historically recognized these EKU OIDs:
SEC_OID_NS_KEY_USAGE_GOVT_APPROVED      (ssl step up)
SEC_OID_EXT_KEY_USAGE_SERVER_AUTH
SEC_OID_EXT_KEY_USAGE_CLIENT_AUTH
SEC_OID_EXT_KEY_USAGE_CODE_SIGN
SEC_OID_EXT_KEY_USAGE_EMAIL_PROTECT
SEC_OID_EXT_KEY_USAGE_TIME_STAMP
SEC_OID_OCSP_RESPONDER

We should also take note of at least one of Microsoft's EKU OIDs,
namely mSGC, seen in 
http://lxr.mozilla.org/security/source/security/nss/cmd/lib/moreoids.c#61
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-08-08 14:44:31 PDT
Besides being absurdly inefficient, there is a fundamental flaw in the 
design of the EKU checker and the associated PKIX_ComCertSelParams object.

The design assumes that there is exactly one specific OID that is required
and that the caller always knows exactly which OID that is.  

But that is clearly a flawed assumption, as shown by this bug.  The caller
wishes to require an OID that specifies SSL server usage.  But there is 
more than one such OID.  The caller does not wish to require any particular
one of them, but rather to require ANY one of them.  

The existing design simply cannot represent the notion of "any one of the 
various SSL server EKUs" in the PKIX_ComCertSelParams, and the existing 
methods cannot set or return such a value.  
Comment 5 Nelson Bolyard (seldom reads bugmail) 2008-02-29 20:10:38 PST
EKU processing is completely broken in cert_VerifyCertChainPkix.

cert_VerifyCertChainPkix calls two functions to setup the PKIX_ProcessingParams
and those two functions don't do it right.  They are (in order called):
- cert_CreatePkixProcessingParams   and
- cert_ProcessingParamsSetKuAndEku

The first of those functions creates the comCertSelParams, and then is 
supposed to call PKIX_PL_EkuChecker_Create but that call is commented out.  
I fixed that, but it's still not right.

PKIX_PL_EkuChecker_Create calls pkix_pl_EkuChecker_Create which assumes that
the "certSelector" already contains the required EKU inside the comCertSelParams.  pkix_pl_EkuChecker_Create calls pkix_pl_EkuChecker_GetRequiredEku which calls PKIX_ComCertSelParams_GetExtendedKeyUsage to get the required EKU.
But the required EKU has not yet been set in the comCertSelParams.
So, the EkuChecker is created to check for NO EKU.

Then after cert_CreatePkixProcessingParams returns, cert_ProcessingParamsSetKuAndEku gets the comCertSelParams, and sets the
required EKU in them.  But it's too late.  The comCertSelParams has already
been checked for a required EKU, and no code ever checks the comCertSelParams 
for a required EKU again thereafter.  

I have a patch that fixes that problem.  With that patch in place, the 
original problem that I described in comment 3 is now manifest.  
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-03-03 14:35:10 PST
Alexei, Please clarify something.  
Is this bug about use of libPKIX through the old CERT_VerifyCert* APIs?
or about the new CERT_PKIXVerifyCert function?
or both? 
or?

I found that the behavior of CERT_PKIXVerifyCert (as described in comment 5 
above) is very different than the behavior of CERT_VerifyCertificate when 
used with libPKIX via the environment variable.  

Maybe this bug should be about the old CERT_VerifyCert* APIs, and another
bug should be opened for comment 5 above.  Do you agree?
Comment 7 Alexei Volkov 2008-03-03 15:14:04 PST
Created attachment 307111 [details] [diff] [review]
Verify EKU

Possible fix. This is not probably the right bug to attach the patch.

When I wrote the code, I had an impression that cert EKU verification was a part pkix_CertSelector_DefaultMatch functionality which actually do verification of EKU IF cert selector has specified eku list. But this is not the case. pkix_BuildForwardDepthFirstSearch function creates special sub copy of the parameters that was originally defined by user. Only validation date, subject name, and basic constraints are taken into consideration when cert get selected.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-03-03 15:26:00 PST
Comment on attachment 307111 [details] [diff] [review]
Verify EKU

Amazing.  This is the EXACT same patch that I devised over the weekend!
Great minds run in the same channel.
r=nelson
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-03-04 02:39:29 PST
Created attachment 307205 [details]
intermediate CA cert for above server cert

With the above server cert in a file named cert.000
and this intermediate CA cert in file named cert.001
and with your secmod.db file including nssckbi, 
the command 
>  vfychain -b 0801010101Z -u 1 -v cert.000 cert.001
which tests the above chain using the old cert library, reports
> Chain is good!

The command 
> vfychain -b 0801010101Z -u 1 -v -p cert.000 cert.001
which tests the above chain using the new CERT_PKIXVerifyCert function
reports:
> Chain is bad, -8164 = This certificate is not valid.
which is wrong and not a helpful error

The command
> NSS_ENABLE_PKIX_VERIFY=1 vfychain -b 0801010101Z -u 1 -v cert.000 cert.001
which runs the test using the old CERT_VerifyCert* API, but uses libPKIX,
returns the following result:
> Chain is bad, -8101 = Certificate type not approved for application.
That result is wrong, but the error code correctly explains what problem 
libPKIX found (yay for good error messages!).  I believe THIS result is
the subject of this bug, but perhaps the result shown above with error 
-8164 is also within the scope of this bug.

Finally, the command
> NSS_ENABLE_PKIX_VERIFY=1 vfyserv www.microsoft.com
returns the result
> Connecting to host www.microsoft.com (addr 207.46.192.254) on port 443
> Error in function PR_Write: -8101
> - Certificate type not approved for application.
which I believe is the same result as shown for the preceding vfychain
command.  In other words, I think microsoft's cert has the same issue
as the expired www.fastnet.com.ph cert attached above.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2008-03-04 02:43:12 PST
Upon further investigation, I see that the microsoft cert has the same
symptom, but the cert is very different.  The EKU for that cert appears
to be OK.  So, I suspect that is a different problem. :(
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-03-04 04:02:06 PST
I have found a problem here that seems to be the cause of this failure.

It appears to be an unintended consequence of Julien's patch to fix 
bug 201134.  That patch changed function cert_GetCertType to only 
compute the value once, and if called again, to return the same
value it computed the first time.  

The problem is that that function makes part of its computation based
on trust information for the cert.  But that function gets called from
CERT_DecodeDERCertificate *BEFORE* the trust information for the cert
has been found.  The trust pointer is always NULL when called from 
CERT_DecodeDERCertificate.  Consequently, cert_GetCertType behaves as 
if the cert has no trust information, and computes a cert type value
that ignores the trust flags.

Here is a stack trace of the first call to that function.

nss3.dll!cert_GetCertType()                   Line 690	
nss3.dll!CERT_DecodeDERCertificate()          Line 935 + 0x9 bytes	
nss3.dll!nssDecodedPKIXCertificate_Create()   Line 482 + 0xd bytes	
nss3.dll!stan_GetCERTCertificate()            Line 825 + 0xe bytes	
nss3.dll!STAN_GetCERTCertificateOrRelease()   Line 894 + 0xb bytes	
nss3.dll!CERT_CreateSubjectCertList()         Line 719 + 0xb bytes	
nss3.dll!pkix_pl_Pk11CertStore_CertQuery()    Line 242 + 0x1b bytes	
nss3.dll!pkix_pl_Pk11CertStore_GetCert()      Line 534 + 0x11 bytes	
nss3.dll!pkix_Build_GatherCerts()             Line 2022 + 0x1d bytes	
nss3.dll!pkix_BuildForwardDepthFirstSearch()  Line 2574 + 0x1e bytes	
nss3.dll!pkix_Build_InitiateBuildChain()      Line 4142 + 0x18 bytes	
nss3.dll!PKIX_BuildChain()                    Line 4332 + 0x1d bytes	
nss3.dll!cert_BuildAndValidateChain()         Line 784 + 0x1d bytes	
nss3.dll!cert_VerifyCertChainPkix()           Line 1194 + 0x15 bytes	
nss3.dll!cert_VerifyCertChain()               Line 870 + 0x29 bytes	
nss3.dll!CERT_VerifyCertificate()             Line 1288 + 0x2d bytes	
vfychain.exe!main()                           Line 357 + 0x27 bytes	

The function gets called a second time, later in this call stack:

nss3.dll!cert_GetCertType()                    Line 572
nss3.dll!pkix_pl_Cert_CheckExtendedKeyUsage()  Line 781 + 0xb bytes
nss3.dll!pkix_pl_EkuChecker_Check()            Line 350 + 0x17 bytes
nss3.dll!pkix_Build_VerifyCertificate()        Line 1145 + 0x15 bytes
nss3.dll!pkix_BuildForwardDepthFirstSearch()   Line 2638 + 0x30 bytes
nss3.dll!pkix_Build_InitiateBuildChain()       Line 4142 + 0x18 bytes
nss3.dll!PKIX_BuildChain()                     Line 4332 + 0x1d bytes
nss3.dll!cert_BuildAndValidateChain()          Line 784 + 0x1d bytes
nss3.dll!cert_VerifyCertChainPkix()            Line 1194 + 0x15 bytes
nss3.dll!cert_VerifyCertChain()                Line 870 + 0x29 bytes
nss3.dll!CERT_VerifyCertificate()              Line 1288 + 0x2d bytes
vfychain.exe!main()                            Line 357 + 0x27 bytes

This time, the trust information is present, but since the value of 
cert->nsCertType has already been computed once, it does not get 
recomputed here.  Consequently, the cert type information (bits) that 
would come from the trust flags is never computed, and the cert type 
ends up showing that the cert is NOT a CA cert, even though the trust
flags say that it is.  

If (for testing purposes) I modify cert_GetCertType to recompute the 
value each time it is called, this bug can no longer be reproduced.
I do not mean to suggest that that is the right fix.  But clearly,
we need to ensure that the trust information HAS been used in the 
computation of the cert type information before we rely on that cert
type information.  
Comment 12 Nelson Bolyard (seldom reads bugmail) 2008-03-04 04:10:10 PST
The cert in question is a Verisign root CA cert with the subject name:
OU=Class 3 Public Primary Certification Authority,O="VeriSign, Inc.",C=US
Serial number: (hex) 70 ba e4 1d 10 d9 29 34 b6 38 ca 7b 03 cc ba bf

This cert is an X.509 v1 cert with no extensions, and hence no basic 
constraints extension.  There are trust flags for this cert that say it is 
a valid and trusted CA cert.  Because there is no trust information present 
in the CERTCertificate the first time it is called, cert_GetCertType 
concludes that the cert is NOT a CA cert.  Consequently, this cert fails the
required cert type test in pkix_pl_Cert_CheckExtendedKeyUsage.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2008-03-04 04:40:58 PST
Note that if I provide the root CA cert on the command line, so that
vfychain tries to import it, then the first time that this root cert 
is encountered by cert_GetCertType, the stack is:

nss3.dll!cert_GetCertType()                  Line 690
nss3.dll!CERT_DecodeDERCertificate()         Line 935 + 0x9 bytes
nss3.dll!nssDecodedPKIXCertificate_Create()  Line 482 + 0xd bytes
nss3.dll!stan_GetCERTCertificate()           Line 825 + 0xe bytes
nss3.dll!STAN_GetCERTCertificateOrRelease()  Line 894 + 0xb bytes
nss3.dll!__CERT_NewTempCertificate()         Line 395 + 0x9 bytes
vfychain.exe!getCert()                       Line 244 + 0x13 bytes
vfychain.exe!main()                          Line 326 + 0x10 bytes

Again, the trust information is missing.  The result is exactly the 
same as before.

In the case of the chain for the www.microsoft.com cert, the problem 
is the same, but with a different root certificate.  This time, the 
root CA cert has the name: 
  CN=GTE CyberTrust Global Root,
  OU="GTE CyberTrust Solutions, Inc.",
  O=GTE Corporation,
  C=US
Serial number: (hex) 01 a5

But otherwise the problem is the same.

Note that we have still not yet encountered the case that I described in 
comment 3, where the EKU type mismatches, because it's expecing the 
TLS server EKU but gets the SSL Step-Up EKU instead.  I still think that's
a problem, but we haven't yet encountered it.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2008-03-04 05:07:50 PST
More findings.

When libPKIX encounters an expired cert, the error code that gets reported
is -8164 = This certificate is not valid.  That's a bug in our error mapping 
that needs to be fixed.

When vfychain is called with the -b and -p arguments, it does NOT pass the 
date from the -b argument in to CERT_PKIXVerifyCert.  So, when vfychain is
used with the -p argument, it always tests with the current time, regardless
of whether the -b argument is present or not.  That's a bug in vfychain,
and maybe also in CERT_PKIXVerifyCert.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2008-03-04 05:30:30 PST
When I run this test:
> NSS_ENABLE_PKIX_VERIFY=1 vfychain -u 1 -v cert.000 cert.001
which uses libPKIX with the old cert API and does not specify a date,
I get these resuts:

Chain is bad, -8164 = This certificate is not valid.
PROBLEM WITH THE CERT CHAIN:
CERT 0. CN=www.fastnet.com.ph,OU="Member, VeriSign Trust Network",OU="Authentica
ted by mySecureSign, Inc.",OU=Terms of use at www.mysecuresign.com/rpa (c) 01,OU
=Technical Support Department,O=Equitable PCI Bank,L=Makati City,ST=Metro Manila
,C=PH :
  ERROR -8181: Peer's Certificate has expired.
  ERROR -8181: Peer's Certificate has expired.

This tells me that, at some point, the right error code is generated, but
then, at some other point, it gets turned into the wrong error.  Also, the
error -8101 should only appear once in that log.  That error can only occur
once for any given cert, but it is reported twice for this cert.
Comment 16 Nelson Bolyard (seldom reads bugmail) 2008-03-04 19:57:51 PST
Created attachment 307388 [details] [diff] [review]
Correct computation of nsCertType & recompute it when trust changes, v1

This patch, together with the "Verify EKU" patch above (which I have already
given r+) seem to solve this problem.  

There may be a better way to force the nsCertType to be re-evaluated when 
the trust is obtained.  Maybe we even want to drop the first call to that
function that is in CERT_DecodeDERCert, and just evaluated it in this new 
place.  But this seems to work.
Comment 17 Nelson Bolyard (seldom reads bugmail) 2008-03-04 20:06:15 PST
Comment on attachment 307388 [details] [diff] [review]
Correct computation of nsCertType & recompute it when trust changes, v1

Julien, note that the code to force the nsCertType to be re-evaluated is 
inside of function fill_CERTCertificateFields.  Please take careful note
of the comment at the beginning of that function.  That may ease any 
concerns you have about the safety of this. After this function returns,
the caller releases its lock on the stan certificate object.
Comment 18 Julien Pierre 2008-03-04 20:38:24 PST
Nelson,

Could we fix this by making libpkix uses CERT_NewTempCertificate instead of CERT_DecodeDERCertificate ? Would the nscerttype value be correct the first time if this was the case ?

Your patch looks like it would work. It is probably the right thing, because trust can change over time, and thus we would need to recompute nsCertType occasionally. The field really doesn't belong in the public structure :(

If we choose to recompute, do we know that the new trust is always different when we refill the cert structure ? It might be worth comparing it with the value of the old trust, since the recomputation does not look cheap.
Comment 19 Nelson Bolyard (seldom reads bugmail) 2008-03-04 21:42:26 PST
(In reply to comment #18)

> Could we fix this by making libpkix uses CERT_NewTempCertificate instead of
> CERT_DecodeDERCertificate ? Would the nscerttype value be correct the first
> time if this was the case ?

No, Apparently not.  Look at the stack in comment 13.  

> Your patch looks like it would work. It is probably the right thing, because
> trust can change over time, and thus we would need to recompute nsCertType
> occasionally. 

The only change with which I am concerned at the moment is the change from
no-trust (because the code just hasn't gotten around to getting the trust 
yet) to having trust.  

Note that there is another path through the code for a similar scenario.
It is for the case where a "perm cert" (token object) is given trust AFTER 
it has been initially processed.  My patch doesn't address that case.  
More on that case below.

In tracing through all this code, I found NUMEROUS places where the code
checks a value that has not yet been set, then later sets it, but does not
check it again.  These were NOT in libPKIX or related code.  They were in 
good old certdb.  I find this quite discouraging. :(

> If we choose to recompute, do we know that the new trust is always different
> when we refill the cert structure ? 

In the code path I patched, we know only that it is transitioning from 
no trust to some trust.

> It might be worth comparing it with the
> value of the old trust, since the recomputation does not look cheap.

There's no way to compare the old and new trust without computing the new
trust.  (Is there?)

In function stan_GetCERTCertificate(), you will see this block of code:

852     if (!cc->nssCertificate || forceUpdate) {
853         fill_CERTCertificateFields(c, cc, forceUpdate);
854     } else if (!cc->trust && !c->object.cryptoContext) {
855         /* if it's a perm cert, it might have been stored before the
856          * trust, so look for the trust again.  But a temp cert can be
857          * ignored.
858          */
859         CERTCertTrust* trust = NULL;
860         trust = nssTrust_GetCERTCertTrustForCert(c, cc);
861         cc->trust = trust;
862     }

The patch I made affects the first path there, the one that calls 
fill_CERTCertificateFields, but there is a second case, where trust is 
added to the cert (by the application) after the CERTCertificate and 
NSSCertificate were initialized.  My patch does not cause the nsCertType
to be recomputed in that case.  

Since this is the function that holds the object lock, I thought about 
putting my patch in this function, instead of in fill_CERTCertificateFields.
I thought about something like this:

   +    PRBool updated = PR_FALSE;

852     if (!cc->nssCertificate || forceUpdate) {
853         fill_CERTCertificateFields(c, cc, forceUpdate);
   +        updated = PR_TRUE;
854     } else if (!cc->trust && !c->object.cryptoContext) {
855         /* if it's a perm cert, it might have been stored before the
856          * trust, so look for the trust again.  But a temp cert can be
857          * ignored.
858          */
859         CERTCertTrust* trust = NULL;
860         trust = nssTrust_GetCERTCertTrustForCert(c, cc);
861         cc->trust = trust;
   +        updated = PR_TRUE;
862     }
   +    if (updated) {
   +	    /* force the cert type to be recomputed to include trust info */
   +	    cc->nsCertType = 0;
   +	    cert_GetCertType(cc);
   +    }

I could easily be persuaded that that is a superior fix, but I observe
that it would call cert_GetCertType every time that fill_CERTCertificateFields
was called, whereas the present patch does not do that.  

Another option is to keep the existing patch, but also add this:

854     } else if (!cc->trust && !c->object.cryptoContext) {
855         /* if it's a perm cert, it might have been stored before the
856          * trust, so look for the trust again.  But a temp cert can be
857          * ignored.
858          */
859         CERTCertTrust* trust = NULL;
860         trust = nssTrust_GetCERTCertTrustForCert(c, cc);
861         cc->trust = trust;
   +	    /* force the cert type to be recomputed to include trust info */
   +	    cc->nsCertType = 0;
   +	    cert_GetCertType(cc);
862     }

I invite analysis on which of these is the best alternative (if any :).
Comment 20 Alexei Volkov 2008-03-11 16:24:29 PDT
attachment 307111 [details] [diff] [review] is integrated.
Comment 21 Nelson Bolyard (seldom reads bugmail) 2008-03-11 20:22:31 PDT
Doesn't attachment 307111 [details] [diff] [review] depend on attachment 307388 [details] [diff] [review]?
Doesn't committing 307111 without committing 307388 cause test failures?

Unfortunately, I now think that patch 307388 is wrong, and may reintroduce 
a race that was previously fixed in this code.  That's why I haven't committed
that patch.  Work on that is stalled while I debug and fix the windows CVS 
client. 
Comment 22 Slavomir Katuscak 2008-03-12 03:08:12 PDT
Alexei, since your patch was integrated, there are some failures in Tinderboxes in PKIX test cycles:

fips.sh: Validate the certificate --------------------------
certutil -d ../fips -V -n FIPS_PUB_140_Test_Certificate -u SR -e -f ../tests.fipspw.23427
1[80947e0]: Error at level 0: Unable to build chain
1[80947e0]: Error at level 1: Unable to build chain
1[80947e0]: Error at level 2: Unable to build chain
1[80947e0]: Error at level 3: Unable to build chain
1[80947e0]: Error at level 4: Unable to build chain
1[80947e0]: Error at level 5: Unable to build chain
1[80947e0]: Class Error leaked 1 objects of size 20 bytes, total = 20 bytes
certutil: certificate is invalid: Certificate type not approved for application.
fips.sh: #601: Validate the certificate (certutil -V -e) . - FAILED

There are some more failures introduced at +- same time (bug 391296), but this one looks different and only this one is PKIX specific. Please check if this is caused by the patch from this bug.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2008-03-12 11:19:53 PDT
Alexei, 
Please interpret the interesting diagnostic output quoted in comment 22 for us.
Is that entire set of 8 lines of error text just a single error report?
The leak report seems out of place.  Seems like it should be the last thing
output by the program, long after certutil reported the cert was invalid. no?
Maybe this is an issue with output going to two streams, stderr and stdout ?

Alexei or Slavo:
If you repeat that test with patch 307388 applied, does that cure this problem?  
If so, then the right thing to do is to accelerate work on the replacement
for that patch.
Comment 24 Alexei Volkov 2008-03-13 12:23:40 PDT
> fips.sh: Validate the certificate --------------------------
> certutil -d ../fips -V -n FIPS_PUB_140_Test_Certificate -u SR -e -f
> ../tests.fipspw.23427
> 1[80947e0]: Error at level 0: Unable to build chain
> 1[80947e0]: Error at level 1: Unable to build chain
> 1[80947e0]: Error at level 2: Unable to build chain
> 1[80947e0]: Error at level 3: Unable to build chain
> 1[80947e0]: Error at level 4: Unable to build chain
> 1[80947e0]: Error at level 5: Unable to build chain
> 1[80947e0]: Class Error leaked 1 objects of size 20 bytes, total = 20 bytes
> certutil: certificate is invalid: Certificate type not approved for
> application.
> fips.sh: #601: Validate the certificate (certutil -V -e) . - FAILED
> 
The problem reported by libpkix was:
*** CERTVFYPKIX Error- Unable to build chain
*** Cause (2): *** BUILD Error- pkix_Build_InitiateBuildChain failed
*** Cause (3): *** BUILD Error- pkix_BuildForwardDepthFirstSearch failed
*** Cause (4): *** BUILD Error- userCheckerCheck failed
*** Cause (5): *** BUILD Error- userCheckerCheck failed
*** Cause (6): *** EKUCHECKER Error- Extended Key Usage Checking failed

Need to understand why it was incorrectly converted to a useless error code.
Comment 25 Alexei Volkov 2008-03-13 12:27:33 PDT
(In reply to comment #21)
> Doesn't attachment 307111 [details] [diff] [review] depend on attachment 307388 [details] [diff] [review]?
> Doesn't committing 307111 without committing 307388 cause test failures?
>
> Unfortunately, I now think that patch 307388 is wrong, and may reintroduce 
> a race that was previously fixed in this code.  That's why I haven't committed
> that patch.  Work on that is stalled while I debug and fix the windows CVS 
> client. 
Incorrect trust settings in nsCertType is the reason of the failures that we see in tinderboxes.
I'm backing out the patch 307111 to make tree green again.
Comment 26 Nelson Bolyard (seldom reads bugmail) 2008-03-13 12:35:28 PDT
Alexei, in comment 24, you wrote:
> Need to understand why it was incorrectly converted to a useless error code.

I don't understand that comment.  What error code did you think was useless?
The error "Certificate type not approved for application." means that the 
KU or EKU was invalid for the usage.  That's the right error code for an EKU
failure, I believe.  
Comment 27 Alexei Volkov 2008-03-13 13:40:49 PDT
Created attachment 309206 [details] [diff] [review]
log correct error info 

Also fixes PKIX_Error object leak.
Comment 28 Nelson Bolyard (seldom reads bugmail) 2008-03-13 15:42:54 PDT
Comment on attachment 309206 [details] [diff] [review]
log correct error info 

r=nelson

I see this changes the way that errors are reported. Please add a comment to this bug explaining what the problem was, and what this fix does.  Perhaps you can show an example of the undesirable output, and what the output looks like with this patch in place.
Comment 29 Nelson Bolyard (seldom reads bugmail) 2008-03-13 21:54:08 PDT
Created attachment 309337 [details] [diff] [review]
Correct computation of nsCertType, v2

This patch separates cert_GetCertType into two functions, one that computes
the new value, and one that conditionally stores it into the CERTCertificate.
Then rather than setting the nsCertType to zero to force it to be recomputed,
fill_CERTCertificateFields calls the second one to do the recomputation and
then stores the value itself.  This eliminates the window of time during 
which the nsCertType contains a (presumably inaccurate) zero value.
I believe this patch is correct, and the previous one was flawed.

Julien, please review
Comment 30 Nelson Bolyard (seldom reads bugmail) 2008-03-13 21:57:42 PDT
Comment on attachment 309337 [details] [diff] [review]
Correct computation of nsCertType, v2

Oh, this patch also moves the declaration of an NSS private function 
(that is not exported from nss3.dll) from a public header file to a 
private header file.
Comment 31 Nelson Bolyard (seldom reads bugmail) 2008-03-14 12:19:25 PDT
Comment on attachment 309337 [details] [diff] [review]
Correct computation of nsCertType, v2

In the interest of getting this reviewed in time for checkin TODAY, I'm adding review requests for other potential reviewers.
Only one review request is needed.
Comment 32 Nelson Bolyard (seldom reads bugmail) 2008-03-14 12:20:38 PDT
This bug blocks the fix for bug 422717
Comment 33 Robert Relyea 2008-03-14 13:40:50 PDT
Comment on attachment 309337 [details] [diff] [review]
Correct computation of nsCertType, v2

r+ rrelyea
Comment 34 Alexei Volkov 2008-03-14 16:41:49 PDT
attachment 309206 [details] [diff] [review] has been integrated.
Comment 35 Alexei Volkov 2008-03-14 19:15:52 PDT
Patch is integrated.
Comment 36 Alexei Volkov 2008-03-19 13:40:34 PDT
*** Bug 422717 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.