Provides a certificate for SSL client authentication on NSS sockets....

[Replaced by http://codereview.chromium.org/276037 ]

Provides a certificate for SSL client authentication on NSS sockets.
GUI is still missing, so certificates and private keys have to be
stored manually, p.e.:
 $ pk12util -d sql:$HOME/.pki/nssdb -i PKCS12_file.p12
Adds --auto-ssl-client-auth command-line option to enable this feature.

BUG=16830
TEST=Try to connect to a web page that requires SSL authentication and confirm that it connects if and only if a valid certificate is stored in .pki/nssdb database.

[wtc, 2009-09-24 17:19:19]

Great!  I will look at the patch tomorrow or on
Monday.  (I'm busy today.)  Thanks a lot for the
patch.

[wtc, 2009-09-24 17:36:01]

I took a quick look.  The main issue with your patch is
that it does not ask the user for approval of sending his
client certificate to the server.  Please see
http://dev.chromium.org/developers/design-documents/ssl-client-authentication
for the reason why we need to ask the user for approval.

We don't have a cert selection dialog for Linux yet.
As a workaround, we can resurrect the
--auto-ssl-client-auth command-line option for Linux.
See r18819 and r19456.

[wtc, 2009-09-25 15:59:43]

http://codereview.chromium.org/220009/diff/6001/7002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/6001/7002#newcode70
Line 70: #include "chrome/common/chrome_switches.h"
The net module can't use functions from the chrome module.
I will describe the right solution below.

http://codereview.chromium.org/220009/diff/6001/7002#newcode717
Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert,
pRetKey);
We should consider NSS_GetClientAuthData as a reference
implementation of the auth cert handler.  Its limitation
is that it returns only one cert, even if the user has
multiple certs that meet the server's criteria:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/auth...

Note the "break" statement in that for loop.

Our auth cert handler can be based on the
NSS_GetClientAuthData algorithm, with the following
differences:
The first time, it finds all the certs that meet the
server's criteria, saves the certs, and then returns
SECFailure to make the handshake fail.  We need to
make sure some other code sets the ERR_SSL_CLIENT_AUTH_CERT_NEEDED error.
This error code will cause higher-level code to
retrieves the list of client certs from this object,
and ask the user to select one.

Since we don't have a cert selection dialog for Linux
yet, in resource_dispatcher_host.cc, we check
CommandLine::ForCurrentProcess()->HasSwitch(switches::kAutoSSLClientAuth).
If true, we pick the first cert in the list automatically.
If false, we return NULL.

If a cert is selected (automatically), the higher-level
code will redo the handshake, specifying the chosen
client cert for us to use.

The second time this auth cert handler is called, the
higher-level code should have specified a client cert,
chosen by the user, for us to use.  Then we just return
that cert.

[wtc, 2009-09-25 16:01:07]

http://codereview.chromium.org/220009/diff/6001/7003
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/220009/diff/6001/7003#newcode95
Line 95: #if defined(USE_NSS)
This test should be #if defined(OS_LINUX)
because the issue is that we don't have a cert selection
dialog for Linux.

[wtc, 2009-09-25 16:09:07]

http://codereview.chromium.org/220009/diff/6001/7002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/6001/7002#newcode717
Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert,
pRetKey);
On 2009/09/25 15:59:44, wtc wrote:
> We should consider NSS_GetClientAuthData as a reference
> implementation of the auth cert handler.

Please replace "auth cert handler" by "client auth data
handler" throughout my comment.  Sorry about the confusion.

Here is the client auth data handler in Mozilla/Firefox:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSI...

You can study its algorithm.  Note that it does the UI
right in the client auth data handler.  We can't do that
in Chromium.

[Jaime Soriano, 2009-09-28 10:46:34]

http://codereview.chromium.org/220009/diff/6001/7003
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/220009/diff/6001/7003#newcode95
Line 95: #if defined(USE_NSS)
On 2009/09/25 16:01:07, wtc wrote:
> This test should be #if defined(OS_LINUX)
> because the issue is that we don't have a cert selection
> dialog for Linux.
I used USE_NSS because my implementation is for NSS sockets.

http://codereview.chromium.org/220009/diff/6001/7002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/6001/7002#newcode717
Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert,
pRetKey);
On 2009/09/25 15:59:44, wtc wrote:
> We should consider NSS_GetClientAuthData as a reference
> implementation of the auth cert handler.  Its limitation
> is that it returns only one cert, even if the user has
> multiple certs that meet the server's criteria:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/auth...
> 
> Note the "break" statement in that for loop.
> 
Yes, I used it as initial implementation and it's enough for our needs, but as
you say, it's not how the final implementation has to be done.

> Our auth cert handler can be based on the
> NSS_GetClientAuthData algorithm, with the following
> differences:
> The first time, it finds all the certs that meet the
> server's criteria, saves the certs, and then returns
> SECFailure to make the handshake fail.  We need to
> make sure some other code sets the ERR_SSL_CLIENT_AUTH_CERT_NEEDED error.
> This error code will cause higher-level code to
> retrieves the list of client certs from this object,
> and ask the user to select one.
> 
> Since we don't have a cert selection dialog for Linux
> yet, in resource_dispatcher_host.cc, we check
> CommandLine::ForCurrentProcess()->HasSwitch(switches::kAutoSSLClientAuth).
> If true, we pick the first cert in the list automatically.
> If false, we return NULL.
> 
I tried to set the ERR_SSL_CLIENT_AUTH_CERT_NEEDED (see the first patch set),
but it seems that ResourceDispatcherHost::OnCertificateRequested and
SSLClientSocketNSS::GetSSLCertRequestInfo were never called.
What was I doing wrong?

As I didn't manage to do it, I included the Chrome switches here instead of in
the resource dispatcher in this temporal implementation.

> If a cert is selected (automatically), the higher-level
> code will redo the handshake, specifying the chosen
> client cert for us to use.
> 
> The second time this auth cert handler is called, the
> higher-level code should have specified a client cert,
> chosen by the user, for us to use.  Then we just return
> that cert.
Ok, I'll try to do it.

[Jaime Soriano, 2009-10-01 13:55:57]

http://codereview.chromium.org/220009/diff/8004/8006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/8004/8006#newcode727
Line 727: if (cert && (privkey = PK11_FindKeyByAnyCert(cert, proto_win))) {
PK11_FindKeyByAnyCert provokes segmentation fault sometimes when refreshing a
SSL-client-authenticated site:
#0  0xb725b8aa in memcpy () from /lib/tls/i686/cmov/libc.so.6
#1  0x09eeeb58 in sqlite3VdbeMemSetStr (pMem=0xb16ff368, z=0x31617a6f <Address
0x31617a6f out of bounds>, n=101658641, enc=0 '\0', xDel=0xffffffff)
    at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbemem.c:715
#2  0x09ee9640 in bindText (pStmt=0xb1634528, i=1, zData=0x31617a6f,
nData=101658641, xDel=0xffffffff, encoding=0 '\0')
    at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbeapi.c:937
#3  0x09ee96f0 in sqlite3_bind_blob (pStmt=0xb1634528, i=1, zData=0x31617a6f,
nData=101658641, xDel=0xffffffff)
    at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbeapi.c:960
#4  0xb256685d in sdb_FindObjectsInit (sdb=0xb5023d08, template=0xb5a95370,
count=1, find=0xb5a952a8) at sdb.c:716
#5  0xb2568814 in sftkdb_FindObjectsInit (handle=0xb242caa8,
template=0xb5a95370, count=1, find=0xb5a952a8) at sftkdb.c:1219
#6  0xb25501d7 in sftk_searchDatabase (handle=0xb242caa8, search=0xb162f6e8,
pTemplate=0xb5a95370, ulCount=1) at pkcs11.c:4138
#7  0xb2552113 in NSC_FindObjectsInit (hSession=16777217, pTemplate=0xb5a95370,
ulCount=1) at pkcs11.c:4263
#8  0xb76731ae in pk11_FindObjectByTemplate (slot=0xb242d5e8,
theTemplate=0xb5a95370, tsize=1) at pk11obj.c:1477
#9  0xb765eb6a in PK11_FindObjectForCert (cert=0xb2489d98, wincx=0x0,
pSlot=0xb5a953b8) at pk11cert.c:1172
#10 0xb765ecbc in PK11_FindKeyByAnyCert (cert=0xb2489d98, wincx=0x0) at
pk11cert.c:1650
#11 0x09c774b8 in net::SSLClientSocketNSS::ClientAuthHandler (arg=0xb1625838,
socket=0xb1620998, caNames=0xb5a955b4, pRetCert=0xb16f7874, pRetKey=0xb16f7878)
    at /home/kronoss/src/chromium/src/net/socket/ssl_client_socket_nss.cc:728
...

[wtc, 2009-10-07 02:41:09]

I will try to take a look at this tomorrow.  I have been swamped
with work.  Sorry.

[Jaime Soriano, 2009-10-07 08:17:44]

On 2009/10/07 02:41:09, wtc wrote:
> I will try to take a look at this tomorrow.  I have been swamped
> with work.  Sorry.

Ok, don't worry, thanks!

[Jaime Soriano, 2009-10-07 09:58:26]

http://codereview.chromium.org/220009/diff/8004/8006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/8004/8006#newcode759
Line 759: if ( PK11_FindKeyByAnyCert(cert, proto_win) ) {
This key has to be freed

[wtc, 2009-10-10 00:13:34]

Jaime,

Finally I was able to spend some time on this changelist
this afternoon.  I tested it against https://www.openid.com/,
and it works.  https://www.openid.com/ does SSL client
authentication during a renegotiation handshake, so I needed
to add some code to support that.  I had a test server that
did SSL client authentication during a regular handshake,
but I had to reuse that server for some other testing, so
I can't test regular SSL client auth today.

I edited the description of your CL slightly.  One question:
since SSL_ClearSessionCache returns void, what do you mean
by "SSL_ClearSessionCache fails"?  Do you mean it crashes?

I suggested some changes below.  Depending on your interest
in contributing to Chromium, you are welcome to make those
changes, or I can fix them for you.  Of course, I hope you
will continue to contribute to Chromium!

Please let me know if you have questions about renegotiation
handshake.  That's what SSL_ReHandshake does, by the way.

Thanks a lot for your help.

Could you add a TODO comment about this issue?
http://code.google.com/p/chromium/issues/detail?id=13934

Although NSS doesn't have the API limitation of Schannel,
we are using NSS as if it had that limitation, so we will
also be sending a client cert before we verify the server
cert on Linux.  I will look into how to fix that.

http://codereview.chromium.org/220009/diff/8004/8009
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/220009/diff/8004/8009#newcode1060
Line 1060: #else
The #else block should include all of the original code
in this function, in particular lines 1045-1051.

http://codereview.chromium.org/220009/diff/8004/8006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/8004/8006#newcode369
Line 369: server_cert_verify_result_.Reset();
Let's also reset client_auth_cert_needed_ to false here.

http://codereview.chromium.org/220009/diff/8004/8006#newcode498
Line 498: EnterFunction("");
Let's remove the EnterFunction/LeaveFunction unless they
are useful to you.  (I don't use them.)

To match our Windows implementation in ssl_client_socket_win.cc,
ClientAuthHandler should simply
remember the caNames argument in a member variable, and
we look up the client certs (the code in lines 739-769) in
this method.

http://codereview.chromium.org/220009/diff/8004/8006#newcode707
Line 707: void * arg,
The parameters should be indented by 4 spaces.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/220009/diff/8004/8006#newcode726
Line 726: cert = that->ssl_config_.client_cert->os_cert_handle();
I think we need to acquire a reference to 'cert' with a
CERT_DupCertificate(cert) call.

http://codereview.chromium.org/220009/diff/8004/8006#newcode759
Line 759: if ( PK11_FindKeyByAnyCert(cert, proto_win) ) {
On 2009/10/07 09:58:26, Jaime Soriano wrote:
> This key has to be freed

Yes, I noticed this issue, too.

http://codereview.chromium.org/220009/diff/8004/8006#newcode792
Line 792: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) {
I believe this SSL_ReHandshake call should be removed.
I removed it, and the code still works.  Why do you need
this SSL_ReHandshake call?

http://codereview.chromium.org/220009/diff/8004/8006#newcode900
Line 900: int SSLClientSocketNSS::DoPayloadRead() {
To support a server-initiated renegotiation handshake, I
added the following code here:

 int SSLClientSocketNSS::DoPayloadRead() {
   EnterFunction(user_buf_len_);
+  if (client_auth_cert_needed_) {
+    user_buf_ = NULL;
+    LeaveFunction("");
+    return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+  }
   int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_);
   if (rv >= 0) {
     LogData(user_buf_->data(), rv);

I'm not sure if that's the best solution, but it works.

http://codereview.chromium.org/220009/diff/8004/8005
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/220009/diff/8004/8005#newcode15
Line 15: #include <keyt.h>
Please move <keyt.h> up, right below #undef Lock, and
then add a blank line before <string>.

The blank line separates the C system headers from C++
system headers.  See the example at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/220009/diff/8004/8005#newcode82
Line 82: // NSS calls this when client authentication is required.
Nit: change "required" to "requested".  It's possible for a
web server to request but not require a client cert, so
using "requested" is more accurate here.

http://codereview.chromium.org/220009/diff/8004/8005#newcode84
Line 84: PRFileDesc *socket,
The 2nd - 4th parameters should be aligned with the
first parameter.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

We should rename the function parameters following our
coding style.  Also in our C++ code we put the *
next to the type rather than the parameter/variable.  I
can fix these nits for you when I check this in.

[Jaime Soriano, 2009-10-10 14:16:03]

On 2009/10/10 00:13:34, wtc wrote:
> I edited the description of your CL slightly.  One question:
> since SSL_ClearSessionCache returns void, what do you mean
> by "SSL_ClearSessionCache fails"?  Do you mean it crashes?
> 
Yes, segmentation fault.

> I suggested some changes below.  Depending on your interest
> in contributing to Chromium, you are welcome to make those
> changes, or I can fix them for you.  Of course, I hope you
> will continue to contribute to Chromium!
> 
Sure, I can continue fixing it :)

> Could you add a TODO comment about this issue?
> http://code.google.com/p/chromium/issues/detail?id=13934
> 
Ok, I'll do it.

> Although NSS doesn't have the API limitation of Schannel,
> we are using NSS as if it had that limitation, so we will
> also be sending a client cert before we verify the server
> cert on Linux.  I will look into how to fix that.
> 

Thanks for your suggestions, I'll continue the work when I have some time, maybe
today later or on Monday.

[Jaime Soriano, 2009-10-10 14:31:50]

http://codereview.chromium.org/220009/diff/8004/8009
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/220009/diff/8004/8009#newcode1060
Line 1060: #else
On 2009/10/10 00:13:34, wtc wrote:
> The #else block should include all of the original code
> in this function, in particular lines 1045-1051.
Shouldn't we check if the client certs list is empty in both cases?

http://codereview.chromium.org/220009/diff/8004/8006
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/8004/8006#newcode726
Line 726: cert = that->ssl_config_.client_cert->os_cert_handle();
On 2009/10/10 00:13:34, wtc wrote:
> I think we need to acquire a reference to 'cert' with a
> CERT_DupCertificate(cert) call.
mmm, yes, it's possible, I debugged this a bit more, and it seems that cert
fields are empty on the next line, what very probably provokes the failures.
I'll check with your suggestion, good point.

http://codereview.chromium.org/220009/diff/8004/8006#newcode792
Line 792: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) {
On 2009/10/10 00:13:34, wtc wrote:
> I believe this SSL_ReHandshake call should be removed.
> I removed it, and the code still works.  Why do you need
> this SSL_ReHandshake call?
Without doing that, the client authentication wasn't requested on the "second
pass", anyway I'll try again.

http://codereview.chromium.org/220009/diff/8004/8006#newcode900
Line 900: int SSLClientSocketNSS::DoPayloadRead() {
On 2009/10/10 00:13:34, wtc wrote:
> To support a server-initiated renegotiation handshake, I
> added the following code here:
> 
>  int SSLClientSocketNSS::DoPayloadRead() {
>    EnterFunction(user_buf_len_);
> +  if (client_auth_cert_needed_) {
> +    user_buf_ = NULL;
> +    LeaveFunction("");
> +    return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
> +  }
>    int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_);
>    if (rv >= 0) {
>      LogData(user_buf_->data(), rv);
> 
> I'm not sure if that's the best solution, but it works.
Ok, I'll add it to the patch.

http://codereview.chromium.org/220009/diff/8004/8005
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/220009/diff/8004/8005#newcode15
Line 15: #include <keyt.h>
On 2009/10/10 00:13:34, wtc wrote:
> Please move <keyt.h> up, right below #undef Lock, and
> then add a blank line before <string>.
> 
> The blank line separates the C system headers from C++
> system headers.  See the example at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
> 
I have to take a deeper look to the guidelines :)

[Jaime Soriano, 2009-10-13 11:13:39]

I didn't manage to change the way I do the handshake renegotiation.

http://codereview.chromium.org/220009/diff/18001/19002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/220009/diff/18001/19002#newcode374
Line 374: client_auth_ca_names_  = NULL;
If client_auth_ca_names_ is freed here with CERT_FreeDistNames, something fails
later double-freeing it.

http://codereview.chromium.org/220009/diff/18001/19002#newcode801
Line 801: that->client_auth_ca_names_ = ca_names_copy;
I'm not sure if this is the correct way to duplicate a CERTDistNames.
It works, but I cannot explicitelly free it.

http://codereview.chromium.org/220009/diff/18001/19002#newcode824
Line 824: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) {
I tried to remove this, but then it didn't request the client authentication on
the second pass.
I also tried with this lines by wtc:
>  int SSLClientSocketNSS::DoPayloadRead() {
>    EnterFunction(user_buf_len_);
> +  if (client_auth_cert_needed_) {
> +    user_buf_ = NULL;
> +    LeaveFunction("");
> +    return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
> +  }
>    int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_);
>    if (rv >= 0) {

[wtc, 2009-10-14 01:56:19]

Jaime,

Thanks for the new Patch Set.  I didn't have time to review
it today.  I will try to review and test it tomorrow.

[wtc, 2009-10-15 02:25:16]

Jaime,

I have started to review your new patch set.  It has been
moved to http://codereview.chromium.org/276037.

http://codereview.chromium.org/220009/diff/8004/8009
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/220009/diff/8004/8009#newcode1060
Line 1060: #else
On 2009/10/10 14:31:50, Jaime Soriano wrote:
>
> Shouldn't we check if the client certs list is empty in both cases?

Yes, we should.  In the OS_LINUX case, we check if the
client certs list is empty in the ? : expression.

[mark4porn, 2011-08-06 21:51:54]

ncluded in the email sent (if any).