net: fix crash when failing to import a client-side cert into NSS.

net: fix crash when failing to import a client-side cert into NSS.

(See the attached bug for details of the crash.)

I believe the crash happens here (secitem.c:240):

SECStatus
SECITEM_CopyItem(PRArenaPool *arena, SECItem *to, const SECItem *from)
{
    to->type = from->type;

This is called from sslplatf.c:87:

for (len = 0, node = CERT_LIST_HEAD(list); !CERT_LIST_END(node, list);
    len++, node = CERT_LIST_NEXT(node)) {
    // Check to see if the last cert to be sent is a self-signed cert,
    // and if so, omit it from the list of certificates. However, if
    // there is only one cert (len == 0), include the cert, as it means
    // the EE cert is self-signed.
    if (len > 0 && (len == chain->len - 1) && node->cert->isRoot) {
        chain->len = len;
        break;
    }
    SECITEM_CopyItem(arena, &chain->certs[len], &node->cert->derCert);
}

I think node->cert is NULL an that the compiler is confusing things by having
SECITEM_CopyItem on the stack.

The list of certs eventually comes from
SSLClientSocketNSS::PlatformClientAuthHandler which doesn't check for NULL
results when importing the certificates.

The reporter notes that the certificate had a duplicate issuer and serial,
which does cause NSS to reject certificates in some cases.

BUG=97355
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102943

[wtc, 2011-09-22 17:56:38]

mattm: could you also review this patch?  Thanks.

[mattm, 2011-09-22 21:31:18]

I also found another call of CERT_NewTempCertificate without checking the result
in ssl_server_socket_nss.cc:370

http://codereview.chromium.org/7995009/diff/1/net/socket/ssl_client_socket_ns...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:2338: os_error = errKCCreateChainFailed;
Do we need a break here?

[agl, 2011-09-22 22:07:09]

http://codereview.chromium.org/7995009/diff/1/net/socket/ssl_client_socket_ns...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:2338: os_error = errKCCreateChainFailed;
On 2011/09/22 21:31:19, mattm wrote:
> Do we need a break here?

I don't think so but, on the other hand, why not? Done.

[mattm, 2011-09-22 23:19:21]

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

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2174: return SECFailure;
Oh, these two cases also need to (maybe) decref the hCryptProv and free the
key_context?

[wtc, 2011-09-23 00:55:54]

Review comments on Patch Set 2:

Now that I actually looked at the CL, I realized this is
rsleevi's code.  So I've cc'ed him.

rsleevi: your review is optional.  mattm and I should be
able to review this.

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

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2165: *result_certs = CERT_NewCertList();
We should store the return value of CERT_NewCertList(),
and only save it in *result_certs when we are about to
return SECSuccess on line 2195.

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2174: return SECFailure;
On 2011/09/22 23:19:21, mattm wrote:
> Oh, these two cases also need to (maybe) decref the hCryptProv and free the
> key_context?

mattm is right.  Perhaps we should do the error cleanup in
a more systematic way, or we can build the cert list first,
and then build key_context.

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:374: return ERR_UNEXPECTED;
Let's return MapNSSError(PORT_GetError()) instead.

[agl, 2011-09-23 17:52:22]

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

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2165: *result_certs = CERT_NewCertList();
On 2011/09/23 00:55:55, wtc wrote:
> We should store the return value of CERT_NewCertList(),
> and only save it in *result_certs when we are about to
> return SECSuccess on line 2195.

Done.

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2174: return SECFailure;
On 2011/09/23 00:55:55, wtc wrote:
> mattm is right.  Perhaps we should do the error cleanup in
> a more systematic way, or we can build the cert list first,
> and then build key_context.

Indeed, the state to be cleaned up is much more complex than I first noticed.

I've reduced the scope of the key_context so that we don't have to clean it up.
I think I've also dealt with the hCryptProv but that CryptoAPI call is odd. It
appears to return a flag indicating whether the caller needs to free a
refcounted object and I believe, in our case, that's always false. But why
wouldn't it just consistently return a reference in any case?

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_nss.cc:374: return ERR_UNEXPECTED;
On 2011/09/23 00:55:55, wtc wrote:
> Let's return MapNSSError(PORT_GetError()) instead.

Done.

[wtc, 2011-09-23 19:53:34]

LGTM on Patch Set 6.

My main concern is that I've never seen the INVALID_HANDLE
macro before.  In Windows code, handles are usually initialized
to NULL or 0.  There is a INVALID_HANDLE_VALUE macro,
returned by CreateFile on failure, but I remember
INVALID_HANDLE_VALUE has the value -1.

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

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2174: return SECFailure;
On 2011/09/23 17:52:22, agl wrote:
>
> I think I've also dealt with the hCryptProv but that CryptoAPI call is odd. It
> appears to return a flag indicating whether the caller needs to free a
> refcounted object and I believe, in our case, that's always false. But why
> wouldn't it just consistently return a reference in any case? 

I don't know.  Perhaps they trade off easy of use for
performance (avoid a CryptReleaseContext call when it's
not necessary).

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2142: DWORD dwKeySpec = 0;
I haven't seen the INVALID_HANDLE macro before.  Since
HCRYPTPROV_OR_NCRYPT_KEY_HANDLE is ULONG_PTR, which is
an integer type, you can simply initialize it to 0.
At least verify INVALID_HANDLE has the value 0.

Nit: we should follow the Google C++ naming convention and
name these variables crypt_prov and key_spec.

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2149: DCHECK(must_free == FALSE);
You can use DCHECK_EQ here.  cpplint probably will suggest
this.

Can you change this to CHECK?  (I confirmed MSDN says
this is true.)  Also, it seems that we should only assert
this when the function succeeds.

Similarly, I believe we can also assert hCryptProv is
non-NULL when the function succeeds, and remove the
hCryptProv != INVALID_HANDLE test below.  rsleevi likes
very defensive programming (i.e., he doesn't trust the
OS library's documentation), so I suspect the
hCryptProv != INVALID_HANDLE test is an instance of
very defensive programming.

[Ryan Sleevi, 2011-09-23 22:33:49]

LGTM, modulo possible Mac cleanup. I'm a bit surprised that NSS would reject a
certificate that CryptoAPI/CDSA process - I always expected it would be the
inverse (http://crbug.com/91341).

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

http://codereview.chromium.org/7995009/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2174: return SECFailure;
On 2011/09/23 19:53:34, wtc wrote:
> On 2011/09/23 17:52:22, agl wrote:
> >
> > I think I've also dealt with the hCryptProv but that CryptoAPI call is odd.
It
> > appears to return a flag indicating whether the caller needs to free a
> > refcounted object and I believe, in our case, that's always false. But why
> > wouldn't it just consistently return a reference in any case? 
> 
> I don't know.  Perhaps they trade off easy of use for
> performance (avoid a CryptReleaseContext call when it's
> not necessary).

This API was originally introduced and ran on Windows 95. In it's current
implementation, it's just an InterlockedIncrement/InterlockedDecrement. However,
these primitives behave differently on 95 (
http://blogs.msdn.com/b/oldnewthing/archive/2004/05/06/127141.aspx ), so my
guess is that wtc is correct - it's a performance thing.

Now a days it hardly matters now with XADD.

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2149: DCHECK(must_free == FALSE);
On 2011/09/23 19:53:34, wtc wrote:
> You can use DCHECK_EQ here.  cpplint probably will suggest
> this.
> 
> Can you change this to CHECK?  (I confirmed MSDN says
> this is true.)  Also, it seems that we should only assert
> this when the function succeeds.
> 
> Similarly, I believe we can also assert hCryptProv is
> non-NULL when the function succeeds, and remove the
> hCryptProv != INVALID_HANDLE test below.  rsleevi likes
> very defensive programming (i.e., he doesn't trust the
> OS library's documentation), so I suspect the
> hCryptProv != INVALID_HANDLE test is an instance of
> very defensive programming.

Double checked my notes from disassembling this function when looking into
http://crbug.com/71928 and I agree, it should be fine to CHECK here. The code
responsible for updating |must_free| is solely in Windows - not in CSP control -
so I trust that. It's only when Windows passes off to CSPs to do work that the
distrust of MSDN docs comes in.

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2315: *result_certs = CERT_NewCertList();
Should the same restructuring added in the Windows implementation be added here?
Not setting |*result_certs| or |*result_private_key| until the very end (lines
296-297)

[wtc, 2011-09-23 22:52:12]

rsleevi: thanks a lot for the code review.

On 2011/09/23 22:33:49, Ryan Sleevi wrote:
> LGTM, modulo possible Mac cleanup. I'm a bit surprised that NSS would reject a
> certificate that CryptoAPI/CDSA process - I always expected it would be the
> inverse (http://crbug.com/91341).

In this case, the client certificate and the certificate of the
server that requests client auth have the same issuer and serial
number.  During an SSL handshake, Chrome first imports the server
certificate into NSS.  So when Chrome imports the client certificate
into NSS, it fails with SEC_ERROR_REUSED_ISSUER_AND_SERIAL.

[wtc, 2011-09-26 18:26:07]

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7995009/diff/11003/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2315: *result_certs = CERT_NewCertList();
On 2011/09/23 22:33:50, Ryan Sleevi wrote:
> Should the same restructuring added in the Windows implementation be added
here?
> Not setting |*result_certs| or |*result_private_key| until the very end (lines
> 296-297)

rsleevi: ("lines 296-297" is a typo).  The Mac code uses
a different strategy: set |*result_certs| and |*result_private_key|
first, and clean them up on failure (lines 2352-2359).

So the Mac code should not need more changes.

[commit-bot: I haz the power, 2011-09-26 19:38:36]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/7995009/11003

[commit-bot: I haz the power, 2011-09-26 20:01:07]

Try job failure for 7995009-11003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...