Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(605)

Issue 113578: Implement X509Certificate::Verify for Linux.... (Closed)

Created:
11 years, 7 months ago by ukai
Modified:
9 years, 7 months ago
Reviewers:
wtc
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Implement 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 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -8 lines) Patch
M base/worker_pool_linux.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments Download
M net/base/x509_certificate_nss.cc View 1 2 3 4 5 6 7 8 4 chunks +270 lines, -7 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
ukai
11 years, 7 months ago (2009-05-19 11:38:05 UTC) #1
wtc
This is good progress! Could you run "ident libnss3.so" on the libnss3.so you're using, and ...
11 years, 7 months ago (2009-05-19 20:50:36 UTC) #2
wtc
I compared your code with nsIdentityChecking.cpp. Your code is more similar to nsIdentityChecking.cpp than vfychain.c. ...
11 years, 7 months ago (2009-05-19 22:48:29 UTC) #3
ukai
On 2009/05/19 20:50:36, wtc wrote: > This is good progress! > > Could you run ...
11 years, 7 months ago (2009-05-20 07:32:52 UTC) #4
ukai
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 ...
11 years, 7 months ago (2009-05-20 07:33:20 UTC) #5
ukai
I updated the change to fix net_unittests errors. I still got several errors: [ FAILED ...
11 years, 7 months ago (2009-05-20 09:08:54 UTC) #6
wtc
Fumitoshi, Let me respond to your comments first. I will review your new Patch Set ...
11 years, 7 months ago (2009-05-21 00:41:28 UTC) #7
wtc
Hi Fumitoshi, I'm sorry that I didn't finish the review because I discovered a hairy ...
11 years, 7 months ago (2009-05-21 02:53:09 UTC) #8
ukai
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 ...
11 years, 7 months ago (2009-05-21 06:28:14 UTC) #9
wtc
The only remaining issue is to figure out how to make CERT_PKIXVerifyCert trust our test ...
11 years, 7 months ago (2009-05-21 18:40:08 UTC) #10
wtc
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: ...
11 years, 7 months ago (2009-05-21 21:15:44 UTC) #11
ukai
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 ...
11 years, 7 months ago (2009-05-22 06:16:32 UTC) #12
wtc
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: ...
11 years, 7 months ago (2009-05-22 18:31:53 UTC) #13
wtc
LGTM. Thanks for your patience. It's a long journey. I will file NSS bugs about ...
11 years, 7 months ago (2009-05-22 18:49:48 UTC) #14
ukai
Thanks for review. I notice I did wrong experiments and find another issue. Could you ...
11 years, 7 months ago (2009-05-25 11:21:19 UTC) #15
wtc
Let me first respond to your comments. I will review the new Patch Set next. ...
11 years, 7 months ago (2009-05-28 00:51:33 UTC) #16
wtc
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 ...
11 years, 7 months ago (2009-05-28 01:00:47 UTC) #17
ukai
11 years, 7 months ago (2009-05-28 08:25:57 UTC) #18
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.

Powered by Google App Engine
This is Rietveld 408576698