|
|
Created:
11 years, 7 months ago by ukai Modified:
9 years, 7 months ago Reviewers:
wtc CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement X509Certificate::Verify for Linux.
Use CERT_PKIXVerifyCert() with CRL to verify certificate.
With OCSP, CERT_PKIXVerifyCert() failed with
SEC_ERROR_INVALID_ARGS.
Increase stack size. It was not enough size if we use
CERT_PKIXVerifyCert() on some sites. For example,
https://www.google.com/ works, but https://bugs.webkit.org/
or https://www.thawte.com/ would die by SIGSEGV. This is
because pkix_List_Destroy() routine destroys PKIX_List
recursively, so if there are some long PKIX_Lists, it
consumes stack a lot and dies by stack overflow.
Note that X509Certificate::Verify isn't used in SSLClientSocketNSS yet.
BUG=10911
TEST=net_unittests passes
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17071
Patch Set 1 #
Total comments: 53
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 31
Patch Set 5 : '' #
Total comments: 9
Patch Set 6 : '' #
Total comments: 5
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 4
Patch Set 9 : '' #Messages
Total messages: 18 (0 generated)
This is good progress! Could you run "ident libnss3.so" on the libnss3.so you're using, and let me know what the version number is? The pkix_List_Destroy recursion bug you ran into has been fixed in NSS 3.12.3. I have some comments below. The only part I haven't reviewed with confidence is the revocation_flags related code. I only compared your code with vfychain.c. I will compare your code with nsIdentityChecking.cpp next. http://codereview.chromium.org/113578/diff/1/5 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/1/5#newcode18 Line 18: const int kWorkerThreadStackSize = 128 * 1024; A stack size of 64 KB is definitely too small. Based on the NSPR library that I maintain, 128 KB seems to be large enough: http://mxr.mozilla.org/nspr/ident?i=_MD_MINIMUM_STACK_SIZE http://mxr.mozilla.org/nspr/ident?i=_MD_DEFAULT_STACK_SIZE (In NSPR, originally we also used 64 KB as the default stack size, but we ran into crashes on some platforms, and increased the stack size to 128 KB on those platforms.) I prefer that we just use the system default thread stack size, but we don't need to resolve that issue here. http://codereview.chromium.org/113578/diff/1/2 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1/2#newcode278 Line 278: server_cert_ = NULL; I think you can just set server_cert_ to NULL without the null pointer check. http://codereview.chromium.org/113578/diff/1/2#newcode574 Line 574: // Remember the certificate as it will no longer be accessible if the I wonder if we should also remember the intermediate CA certs if the server sends them to us: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/... We'll need the intermediate CA certs when we verify the server's certificate later. http://codereview.chromium.org/113578/diff/1/2#newcode613 Line 613: completed_handshake_ = true; Delete this line. It has been moved to DoVerifyCertComplete. http://codereview.chromium.org/113578/diff/1/2#newcode635 Line 635: if (result < 0) { Nit: no braces. http://codereview.chromium.org/113578/diff/1/2#newcode639 Line 639: InvalidateSessionIfBadCertificate(); We now need to call InvalidateSessionIfBadCertificate() in DoVerifyCertComplete. http://codereview.chromium.org/113578/diff/1/2#newcode660 Line 660: ssl_config_.allowed_bad_certs_.count(server_cert_)) We don't need SSLClientSocketNSS::OwnBadCertHandler now. http://codereview.chromium.org/113578/diff/1/4 File net/base/ssl_client_socket_nss.h (right): http://codereview.chromium.org/113578/diff/1/4#newcode98 Line 98: CertVerifier verifier_; Nit: add a blank line above, to make it clear that the comment "Set during handshake" only applies to server_cert. http://codereview.chromium.org/113578/diff/1/4#newcode99 Line 99: CertVerifyResult server_cert_verify_result_; server_cert_verify_result_ should be able to replace server_cert_error_. http://codereview.chromium.org/113578/diff/1/3 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1/3#newcode30 Line 30: class ScopedCERTCertificate { Note: these ScopedXXX classes can also be implemented using the scoped_ptr_malloc template in "base/scoped_ptr.h". See firefox3_importer.cc and hmac_nss.cc for examples. I am fine with the custom ScopedXXX classes though. http://codereview.chromium.org/113578/diff/1/3#newcode33 Line 33: : cert_(cert) {} Please add a blank line between the methods and between the public and private sections. http://codereview.chromium.org/113578/diff/1/3#newcode56 Line 56: class ScopedCERTValOutParam { What ScopedCERTValOutParam does is less obvious, so it's a good idea to document it. For example, is 'cvout' pointing to a single CERTValOutParam object, or an array of CERTValOutParam objects? The code shows it's pointing to an array, which is not obvious. The second non-obvious point is that the destructor frees the individual array elements (p->value) but not the array itself. http://codereview.chromium.org/113578/diff/1/3#newcode87 Line 87: // Map PORT_GetError() return values to NET_ERROR. Nit: change NET_ERROR to "our network error codes". http://codereview.chromium.org/113578/diff/1/3#newcode97 Line 97: case SEC_ERROR_UNTRUSTED_CERT: This error code could also be mapped to ERR_CERT_AUTHORITY_INVALID. http://codereview.chromium.org/113578/diff/1/3#newcode98 Line 98: return ERR_CERT_CONTAINS_ERRORS; Let's not use ERR_CERT_CONTAINS_ERRORS. Use ERR_CERT_INVALID istead. http://codereview.chromium.org/113578/diff/1/3#newcode103 Line 103: case SEC_ERROR_CERT_USAGES_INVALID: Please add a TODO(port) comment that we should add an ERR_CERT_WRONG_USAGE error code. http://codereview.chromium.org/113578/diff/1/3#newcode335 Line 335: CertVerifyResult* verify_result) const { CertVerifyResult's cert_status is a bitwise-OR of the bits defined in "net/base/cert_status_flags.h". So this function should save all the certificate errors in the cert_status field, and then return the network error code corresponding to the most serious error. So this function should keep going after the CERT_VerifyCertName and CERT_CheckCertValidTimes calls. If these two functions fail, just set the corresponding bit in verify_result->cert_status. http://codereview.chromium.org/113578/diff/1/3#newcode343 Line 343: // Make sure that the cert is valid at now. Nit: change "at now" to "now". But I think CERT_PKIXVerifyCert should take care of checking the cert's validity times. http://codereview.chromium.org/113578/diff/1/3#newcode346 Line 346: if (validity != secCertTimeValid) { Nit: In the network module we usually don't use braces for if statements with a one-line body. http://codereview.chromium.org/113578/diff/1/3#newcode355 Line 355: PRUint64 revocation_method_flags = CERT_REV_M_TEST_USING_THIS_METHOD | Nit: move "CERT_REV_M_TEST_USING_THIS_METHOD |" to the next line. http://codereview.chromium.org/113578/diff/1/3#newcode396 Line 396: CERTCertList *root_certs = PK11_ListCerts(PK11CertListCA, NULL); According to documentation of cert_pi_trustAnchors, we can just pass NULL to use the default trusted roots: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... http://codereview.chromium.org/113578/diff/1/3#newcode398 Line 398: cvin[cvin_index].type = cert_pi_trustAnchors; When I compare your code with vfychain.c, I noticed that vfychain.c has one more entry (related to cert fetching) in cvin: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/vfychain... The cert_pi_useAIACertFetch type may be a new feature in NSS 3.12.3.
I compared your code with nsIdentityChecking.cpp. Your code is more similar to nsIdentityChecking.cpp than vfychain.c. I have some more info on setting up OCSP below. It will require a lot of code, so we should do that in a separate CL. http://codereview.chromium.org/113578/diff/1/3 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1/3#newcode351 Line 351: // TODO(ukai): Fix to use OSCP. NSS has a default HTTP client, used for OCSP, that doesn't know how to use a proxy server. This is the Firefox code that sets up OCSP and installs an alternative HTTP client for NSS: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSC... We can omit RegisterMyOCSPAIAInfoCallback() initially. http://codereview.chromium.org/113578/diff/1/3#newcode402 Line 402: cvin[cvin_index].value.scalar.time = PR_Now(); PR_Now() is the default, so this cvin array element can be omitted: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c...
On 2009/05/19 20:50:36, wtc wrote: > This is good progress! > > Could you run "ident libnss3.so" on the libnss3.so > you're using, and let me know what the version number > is? on Ubuntu/hardy, it uses libnss3-1d 3.12.0.3-0ubuntu0.8.04 http://packages.ubuntu.com/ja/hardy/libnss3-1d $ ident /usr/lib/libnss3.so /usr/lib/libnss3.so: $Header: NSS 3.12.0.3 Mar 18 2009 10:56:48 $ > > The pkix_List_Destroy recursion bug you ran into has > been fixed in NSS 3.12.3. Sounds good. > I have some comments below. The only part I haven't > reviewed with confidence is the revocation_flags > related code. I only compared your code with vfychain.c. > I will compare your code with nsIdentityChecking.cpp > next. Thanks! > > http://codereview.chromium.org/113578/diff/1/5 > File base/worker_pool_linux.cc (right): > > http://codereview.chromium.org/113578/diff/1/5#newcode18 > Line 18: const int kWorkerThreadStackSize = 128 * 1024; > A stack size of 64 KB is definitely too small. > > Based on the NSPR library that I maintain, 128 KB seems to > be large enough: > http://mxr.mozilla.org/nspr/ident?i=_MD_MINIMUM_STACK_SIZE > http://mxr.mozilla.org/nspr/ident?i=_MD_DEFAULT_STACK_SIZE > > (In NSPR, originally we also used 64 KB as the default > stack size, but we ran into crashes on some platforms, > and increased the stack size to 128 KB on those platforms.) > > I prefer that we just use the system default thread stack > size, but we don't need to resolve that issue here. > > http://codereview.chromium.org/113578/diff/1/2 > File net/base/ssl_client_socket_nss.cc (right): > > http://codereview.chromium.org/113578/diff/1/2#newcode278 > Line 278: server_cert_ = NULL; > I think you can just set server_cert_ to NULL without the > null pointer check. > > http://codereview.chromium.org/113578/diff/1/2#newcode574 > Line 574: // Remember the certificate as it will no longer be accessible if the > I wonder if we should also remember the intermediate CA > certs if the server sends them to us: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/... > > We'll need the intermediate CA certs when we verify the > server's certificate later. > > http://codereview.chromium.org/113578/diff/1/2#newcode613 > Line 613: completed_handshake_ = true; > Delete this line. It has been moved to > DoVerifyCertComplete. > > http://codereview.chromium.org/113578/diff/1/2#newcode635 > Line 635: if (result < 0) { > Nit: no braces. > > http://codereview.chromium.org/113578/diff/1/2#newcode639 > Line 639: InvalidateSessionIfBadCertificate(); > We now need to call InvalidateSessionIfBadCertificate() in > DoVerifyCertComplete. > > http://codereview.chromium.org/113578/diff/1/2#newcode660 > Line 660: ssl_config_.allowed_bad_certs_.count(server_cert_)) > We don't need SSLClientSocketNSS::OwnBadCertHandler now. > > http://codereview.chromium.org/113578/diff/1/4 > File net/base/ssl_client_socket_nss.h (right): > > http://codereview.chromium.org/113578/diff/1/4#newcode98 > Line 98: CertVerifier verifier_; > Nit: add a blank line above, to make it clear that the > comment "Set during handshake" only applies to server_cert. > > http://codereview.chromium.org/113578/diff/1/4#newcode99 > Line 99: CertVerifyResult server_cert_verify_result_; > server_cert_verify_result_ should be able to replace > server_cert_error_. > > http://codereview.chromium.org/113578/diff/1/3 > File net/base/x509_certificate_nss.cc (right): > > http://codereview.chromium.org/113578/diff/1/3#newcode30 > Line 30: class ScopedCERTCertificate { > Note: these ScopedXXX classes can also be implemented using > the scoped_ptr_malloc template in "base/scoped_ptr.h". > See firefox3_importer.cc and hmac_nss.cc for examples. > > I am fine with the custom ScopedXXX classes though. > > http://codereview.chromium.org/113578/diff/1/3#newcode33 > Line 33: : cert_(cert) {} > Please add a blank line between the methods and between the > public and private sections. > > http://codereview.chromium.org/113578/diff/1/3#newcode56 > Line 56: class ScopedCERTValOutParam { > What ScopedCERTValOutParam does is less obvious, so it's a > good idea to document it. > > For example, is 'cvout' pointing to a single CERTValOutParam > object, or an array of CERTValOutParam objects? The code > shows it's pointing to an array, which is not obvious. > > The second non-obvious point is that the destructor frees > the individual array elements (p->value) but not the array > itself. > > http://codereview.chromium.org/113578/diff/1/3#newcode87 > Line 87: // Map PORT_GetError() return values to NET_ERROR. > Nit: change NET_ERROR to "our network error codes". > > http://codereview.chromium.org/113578/diff/1/3#newcode97 > Line 97: case SEC_ERROR_UNTRUSTED_CERT: > This error code could also be mapped to > ERR_CERT_AUTHORITY_INVALID. > > http://codereview.chromium.org/113578/diff/1/3#newcode98 > Line 98: return ERR_CERT_CONTAINS_ERRORS; > Let's not use ERR_CERT_CONTAINS_ERRORS. Use > ERR_CERT_INVALID istead. > > http://codereview.chromium.org/113578/diff/1/3#newcode103 > Line 103: case SEC_ERROR_CERT_USAGES_INVALID: > Please add a TODO(port) comment that we should add an > ERR_CERT_WRONG_USAGE error code. > > http://codereview.chromium.org/113578/diff/1/3#newcode335 > Line 335: CertVerifyResult* verify_result) const { > CertVerifyResult's cert_status is a bitwise-OR of the > bits defined in "net/base/cert_status_flags.h". So this > function should save all the certificate errors in the > cert_status field, and then return the network error code > corresponding to the most serious error. > > So this function should keep going after the > CERT_VerifyCertName and CERT_CheckCertValidTimes calls. > If these two functions fail, just set the corresponding > bit in verify_result->cert_status. > > http://codereview.chromium.org/113578/diff/1/3#newcode343 > Line 343: // Make sure that the cert is valid at now. > Nit: change "at now" to "now". > > But I think CERT_PKIXVerifyCert should take care of checking > the cert's validity times. > > http://codereview.chromium.org/113578/diff/1/3#newcode346 > Line 346: if (validity != secCertTimeValid) { > Nit: In the network module we usually don't use braces for > if statements with a one-line body. > > http://codereview.chromium.org/113578/diff/1/3#newcode355 > Line 355: PRUint64 revocation_method_flags = CERT_REV_M_TEST_USING_THIS_METHOD | > Nit: move "CERT_REV_M_TEST_USING_THIS_METHOD |" to the next > line. > > http://codereview.chromium.org/113578/diff/1/3#newcode396 > Line 396: CERTCertList *root_certs = PK11_ListCerts(PK11CertListCA, NULL); > According to documentation of cert_pi_trustAnchors, we can > just pass NULL to use the default trusted roots: > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... > > http://codereview.chromium.org/113578/diff/1/3#newcode398 > Line 398: cvin[cvin_index].type = cert_pi_trustAnchors; > When I compare your code with vfychain.c, I noticed that > vfychain.c has one more entry (related to cert fetching) > in cvin: > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/vfychain... > > The cert_pi_useAIACertFetch type may be a new feature in > NSS 3.12.3.
Thanks for review! http://codereview.chromium.org/113578/diff/1/5 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/1/5#newcode18 Line 18: const int kWorkerThreadStackSize = 128 * 1024; On 2009/05/19 20:50:36, wtc wrote: > A stack size of 64 KB is definitely too small. > > Based on the NSPR library that I maintain, 128 KB seems to > be large enough: > http://mxr.mozilla.org/nspr/ident?i=_MD_MINIMUM_STACK_SIZE > http://mxr.mozilla.org/nspr/ident?i=_MD_DEFAULT_STACK_SIZE > > (In NSPR, originally we also used 64 KB as the default > stack size, but we ran into crashes on some platforms, > and increased the stack size to 128 KB on those platforms.) Thanks! I added the comment. > > I prefer that we just use the system default thread stack > size, but we don't need to resolve that issue here. > If we pass stack_size = 0 to PlatformThread, then it would use the system default thread stack size. Should we do it, or it's ok to use 128 KB? http://codereview.chromium.org/113578/diff/1/2 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1/2#newcode278 Line 278: server_cert_ = NULL; On 2009/05/19 20:50:36, wtc wrote: > I think you can just set server_cert_ to NULL without the > null pointer check. Done. http://codereview.chromium.org/113578/diff/1/2#newcode574 Line 574: // Remember the certificate as it will no longer be accessible if the On 2009/05/19 20:50:36, wtc wrote: > I wonder if we should also remember the intermediate CA > certs if the server sends them to us: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/... > > We'll need the intermediate CA certs when we verify the > server's certificate later. Done. http://codereview.chromium.org/113578/diff/1/2#newcode613 Line 613: completed_handshake_ = true; On 2009/05/19 20:50:36, wtc wrote: > Delete this line. It has been moved to > DoVerifyCertComplete. Done. http://codereview.chromium.org/113578/diff/1/2#newcode635 Line 635: if (result < 0) { On 2009/05/19 20:50:36, wtc wrote: > Nit: no braces. Done. http://codereview.chromium.org/113578/diff/1/2#newcode639 Line 639: InvalidateSessionIfBadCertificate(); On 2009/05/19 20:50:36, wtc wrote: > We now need to call InvalidateSessionIfBadCertificate() in > DoVerifyCertComplete. Done. http://codereview.chromium.org/113578/diff/1/2#newcode660 Line 660: ssl_config_.allowed_bad_certs_.count(server_cert_)) On 2009/05/19 20:50:36, wtc wrote: > We don't need SSLClientSocketNSS::OwnBadCertHandler now. Done. http://codereview.chromium.org/113578/diff/1/4 File net/base/ssl_client_socket_nss.h (right): http://codereview.chromium.org/113578/diff/1/4#newcode98 Line 98: CertVerifier verifier_; On 2009/05/19 20:50:36, wtc wrote: > Nit: add a blank line above, to make it clear that the > comment "Set during handshake" only applies to server_cert. Done. http://codereview.chromium.org/113578/diff/1/4#newcode99 Line 99: CertVerifyResult server_cert_verify_result_; On 2009/05/19 20:50:36, wtc wrote: > server_cert_verify_result_ should be able to replace > server_cert_error_. Done. http://codereview.chromium.org/113578/diff/1/3 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1/3#newcode30 Line 30: class ScopedCERTCertificate { On 2009/05/19 20:50:36, wtc wrote: > Note: these ScopedXXX classes can also be implemented using > the scoped_ptr_malloc template in "base/scoped_ptr.h". > See firefox3_importer.cc and hmac_nss.cc for examples. > > I am fine with the custom ScopedXXX classes though. I think scoped_ptr_malloc just free the memeory, but we need to call some destroy functions for each type, so I think we need the custom ScopedXXX classses for these types. http://codereview.chromium.org/113578/diff/1/3#newcode33 Line 33: : cert_(cert) {} On 2009/05/19 20:50:36, wtc wrote: > Please add a blank line between the methods and between the > public and private sections. Done. http://codereview.chromium.org/113578/diff/1/3#newcode56 Line 56: class ScopedCERTValOutParam { On 2009/05/19 20:50:36, wtc wrote: > What ScopedCERTValOutParam does is less obvious, so it's a > good idea to document it. > > For example, is 'cvout' pointing to a single CERTValOutParam > object, or an array of CERTValOutParam objects? The code > shows it's pointing to an array, which is not obvious. > > The second non-obvious point is that the destructor frees > the individual array elements (p->value) but not the array > itself. Done. http://codereview.chromium.org/113578/diff/1/3#newcode87 Line 87: // Map PORT_GetError() return values to NET_ERROR. On 2009/05/19 20:50:36, wtc wrote: > Nit: change NET_ERROR to "our network error codes". Done. http://codereview.chromium.org/113578/diff/1/3#newcode97 Line 97: case SEC_ERROR_UNTRUSTED_CERT: On 2009/05/19 20:50:36, wtc wrote: > This error code could also be mapped to > ERR_CERT_AUTHORITY_INVALID. Done. http://codereview.chromium.org/113578/diff/1/3#newcode98 Line 98: return ERR_CERT_CONTAINS_ERRORS; On 2009/05/19 20:50:36, wtc wrote: > Let's not use ERR_CERT_CONTAINS_ERRORS. Use > ERR_CERT_INVALID istead. Ok. remove ERR_CERT_CONTAINS_ERRORS. http://codereview.chromium.org/113578/diff/1/3#newcode103 Line 103: case SEC_ERROR_CERT_USAGES_INVALID: On 2009/05/19 20:50:36, wtc wrote: > Please add a TODO(port) comment that we should add an > ERR_CERT_WRONG_USAGE error code. Done. http://codereview.chromium.org/113578/diff/1/3#newcode335 Line 335: CertVerifyResult* verify_result) const { On 2009/05/19 20:50:36, wtc wrote: > CertVerifyResult's cert_status is a bitwise-OR of the > bits defined in "net/base/cert_status_flags.h". So this > function should save all the certificate errors in the > cert_status field, and then return the network error code > corresponding to the most serious error. > > So this function should keep going after the > CERT_VerifyCertName and CERT_CheckCertValidTimes calls. > If these two functions fail, just set the corresponding > bit in verify_result->cert_status. Done. http://codereview.chromium.org/113578/diff/1/3#newcode343 Line 343: // Make sure that the cert is valid at now. On 2009/05/19 20:50:36, wtc wrote: > Nit: change "at now" to "now". > > But I think CERT_PKIXVerifyCert should take care of checking > the cert's validity times. Done. http://codereview.chromium.org/113578/diff/1/3#newcode346 Line 346: if (validity != secCertTimeValid) { On 2009/05/19 20:50:36, wtc wrote: > Nit: In the network module we usually don't use braces for > if statements with a one-line body. Done. http://codereview.chromium.org/113578/diff/1/3#newcode351 Line 351: // TODO(ukai): Fix to use OSCP. On 2009/05/19 22:48:29, wtc wrote: > NSS has a default HTTP client, used for OCSP, that doesn't > know how to use a proxy server. > > This is the Firefox code that sets up OCSP and installs an > alternative HTTP client for NSS: > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSC... > > We can omit RegisterMyOCSPAIAInfoCallback() initially. Thanks for the info. May I leave it as my TODO? http://codereview.chromium.org/113578/diff/1/3#newcode355 Line 355: PRUint64 revocation_method_flags = CERT_REV_M_TEST_USING_THIS_METHOD | On 2009/05/19 20:50:36, wtc wrote: > Nit: move "CERT_REV_M_TEST_USING_THIS_METHOD |" to the next > line. Done. http://codereview.chromium.org/113578/diff/1/3#newcode396 Line 396: CERTCertList *root_certs = PK11_ListCerts(PK11CertListCA, NULL); On 2009/05/19 20:50:36, wtc wrote: > According to documentation of cert_pi_trustAnchors, we can > just pass NULL to use the default trusted roots: > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... Yes, I tried, but current version of NSS (NSS 3.12.0.3 Mar 18 2009 10:56:48) would die by SIGSEGV. in cert_pkixSetParam in security/nss/lib/certhigh/certvfypkix.c, it doesn't handle NULL case. http://codereview.chromium.org/113578/diff/1/3#newcode398 Line 398: cvin[cvin_index].type = cert_pi_trustAnchors; On 2009/05/19 20:50:36, wtc wrote: > When I compare your code with vfychain.c, I noticed that > vfychain.c has one more entry (related to cert fetching) > in cvin: > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/vfychain... > > The cert_pi_useAIACertFetch type may be a new feature in > NSS 3.12.3. It doesn't exist in NSS 3.12.0.3. http://codereview.chromium.org/113578/diff/1/3#newcode402 Line 402: cvin[cvin_index].value.scalar.time = PR_Now(); On 2009/05/19 22:48:29, wtc wrote: > PR_Now() is the default, so this cvin array element can be > omitted: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... Done.
I updated the change to fix net_unittests errors. I still got several errors: [ FAILED ] HTTPSRequestTest.HTTPSGetTest [ FAILED ] SSLClientSocketTest.Connect [ FAILED ] SSLClientSocketTest.Read [ FAILED ] SSLClientSocketTest.Read_SmallChunks [ FAILED ] SSLClientSocketTest.Read_Interrupted It seems there are because of the certification verification failure: failed with CERT_AUTHORITY_INVALID error (SEC_ERROR_UNTRUSTED_ISSUER returned by CERT_PKIXVerifyCert)
Fumitoshi, Let me respond to your comments first. I will review your new Patch Set next. http://codereview.chromium.org/113578/diff/1/5 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/1/5#newcode18 Line 18: const int kWorkerThreadStackSize = 128 * 1024; On 2009/05/20 07:33:20, ukai wrote: > > If we pass stack_size = 0 to PlatformThread, then it would use the system > default thread stack size. Should we do it, or it's ok to use 128 KB? Let's just increase the thread stack size to 128KB in this CL. We can open a bug to discuss if we should use the system default thread stack size, which can be 512KB or 1MB, several times bigger than 128KB. (My opinion is to use the system default thread stack size.) http://codereview.chromium.org/113578/diff/1/3 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1/3#newcode30 Line 30: class ScopedCERTCertificate { On 2009/05/20 07:33:20, ukai wrote: > > I think scoped_ptr_malloc just free the memeory, but we need to call some > destroy functions for each type, so I think we need the custom ScopedXXX > classses for these types. By default scoped_ptr_malloc just calls free(), but you can pass a custom free function as the second argument (class FreeProc) to the scoped_ptr_malloc template. You can see examples of that in hmac_nss.cc. On second thought, I'm not sure if using scoped_ptr_malloc is a win because we need to define custom free functions. http://codereview.chromium.org/113578/diff/1/3#newcode351 Line 351: // TODO(ukai): Fix to use OSCP. On 2009/05/20 07:33:20, ukai wrote: > > Thanks for the info. > May I leave it as my TODO? Certainly. Feel free to leave any of my suggestions as a TODO. http://codereview.chromium.org/113578/diff/1/3#newcode396 Line 396: CERTCertList *root_certs = PK11_ListCerts(PK11CertListCA, NULL); On 2009/05/20 07:33:20, ukai wrote: > > Yes, I tried, but current version of NSS (NSS 3.12.0.3 Mar 18 2009 10:56:48) > would die by SIGSEGV. > in cert_pkixSetParam in security/nss/lib/certhigh/certvfypkix.c, it doesn't > handle NULL case. That code hasn't changed since NSS_3_12_RTM. I filed a bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=494087 Instead of NULL, could you try passing a pointer to an empty CERTCertList? Call CERT_NewCertList() to get an empty CERTCertList. http://codereview.chromium.org/113578/diff/1/3#newcode398 Line 398: cvin[cvin_index].type = cert_pi_trustAnchors; On 2009/05/20 07:33:20, ukai wrote: > > It doesn't exist in NSS 3.12.0.3. You're right. cert_pi_useAIACertFetch was added in NSS 3.12.1.
Hi Fumitoshi, I'm sorry that I didn't finish the review because I discovered a hairy issue. Please see my comments below. http://codereview.chromium.org/113578/diff/1011/31 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/1011/31#newcode18 Line 18: // A stack size of 64 KB is definitely too small (at least for NSPR library), Please remove "at least for NSPR library" from the comment because it's not clear without the context. You can say "A stack size of 64 KB is too small for the CERT_PKIXVerifyCert function of NSS because of NSS bug 439169." http://codereview.chromium.org/113578/diff/1011/28 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/28#newcode721 Line 721: InvalidateSessionIfBadCertificate(); I just realized that if we don't verify the certificate during our OwnAuthCertHandler callback, there is a race condition that another SSL connection to the same server will succeed by resuming the SSL session, even though we haven't verified the cert yet. Markus discovered this issue when he worked on this code last time. This problem is very difficult to solve. I will need to spend some time to think about it. At least, we need to have a map of SSL session IDs (SSL_GetSessionID()) to certificates and their status. This map is necessary because NSS doesn't call our OwnAuthCertHandler callback for a session resumption handshake, so we need to look up the cert from the map for those connections. I hope you understand what I'm talking about... So -- you may want to reduce this CL to exclude the code that actually uses verifier_.Verify in ssl_client_socket_nss.cc, so that you can check in that part first, while I figure out how to use verifier_.Verify securely in ssl_client_socket_nss.cc. This will require you to undo some of the changes I asked you to make yesterday (such as removing OwnBadCertHandler). Sorry! http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode58 Line 58: // ScopedCERTValOutParam manages ownership of values in CERTValOutParam array Nit: "ownership" => "destruction" Add "the" after "in" http://codereview.chromium.org/113578/diff/1011/29#newcode59 Line 59: // pointed by cvout. cvout must be initialized as passed to "pointed by cvout" => "that cvout points to" http://codereview.chromium.org/113578/diff/1011/29#newcode63 Line 63: // and cert_po_certList, but doesn't release the array itself. Add "types" after cert_po_certList. http://codereview.chromium.org/113578/diff/1011/29#newcode386 Line 386: // We need to sets up OCSP and installs an HTTP client for NSS. Nit: "sets" => "set" "installs" => "install" http://codereview.chromium.org/113578/diff/1011/29#newcode436 Line 436: // TODO(ukai): use cert_pi_useAIACertFetch (new feature in NSS 3.12.3). Nit: "3.12.3" => "3.12.1". http://codereview.chromium.org/113578/diff/1011/29#newcode457 Line 457: LOG(ERROR) << "PKIXVerifyError err=" << err; Nit: "PKIXVerifyError" => "CERT_PKIXVerifyCert failed" http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || I don't understand this if statement. Could you explain it? http://codereview.chromium.org/113578/diff/1011/29#newcode460 Line 460: err == SEC_ERROR_UNTRUSTED_ISSUER))) { // when common name invalid SEC_ERROR_UNTRUSTED_ISSUER != "common name invalid"
Thanks for review! http://codereview.chromium.org/113578/diff/1011/31 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/1011/31#newcode18 Line 18: // A stack size of 64 KB is definitely too small (at least for NSPR library), On 2009/05/21 02:53:09, wtc wrote: > Please remove "at least for NSPR library" from the comment > because it's not clear without the context. > > You can say "A stack size of 64 KB is too small for the > CERT_PKIXVerifyCert function of NSS because of NSS bug 439169." Done. http://codereview.chromium.org/113578/diff/1011/28 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/28#newcode721 Line 721: InvalidateSessionIfBadCertificate(); On 2009/05/21 02:53:09, wtc wrote: > I just realized that if we don't verify the certificate > during our OwnAuthCertHandler callback, there is a race > condition that another SSL connection to the same server > will succeed by resuming the SSL session, even though we > haven't verified the cert yet. Markus discovered this > issue when he worked on this code last time. > > This problem is very difficult to solve. I will need to > spend some time to think about it. At least, we need to > have a map of SSL session IDs (SSL_GetSessionID()) to > certificates and their status. This map is necessary > because NSS doesn't call our OwnAuthCertHandler callback > for a session resumption handshake, so we need to look > up the cert from the map for those connections. > > I hope you understand what I'm talking about... > > So -- you may want to reduce this CL to exclude the > code that actually uses verifier_.Verify in > ssl_client_socket_nss.cc, so that you can check in that > part first, while I figure out how to use verifier_.Verify > securely in ssl_client_socket_nss.cc. > > This will require you to undo some of the changes I asked > you to make yesterday (such as removing OwnBadCertHandler). > Sorry! Ok. I revert the change in this file and its header. If it's ok to add code to remember intermedidate CA certs in OwnAuthCertHandler and looks good, I'll create another change for it. http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode58 Line 58: // ScopedCERTValOutParam manages ownership of values in CERTValOutParam array On 2009/05/21 02:53:09, wtc wrote: > Nit: "ownership" => "destruction" > > Add "the" after "in" Done. http://codereview.chromium.org/113578/diff/1011/29#newcode59 Line 59: // pointed by cvout. cvout must be initialized as passed to On 2009/05/21 02:53:09, wtc wrote: > "pointed by cvout" => "that cvout points to" Done. http://codereview.chromium.org/113578/diff/1011/29#newcode63 Line 63: // and cert_po_certList, but doesn't release the array itself. On 2009/05/21 02:53:09, wtc wrote: > Add "types" after cert_po_certList. Done. http://codereview.chromium.org/113578/diff/1011/29#newcode386 Line 386: // We need to sets up OCSP and installs an HTTP client for NSS. On 2009/05/21 02:53:09, wtc wrote: > Nit: "sets" => "set" > "installs" => "install" Done. http://codereview.chromium.org/113578/diff/1011/29#newcode436 Line 436: // TODO(ukai): use cert_pi_useAIACertFetch (new feature in NSS 3.12.3). On 2009/05/21 02:53:09, wtc wrote: > Nit: "3.12.3" => "3.12.1". Done. http://codereview.chromium.org/113578/diff/1011/29#newcode457 Line 457: LOG(ERROR) << "PKIXVerifyError err=" << err; On 2009/05/21 02:53:09, wtc wrote: > Nit: "PKIXVerifyError" => "CERT_PKIXVerifyCert failed" Done. http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 02:53:09, wtc wrote: > I don't understand this if statement. Could you explain it? 1. SSLClientSocketTest.ConnectExpired failed, beucase CERT_PKIXVerifyCert sets SEC_ERROR_CERT_NOT_VALID (NSS's bug?), so this function would return ERR_CERT_INVALID, but it is expected to return ERR_CERT_DATE_INVALID. So, if cert_status is already set above, we don't need to set new error status, so that it would return ERR_CERT_DATE_INVALID. 2. SSLClientSocketTest.ConnectMismatched failed, because CERT_PKIXVerifyCert sets SEC_ERROR_UNTRUSTED_CERT, so this function would return ERR_CERT_AUTHORITY_INVALID. I think this would be problem that we can't use root_ca_cert.crt as trusted root CA in SSLClientSocketTest. I may need to fix it in another place http://codereview.chromium.org/113578/diff/1011/29#newcode460 Line 460: err == SEC_ERROR_UNTRUSTED_ISSUER))) { // when common name invalid On 2009/05/21 02:53:09, wtc wrote: > SEC_ERROR_UNTRUSTED_ISSUER != "common name invalid" well, found out I got SEC_ERROR_UNTRUSTED_ISSUER in SSLClientSocketTest (with test root_ca_cert.crt). Do we need to initialize NSS with database to fix it?
The only remaining issue is to figure out how to make CERT_PKIXVerifyCert trust our test root CA certificate. I suggest an experiment below. If it doesn't work, we'll need to either figure out a way to pass our test root CA certificate in cert_pi_trustAnchors *for unit tests only*, or switch to the old CERT_VerifyCertificate function and only use CERT_PKIXVerifyCert for EV certificate verification (which is what Firefox is doing). Even if we do that, don't throw away your CERT_PKIXVerifyCert code. It'll be useful in the future. http://codereview.chromium.org/113578/diff/1011/28 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/28#newcode721 Line 721: InvalidateSessionIfBadCertificate(); On 2009/05/21 06:28:14, ukai wrote: > > If it's ok to add code to remember intermedidate CA certs in OwnAuthCertHandler > and looks good, I'll create another change for it. I didn't look at that code, but please create another CL for it or at least add a TODO comment. http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 06:28:14, ukai wrote: > > 1. SSLClientSocketTest.ConnectExpired failed, beucase CERT_PKIXVerifyCert sets > SEC_ERROR_CERT_NOT_VALID (NSS's bug?), so this function would return > ERR_CERT_INVALID, but it is expected to return ERR_CERT_DATE_INVALID. I checked the NSS source code and agreed that it's probably an NSS bug. I filed an NSS bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=494226 Please add a comment that says "CERT_PKIXVerifyCert reports SEC_ERROR_CERT_NOT_VALID instead of SEC_ERROR_EXPIRED_CERTIFICATE for an expired certificate." > 2. SSLClientSocketTest.ConnectMismatched failed, because CERT_PKIXVerifyCert > sets SEC_ERROR_UNTRUSTED_CERT, so this function would return > ERR_CERT_AUTHORITY_INVALID. > I think this would be problem that we can't use root_ca_cert.crt as trusted root > CA in SSLClientSocketTest. > I may need to fix it in another place Yes, it seems that CERT_PKIXVerifyCert is not using our test root CA cert even though we add it to NSS at run time in ssl_test_util.cc:LoadTemporaryCert. I wonder if this is because we pass a list of CAs in cert_pi_trustAnchors that doesn't include our test root CA. My debugging of NSS bug 494087 https://bugzilla.mozilla.org/show_bug.cgi?id=494087#c4 showed that we can omit cert_pi_trustAnchors and still get the desired behavior (use all the default trusted root CAs). Could you try omitting the cert_pi_trustAnchors entry in the cvin array and see if that solves the problem? http://codereview.chromium.org/113578/diff/1011/29#newcode460 Line 460: err == SEC_ERROR_UNTRUSTED_ISSUER))) { // when common name invalid On 2009/05/21 06:28:14, ukai wrote: > > well, found out I got SEC_ERROR_UNTRUSTED_ISSUER in SSLClientSocketTest (with > test root_ca_cert.crt). > Do we need to initialize NSS with database to fix it? No. Initializing NSS with databases is necessary for other features. For certificate verification, we don't need NSS databases. http://codereview.chromium.org/113578/diff/36/1015 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/36/1015#newcode18 Line 18: // A stack size of 64 KB is definitely too small for the CERT_PKIX_VerifyCert Nit: remove "definitely". CERT_PKIX_VerifyCert => CERT_PKIXVerifyCert http://codereview.chromium.org/113578/diff/36/1014 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/36/1014#newcode30 Line 30: class ScopedCERTCertificate { Nit: In these three ScopedXXX classes, please add a blank line: 1. between the constructor and the destructor 2. between the private data member and DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/113578/diff/36/1014#newcode384 Line 384: // TODO(ukai): Fix to use OSCP. Typo: OSCP => OCSP The next line also has this typo. http://codereview.chromium.org/113578/diff/36/1014#newcode457 Line 457: LOG(ERROR) << "CERT_PKIXVerifyError err=" << err; Typo: CERT_PKIXVerifyError => CERT_PKIXVerifyCert failed
http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 18:40:08, wtc wrote: > > I checked the NSS source code and agreed that it's > probably an NSS bug. I filed an NSS bug report: > https://bugzilla.mozilla.org/show_bug.cgi?id=494226 OK, I confirmed that this is a known bug: https://bugzilla.mozilla.org/show_bug.cgi?id=491174 I think we should work around it by changing SEC_ERROR_CERT_NOT_VALID to SEC_ERROR_EXPIRED_CERTIFICATE after a CERT_PKIXVerifyCert failure. Something like this: int err = PORT_GetError(); // CERT_PKIXVerifyCert reports the wrong error code for // expired certificates (NSS bug 491174). if (err == SEC_ERROR_CERT_NOT_VALID && (verify_result->cert_status & CERT_STATUS_DATE_INVALID) != 0) err = SEC_ERROR_EXPIRED_CERTIFICATE;
http://codereview.chromium.org/113578/diff/1011/28 File net/base/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/28#newcode721 Line 721: InvalidateSessionIfBadCertificate(); On 2009/05/21 18:40:08, wtc wrote: > On 2009/05/21 06:28:14, ukai wrote: > > > > If it's ok to add code to remember intermedidate CA certs in > OwnAuthCertHandler > > and looks good, I'll create another change for it. > > I didn't look at that code, but please create another CL > for it or at least add a TODO comment. Ok, I'll create another CL for it. http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 21:15:44, wtc wrote: > On 2009/05/21 18:40:08, wtc wrote: > > > > I checked the NSS source code and agreed that it's > > probably an NSS bug. I filed an NSS bug report: > > https://bugzilla.mozilla.org/show_bug.cgi?id=494226 > > OK, I confirmed that this is a known bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=491174 > > I think we should work around it by changing > SEC_ERROR_CERT_NOT_VALID to SEC_ERROR_EXPIRED_CERTIFICATE > after a CERT_PKIXVerifyCert failure. Something like this: > int err = PORT_GetError(); > // CERT_PKIXVerifyCert reports the wrong error code for > // expired certificates (NSS bug 491174). > if (err == SEC_ERROR_CERT_NOT_VALID && > (verify_result->cert_status & CERT_STATUS_DATE_INVALID) != 0) > err = SEC_ERROR_EXPIRED_CERTIFICATE; Done. http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 18:40:08, wtc wrote: > On 2009/05/21 06:28:14, ukai wrote: > > > > 1. SSLClientSocketTest.ConnectExpired failed, beucase CERT_PKIXVerifyCert > sets > > SEC_ERROR_CERT_NOT_VALID (NSS's bug?), so this function would return > > ERR_CERT_INVALID, but it is expected to return ERR_CERT_DATE_INVALID. > > I checked the NSS source code and agreed that it's > probably an NSS bug. I filed an NSS bug report: > https://bugzilla.mozilla.org/show_bug.cgi?id=494226 > > Please add a comment that says "CERT_PKIXVerifyCert > reports SEC_ERROR_CERT_NOT_VALID instead of > SEC_ERROR_EXPIRED_CERTIFICATE for an expired certificate." > > > 2. SSLClientSocketTest.ConnectMismatched failed, because CERT_PKIXVerifyCert > > sets SEC_ERROR_UNTRUSTED_CERT, so this function would return > > ERR_CERT_AUTHORITY_INVALID. > > I think this would be problem that we can't use root_ca_cert.crt as trusted > root > > CA in SSLClientSocketTest. > > I may need to fix it in another place > > Yes, it seems that CERT_PKIXVerifyCert is not using our > test root CA cert even though we add it to NSS at run > time in ssl_test_util.cc:LoadTemporaryCert. > > I wonder if this is because we pass a list of CAs in > cert_pi_trustAnchors that doesn't include our test > root CA. > > My debugging of NSS bug 494087 > https://bugzilla.mozilla.org/show_bug.cgi?id=494087#c4 > showed that we can omit cert_pi_trustAnchors and still > get the desired behavior (use all the default trusted > root CAs). > > Could you try omitting the cert_pi_trustAnchors > entry in the cvin array and see if that solves the > problem? I try omitting the cert_pi_trustAnchors (and use cert_verifier in ssl_client_socket_nss), iI got SEC_ERROR_UNTRUSTED_ISSUER (-8172) error in SSLClientScoketTest.Connect or so. http://codereview.chromium.org/113578/diff/36/1015 File base/worker_pool_linux.cc (right): http://codereview.chromium.org/113578/diff/36/1015#newcode18 Line 18: // A stack size of 64 KB is definitely too small for the CERT_PKIX_VerifyCert On 2009/05/21 18:40:09, wtc wrote: > Nit: remove "definitely". > > CERT_PKIX_VerifyCert => CERT_PKIXVerifyCert Done. http://codereview.chromium.org/113578/diff/36/1014 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/36/1014#newcode30 Line 30: class ScopedCERTCertificate { On 2009/05/21 18:40:09, wtc wrote: > Nit: In these three ScopedXXX classes, please add a blank line: > 1. between the constructor and the destructor > 2. between the private data member and DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/113578/diff/36/1014#newcode384 Line 384: // TODO(ukai): Fix to use OSCP. On 2009/05/21 18:40:09, wtc wrote: > Typo: OSCP => OCSP > > The next line also has this typo. Done. http://codereview.chromium.org/113578/diff/36/1014#newcode384 Line 384: // TODO(ukai): Fix to use OSCP. On 2009/05/21 18:40:09, wtc wrote: > Typo: OSCP => OCSP > > The next line also has this typo. Done. http://codereview.chromium.org/113578/diff/36/1014#newcode457 Line 457: LOG(ERROR) << "CERT_PKIXVerifyError err=" << err; On 2009/05/21 18:40:09, wtc wrote: > Typo: CERT_PKIXVerifyError => CERT_PKIXVerifyCert failed Done.
http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/21 06:28:14, ukai wrote: > > 2. SSLClientSocketTest.ConnectMismatched failed, because CERT_PKIXVerifyCert > sets SEC_ERROR_UNTRUSTED_CERT, so this function would return > ERR_CERT_AUTHORITY_INVALID. I'm reading your old comment again. Could you confirm that if you pass PK11_ListCerts(PK11CertListCA, NULL) as cert_pi_trustAnchors, you get the SEC_ERROR_UNTRUSTED_CERT error (-8171) rather than SEC_ERROR_UNTRUSTED_ISSUER (-8172)?
LGTM. Thanks for your patience. It's a long journey. I will file NSS bugs about the issues and find out what we need to do. As I mentioned yesterday, we may need to change X509Certificate::Verify to use the old CERT_VerifyCert* function, and only use the new CERT_PKIXVerify function in X509Certificate::IsEV. Here is how Firefox verifies an SSL server certificate: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssla... We should still keep your current PKIX code. We can create two implementations of the Verify method, use a bool member to select which one to use, and set that bool member to select the CERT_VerifyCertNow-based implementation for now. Let's do this in a separate CL. http://codereview.chromium.org/113578/diff/40/41 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/40/41#newcode372 Line 372: Let's add a TODO comment to note the current status of this method. 1. It's not being used. 2. The problem is that we can't pass the unit tests (mention the error code), probably because we can't make CERT_PKIXCertVerify trust our test root CA cert. http://codereview.chromium.org/113578/diff/40/41#newcode450 Line 450: #if 0 Let's just delete this #if 0 code. We know we can't pass NULL as cert_po_trustAnchor anyway.
Thanks for review. I notice I did wrong experiments and find another issue. Could you take a look at this change again, please? http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/22 18:31:53, wtc wrote: > On 2009/05/21 06:28:14, ukai wrote: > > > > 2. SSLClientSocketTest.ConnectMismatched failed, because CERT_PKIXVerifyCert > > sets SEC_ERROR_UNTRUSTED_CERT, so this function would return > > ERR_CERT_AUTHORITY_INVALID. > > I'm reading your old comment again. Could you confirm > that if you pass PK11_ListCerts(PK11CertListCA, NULL) > as cert_pi_trustAnchors, you get the SEC_ERROR_UNTRUSTED_CERT > error (-8171) rather than SEC_ERROR_UNTRUSTED_ISSUER (-8172)? > Oh, sorry. I did wrong experiments. If I use CertVerifier in ssl_client_socket_nss and 1. If I use PK11_ListCerts(PK11CertLicaCA, NULL) for cert_pi_trustAnchors, I get -8172 (SEC_ERROR_UNTRUSTED_ISSUER) for our test root CA certs. 2. If I use NULL for cert_pi_trustAnchors, net_unittests dies by segfault. 3. If I use CERT_NewCertList() for cert_pi_trustAnchors, net_unittests passed. So, we should use CERT_NewCertList() for cert_pi_trustAnchors. However, I got segfault when net_unittests is going to finish (in NSPR). ... [----------] Global test environment tear-down [==========] 587 tests from 53 test cases ran. (109151 ms total) [ PASSED ] 587 tests. YOU HAVE 9 DISABLED TESTS Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0xf7128b90 (LWP 26655)] 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 (gdb) bt #0 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 #1 0xf776a8c2 in PR_Lock (lock=0x0) at ptsynch.c:206 #2 0xf776f9b5 in _pt_thread_death_internal (arg=0x93d3d00, callDestructors=1) at ptthread.c:827 #3 0xf7735bb0 in __nptl_deallocate_tsd () from /lib32/libpthread.so.0 #4 0xf7736509 in start_thread () from /lib32/libpthread.so.0 #5 0xf756809e in clone () from /lib32/libc.so.6 (gdb) http://codereview.chromium.org/113578/diff/40/41#newcode372 Line 372: // Make sure that the hostname matches with the common name of the cert. On 2009/05/22 18:49:48, wtc wrote: > Let's add a TODO comment to note the current status of this > method. > > 1. It's not being used. > > 2. The problem is that we can't pass the unit tests (mention > the error code), probably because we can't make > CERT_PKIXCertVerify trust our test root CA cert. Done. http://codereview.chromium.org/113578/diff/40/41#newcode450 Line 450: cvout[cvout_index].type = cert_po_end; On 2009/05/22 18:49:48, wtc wrote: > Let's just delete this #if 0 code. We know we can't > pass NULL as cert_po_trustAnchor anyway. Oops, this was wrong #if 0. we can't pass NULL as cert_pi_trustAnchors (input), but we should pass NULL as cert_po_trustAnchor (output). Sorry for confusion.
Let me first respond to your comments. I will review the new Patch Set next. http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/25 11:21:19, ukai wrote: > > If I use CertVerifier in ssl_client_socket_nss and > > 1. If I use PK11_ListCerts(PK11CertLicaCA, NULL) for cert_pi_trustAnchors, I > get -8172 (SEC_ERROR_UNTRUSTED_ISSUER) for our test root CA certs. > > 2. If I use NULL for cert_pi_trustAnchors, net_unittests dies by segfault. > > 3. If I use CERT_NewCertList() for cert_pi_trustAnchors, net_unittests passed. > > So, we should use CERT_NewCertList() for cert_pi_trustAnchors. That's good news. What about the fourth case: 4. If you omit cert_pi_trustAnchors, what happens? If net_unittests also passes in this case, that's even better news. > However, I got segfault when net_unittests is going to finish (in NSPR). > > ... > [----------] Global test environment tear-down > [==========] 587 tests from 53 test cases ran. (109151 ms total) > [ PASSED ] 587 tests. > > YOU HAVE 9 DISABLED TESTS > > > Program received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0xf7128b90 (LWP 26655)] > 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 > (gdb) bt > #0 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 > #1 0xf776a8c2 in PR_Lock (lock=0x0) at ptsynch.c:206 > #2 0xf776f9b5 in _pt_thread_death_internal (arg=0x93d3d00, callDestructors=1) > at ptthread.c:827 > #3 0xf7735bb0 in __nptl_deallocate_tsd () from /lib32/libpthread.so.0 > #4 0xf7736509 in start_thread () from /lib32/libpthread.so.0 > #5 0xf756809e in clone () from /lib32/libc.so.6 > (gdb) This is bad... I think this is because the threads in our thread pool terminate after we have called PR_Cleanup. A solution is to call PR_DetachThread() after the cert_->Verify() call in DoVerify() in cert_verifier.cc. You will need to put it inside #if defined(OS_LINUX). Another solution, which Firefox does, is to not call PR_Cleanup(). This requires adding many leaks to the valgrind suppressions.txt. Let's try the PR_DetachThread() workaround first. Yet another solution is to terminate the thread pool before we call PR_Cleanup. I don't know how hard that is. http://codereview.chromium.org/113578/diff/40/41#newcode450 Line 450: cvout[cvout_index].type = cert_po_end; On 2009/05/25 11:21:19, ukai wrote: > > Oops, this was wrong #if 0. > we can't pass NULL as cert_pi_trustAnchors (input), but we should pass NULL as > cert_po_trustAnchor (output). > Sorry for confusion. Ah, you're right. It's my mistake -- I thought this is cvin and cert_pi_trustAnchor :-)
LGTM! Just two comment nits below. http://codereview.chromium.org/113578/diff/1027/45 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1027/45#newcode377 Line 377: // in PR_Cleanup called in NSSInitSingleton destructor. Should this line read if PR_Cleanup is called in NSSInitSingleton destructor ? http://codereview.chromium.org/113578/diff/1027/45#newcode443 Line 443: // We get SEC_ERROR_UNTRUSTED_ISSUER (-8172) for our test root CA certs with Nit: "certs" => "cert". We only only one test root CA cert.
Thanks for review! http://codereview.chromium.org/113578/diff/1011/29 File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/113578/diff/1011/29#newcode458 Line 458: if (!IsCertStatusError(verify_result->cert_status) || On 2009/05/28 00:51:33, wtc wrote: > On 2009/05/25 11:21:19, ukai wrote: > > > > If I use CertVerifier in ssl_client_socket_nss and > > > > 1. If I use PK11_ListCerts(PK11CertLicaCA, NULL) for cert_pi_trustAnchors, I > > get -8172 (SEC_ERROR_UNTRUSTED_ISSUER) for our test root CA certs. > > > > 2. If I use NULL for cert_pi_trustAnchors, net_unittests dies by segfault. > > > > 3. If I use CERT_NewCertList() for cert_pi_trustAnchors, net_unittests passed. > > > > So, we should use CERT_NewCertList() for cert_pi_trustAnchors. > > That's good news. What about the fourth case: > > 4. If you omit cert_pi_trustAnchors, what happens? > > If net_unittests also passes in this case, that's even > better news. It looks ok! Removed cert_pi_trustAnchors. > > > However, I got segfault when net_unittests is going to finish (in NSPR). > > > > ... > > [----------] Global test environment tear-down > > [==========] 587 tests from 53 test cases ran. (109151 ms total) > > [ PASSED ] 587 tests. > > > > YOU HAVE 9 DISABLED TESTS > > > > > > Program received signal SIGSEGV, Segmentation fault. > > [Switching to Thread 0xf7128b90 (LWP 26655)] > > 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 > > (gdb) bt > > #0 0xf7738540 in pthread_mutex_lock () from /lib32/libpthread.so.0 > > #1 0xf776a8c2 in PR_Lock (lock=0x0) at ptsynch.c:206 > > #2 0xf776f9b5 in _pt_thread_death_internal (arg=0x93d3d00, callDestructors=1) > > at ptthread.c:827 > > #3 0xf7735bb0 in __nptl_deallocate_tsd () from /lib32/libpthread.so.0 > > #4 0xf7736509 in start_thread () from /lib32/libpthread.so.0 > > #5 0xf756809e in clone () from /lib32/libc.so.6 > > (gdb) > > This is bad... I think this is because the threads in > our thread pool terminate after we have called PR_Cleanup. > > A solution is to call PR_DetachThread() after the > cert_->Verify() call in DoVerify() in cert_verifier.cc. > You will need to put it inside #if defined(OS_LINUX). It fixes the issue. Thanks! I'll send another change for it. > > Another solution, which Firefox does, is to not call > PR_Cleanup(). This requires adding many leaks to the > valgrind suppressions.txt. Let's try the PR_DetachThread() > workaround first. > > Yet another solution is to terminate the thread pool > before we call PR_Cleanup. I don't know how hard that is. http://codereview.chromium.org/113578/diff/1027/45#newcode377 Line 377: // Make sure that the cert is valid now. On 2009/05/28 01:00:47, wtc wrote: > Should this line read > if PR_Cleanup is called in NSSInitSingleton destructor > ? yes. fixed. http://codereview.chromium.org/113578/diff/1027/45#newcode443 Line 443: int cvout_index = 0; On 2009/05/28 01:00:47, wtc wrote: > Nit: "certs" => "cert". We only only one test root CA cert. Done. |