Support for using OS-native certificates for SSL client auth....

Support for using OS-native certificates for SSL client auth.

Known Limitations
 - Only SSL3/TLS1.0 handshakes are supported. It's unlikely SSLv2 will/should ever be implemented. NSS does not yet support TLS1.1/1.2
 - On Windows, only CryptoAPI keys are supported. Keys that can only be accessed via CNG will fail.

Technical Notes:
 Windows:
  - Only the AT_KEYEXCHANGE key is used, per http://msdn.microsoft.com/en-us/library/aa387461(VS.85).aspx
  - CryptSetHashParam is used to directly set the hash value. This *should* be supported by all CSPs that are compatible with RSA/SChannel, AFAICT, but testing is needed.

 NSS:
  - The define NSS_PLATFORM_CLIENT_AUTH is used to guard all of the new/patched code. The primary implementation details are in sslplatf.c.

BUG=148
BUG=37560
BUG=45369
TEST=Attempt to authenticate with a site that requires HTTPS (eg: https://foaf.me/simpleLogin.php with a FOAF+SSL client certificate).

[rsleevi-old, 2010-06-14 11:15:33]

Two other questions I didn't mention in the description:

Preferred license for sslplatf.c
Style for sslplatf.c (which is currently NSS-like, without all the mixing of
tabs and spaces)

[wtc, 2010-06-16 01:34:27]

Ryan: thanks a lot for this CL!  I will review this CL before your
other CLs because of its importance.

It's fine to not support SSL2 handshakes.

It's fine to only support RSA signing.

We need to send the intermediate CA certs before we can enable this code.

When I researched this issue before, I found Microsoft documentation that says
we need to use CryptSetHashParam to directly set the hash value for
CryptSignHash.  This is what your code does.

sslplaf.c should use NSS's MPL/GPL/LGPL triple license and follow the NSS coding
style.  It is not necessary to use tabs in NSS code.

[rsleevi-old, 2010-06-16 05:56:06]

Now that I've got your attention, I can go forward with the soul-crushing
technical discussion :)

AT_KEYEXCHANGE: 
MSDN documentation gives conflicting statements about whether, for client
authentication, the certificate needs to be flagged as AT_KEYEXCHANGE or
AT_SIGNATURE
 - In Favor:
  * http://msdn.microsoft.com/en-us/library/aa387688(VS.85).aspx
 - Opposed:
  * http://msdn.microsoft.com/en-us/library/aa375195(VS.85).aspx - only states
/server/ keys must be AT_KEYEXCHANGE, and suggests you can store client keys in
PROV_RSA_SIGNATURE (which implies AT_SIGNATURE keys)
  *
http://blogs.msdn.com/b/alejacma/archive/2009/03/16/how-to-create-a-self-sign...
- Which reflects more or less the standard CryptoAPI way for self-signed certs,
which ends up using AT_SIGNATURE
  * http://msdn.microsoft.com/en-us/library/aa386968(VS.85).aspx - which tries
both, AT_SIGNATURE first - when generating a self-signed cert.

So should we be trying both AT_SIGNATURE and AT_KEYEXCHANGE? Recall from the
<keygen> discussion that AT_KEYEXCHANGE is implemented in terms of a bitmask
allowing all the bits of AT_SIGNATURE - that is, the key is good for both
exchange and signing, while AT_SIGNATURE is signing only. 

Certificate Chains:
Is it acceptable to simply import the certificates into NSS, and let NSS chain
building do the heavy lifting, or would you prefer it be explicitly what the OS
provided? The NSS DB /should/ only contain the certificates we've imported, so
it should result in the same chain as was supplied. My only concern would be if
they were running a long session, used a federated identity (on Windows, at
least, on OS X it's already game-over for cross-certification), and both
intermediates ended up in the NSS DB. It'd no longer be "as smart" as the
Windows in chain selection.

The reason for the question is that the fundamental type on the socket itself is
a CERTCertificateList, which is part of the certdb library and has limited
creation opportunities (see mozilla/security/lib/certdb/cert.h).
CERT_CertChainFromCert (which uses the NSS default cert DB),
CERT_CertListFromCert, and CERT_DupCertList. No mutators, no arbitrary creators.
Compare this to CERTCertList (which is currently what I'm using in my modified
API), which is essentially a doubly-linked list that can be freely modified via
public API. The upside is that by using CERTCertList, it also loads the certs
into the NSS DB (since the only acceptable way to create a CERTCertificate is
via CERT_NewTempCertificate, per dev.tech.crypto, which is the same API call
CERT_ImportCerts would use to bring them in). Any certificates loaded via
CERT_NewTempCertificate are thus accessible to CERT_CertChainFromCert, which
makes them available for sending to the server (and makes the job of supporting
chains a small modification to ssl_client_socket_nss.cc, with the NSS work
already being implemented).

The alternative would be to take the CERTCertList, and internal-to-NSS convert
it to a CERTCertificateList and associate the result with the socket. The result
would thus preserve the explicit ordering of the certificates, and the downside
of not checking if the chain is actually legitimate. The problem I have with
that is that it relies upon internal knowledge of how certdb is implemented,
which could chain (since libssl and libcertdb are technically separate layers,
AFAICT). The current implementation of CERT_DestroyCertificateList simply
destroys the arena, which frees all the items (and itself)
(nss/lib/certdb/certhigh.c). I can easily implement an arena allocator that will
convert CERTCertList -> CERTCertificateList, but that's the coupling I'm talking
about. If you don't see any risk/harm (since it'd only be on Mac/Windows, thus
always coming from third_party/nss/mozilla rather than system), then I can go
that way, I just wanted to run it by you first.



CryptSetHashParam:
While any PROV_RSA_SCHANNEL provider /must/ implement CALG_SSL3_SHAMD5 and
/must/ support CryptSetHashParam (per
http://msdn.microsoft.com/en-us/library/aa379865(VS.85).aspx ), I'm not aware of
any requirements for PROV_DSA_SCHANNEL to support the same for CALG_SHA1 (thus,
by proxy, ECC certs, if/when implemented, or CALG_SHA256, if/when TLS 1.1/1.2).
So that's the issue there, and why it may require further changes if it becomes
an issue, as the documentation on CryptSetHashParam is clear that not all CSPs
will allow those operations. However, I believe any such changes can remain
localized to libssl.



Misc:
During the implementation of this CL, I ended up noticing several bugs in the
code that may affect your comments on the above, specifically the chain
handling. I wasn't sure if I should file bugs before this lands, or wait until
after, as I was pretty sure addressing them inside here would be too much code
to review.
 - There's an issue in Windows with the SSL Client Auth Handler interacting with
the X509Certificate cache, which can cause an unnecessary DCHECK in debug, leak
in production.
 - The Windows code doesn't actually check that you're in possession of the
private key (and that it's accessible) until it's passed all the way down to the
NSS layer. A certificate's presence in the MY store is no guarantee the key is
actually available (eg: removed from smart card on another machine). 
 - There are several issues on the OS X side related to chain building, due to
how the Apple X.509 TP is implemented, which means client certs affected by CA
rollover (which would primarily be an issue with internal, organizational
enterprise CAs) can result in invalid chains being supplied. 
 - There's the sorta-known issue (Jens and I discussed on an issue that you were
reviewing) on OS X related to the IsIssuedBy() function being overly generous in
it's matching algorithms, which can result in allowing certs that shouldn't be
allowed in.

Basically, the OS X side, we'd be better served letting NSS validate the chain
and do issuer comparisons, while on the Windows side, it's no question that the
chain building algorithm is much more robust than NSS's. On both, we're being
over-generous in what we allow the user to select.

One last thing I want to highlight, as if I haven't mentioned enough already, is
sslplatf.c:365-368. The current implementation on both Windows and Mac /can/
block by causing a UI element to show. This is identical to Issue #16979 on the
Windows side, and on the Mac side, the same behaviour exists but I didn't see
any bug for it. The only safe way I see to deal with this, in a platform
agnostic manner, is to move handshaking off onto it's own thread, and move it
back to the IO when complete. The Windows code that causes the UI element to
appear is the actual signing operation, which the user can configure the key to
prompt them every time before the key is used. On Mac, it's the
SecKeyGetCredentials call, which AFAIK, can be configured the same way. Neither
provide knowledge beforehand that the call may prompt (CRYPT_SILENT+NTE_SILENT
isn't enough on Win32 to detect prompt conditions), so there's no real trick you
can do. Given that the existing code has this problem, it seems appropriate to
"not deal with here" - I just wanted to make sure that I brought attention to an
IO thread blocker.

[rsleevi-old, 2010-06-16 08:44:44]

This patchset adds support for sending the certificate chain as returned by the
OS. This means it also includes the root certificate - which is legal under the
TLS RFC (eg: RFC 2246, Section 7.4.2. - top of page 38), but generally not
common (and not the default behaviour in the OS implementations, from what I can
tell). The main reason not to send the root would be to reduce network bandwidth
(under the assumption the server has the cert, if they're advertising that
they'll receive it).

Simply chopping the last cert off is not advisable, since the client may not
have the full chain (eg: server advertises C, they have A and B of the chain
A->B->C. They have enough information to know B->C, but they don't have C)

http://codereview.chromium.org/2828002/diff/30002/36001
File chrome/browser/ssl/ssl_client_auth_handler_win.cc (left):

http://codereview.chromium.org/2828002/diff/30002/36001#oldcode50
chrome/browser/ssl/ssl_client_auth_handler_win.cc:50:
net::X509Certificate::OSCertHandles());
The bug here is that cert_context refers to a cert in the client_certs temporary
store. If an actual certificate were created with this context, then line 53
would fail (because a cert is still open), and client_certs would leak (because
the store close wasn't deferred, but instead force-checked)

This brings the Windows implementation in line with the OS X implementation, in
that you're guaranteed to get back the /exact/ same X509Certificate* you
supplied in as a candidate.

The main reason for this change was not to fix the leak, but to make sure that
the certificate intermediates were persisted from chain building (back in
SSLClientSocketNSS) through the UI and back. This is accomplished by ensuring
the same X509Certificate* throughout.

[rsleevi-old, 2010-06-17 06:27:18]

Patchset 3 should contain the last of the must-implement TODOs, by adding
detection of the smart card being ejected. While I'd still like to investigate
the ECC support, I haven't had a chance to sit down and read through RFC 4492 in
depth yet, to see if it's just a matter of SHA-1 (as with DSA), or if there are
other aspects.

Looking at libsecurity_keychain, SecKeyRef refers to KeyItem, which derives from
ItemImpl, which is what a SecKeychainItemRef refers to. Further, digging through
the apple-cdsa archives and the documentation supports the unconditional
up-cast, so I feel safe doing it. This hierarchy allows the associated keychain
and a persistent reference to be pulled out from the SecKeyRef, which can be
used to make sure that both the keychain and the key are still present
throughout the SSL/TLS session (as the key itself is not used again unless
renegotiating)

The downside to this implementation, on both Windows and Mac, is that it ends up
hitting the OS APIs pretty hard, as every block encrypted or decrypted will end
up going through these functions. 

On the OS X side, one could switch from level-triggered to edge-triggered by
setting up a SecKeychainCallback to watch the keychain and see if the SecKeyRef
is deleted, and pass that down to NSS to abort the handshake. The problem is,
for Windows, the KSP management is handled by the OS in the protected area (see
http://technet.microsoft.com/en-us/library/ff404288(WS.10).aspx ), so short of
re-implementing that, I don't see much efficiency gain.

[Ryan Sleevi, 2010-09-02 04:26:43]

Wan-Teh,

   Have you had a chance to look at this? Seeing that you moved native auth to
Pri-1/Milestone 8, I wasn't sure where you stood on this. Wrong design for the
integration? Too coupled to just the TLS client handshake? You'd rather it as a
PKCS#11 module?

[wtc, 2010-09-02 23:34:56]

Ryan,

This fell through the cracks.  I'm very sorry.  Too busy!
I am very interested in this CL.  I'll try to review it soon.

[wtc, 2010-09-23 23:14:26]

rsleevi: I have applied your patch to a current source tree.
I will upload an updated version soon.

http://codereview.chromium.org/2828002/diff/56003/34003
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2828002/diff/56003/34003#newcode1219
net/socket/ssl_client_socket_nss.cc:1219: CERTCertificate* user_cert =
CERT_NewTempCertificate(
Rather than importing the cert into NSS, I think we should
just pass the platform cert handle to NSS, just like we pass
the HCRYPTPROV to NSS.

[Ryan Sleevi, 2010-09-23 23:25:31]

On 2010/09/23 23:14:26, wtc wrote:
> rsleevi: I have applied your patch to a current source tree.
> I will upload an updated version soon.
> 
> http://codereview.chromium.org/2828002/diff/56003/34003
> File net/socket/ssl_client_socket_nss.cc (right):
> 
> http://codereview.chromium.org/2828002/diff/56003/34003#newcode1219
> net/socket/ssl_client_socket_nss.cc:1219: CERTCertificate* user_cert =
> CERT_NewTempCertificate(
> Rather than importing the cert into NSS, I think we should
> just pass the platform cert handle to NSS, just like we pass
> the HCRYPTPROV to NSS.

The conversion was due to the fact that NSS will want access to the certificate
in it's parsed form, so it can make decisions about how to procede with the TLS
handshake (for example, is this a DSA, RSA, or ECC certificate) as well as be
able to send it to the remote party as part of the handshake message. Sending in
the native cert type means NSS has to convert it from native -> internal as it
comes in, or be exposed at even more levels to the native certificate type.
Since we already had the abstraction to get the raw bytes, and NSS already had
the abstraction to turn raw bytes into a certificate, I reused those, rather
than having to duplicate the calls in NSS. 

However, if it's something that may be upstreamed (it seems unlikely, this feels
more like HACK but appropriate), then I'd agree, it'd make more sense if
caller's could just pass in the native type and then discard it once the API
returns, with NSS owning it's internal representation.

[wtc, 2010-09-23 23:39:07]

rsleevi: "gcl update" doesn't allow me to upload a new snapshot
of a CL owned by you.  So I had to create a new CL:
http://codereview.chromium.org/3455019/show

We'll need to ping pong between the two CLs as we do the
code review.  Sorry about the inconvenience.

[Ryan Sleevi, 2010-09-24 02:32:08]

Hopefully I'm doing this right. I used git cl patch to grab your patch and apply
to trunk, and then changed the issue with git cl issue back to this.

http://codereview.chromium.org/2828002/diff/82001/83010
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/2828002/diff/82001/83010#newcode140
net/third_party/nss/ssl/sslplatf.c:140: info->type, CRYPT_SILENT))
In general, I dislike having to call Acquire/Release for every SSL read/write to
make sure the card hasn't been ejected. Unfortunately, I'm not sure of a better
way that isn't constantly interacting with the HKEYPROV. 

The SChannel implementation is special for two reasons. The first is that the
RSA signing operation runs within the context of a privileged process (see
http://msdn.microsoft.com/en-us/library/aa379882(VS.85).aspx ), so the user is
never exposed to the key/card. The second is that the bulk encryption/decryption
keys are then generated by the same CSP backing the client's private key. As
such, ejection events are able to be handled by the CSP (which is already likely
interacting with the smart card services API to interact with the underlying
card), which can then invalidate the bulk encrypt/decrypt keys once it detects
the client card is ejected.

As such, the SChannel implementation is assumed to be much more efficient. The
APIs for determining what card a key lives on (so then you can interact w/ the
smart card services to receive notification of it's ejection) aren't available
until Vista+, and even still, there is no guarantee that the CSP will expose
them (especially with the transition to CNG)

http://codereview.chromium.org/2828002/diff/82001/83010#newcode350
net/third_party/nss/ssl/sslplatf.c:350: OSStatus rv =
SecKeychainGetStatus(info->keychain, &keychainStatus);
Similar to the change above CRYPT_SILENT above, I wonder if
SecKeychainSetUserInteractionAllowed should be called here. The difference
between Secure Transport and SChannel is that with SChannel, the caller (us) can
obtain a handle and determine whether or not we want prompting to happen
ourselves, before passing in. With Secure Transport, we don't really have that
flexibility at a per-key level, only at a "Keychain Services" wide level.

What concerns me is the "Special Considerations" section under
SecKeychainSetUserInteractionAllowed - "If you disable user interaction before
calling a Keychain Services function, be sure to reenable it when you are
finished. Failure to reenable user interaction will affect other clients of the
Keychain Services". To me, this implies that it may be system wide, or at the
minimum, process wide. 

I don't have a Smart Card that works with the Mini to be able to test what
happens during a surprise ejection or lock/unlock of the card that would cause a
PIN dialog.

My primary concern is that, whichever platform, it's preferable that they both
behave the same with regard to prompting, and I'm not sure a good way to do that
here.

http://codereview.chromium.org/2828002/diff/82001/83010#newcode455
net/third_party/nss/ssl/sslplatf.c:455: status = SecKeyGetCredentials(*key,
CSSM_ACL_AUTHORIZATION_SIGN,
This may prompt separately of the Keychain status prompting behaviour above.

[wtc, 2010-11-03 23:30:05]

rsleevi: I'm getting ready to check in this patch (using
my copy of the CL in 3455019).  I've reviewed the diffs
between your Patch Sets 9 and 11.  I encountered the same
problems on Mac last night and fixed them.  I will switch
to your fix for the double definition of SSL_NULL_WITH_NULL_NULL
later.  (I fixed it by changing ssl3ext.c to include
"sslimpl.h" before "sslproto.h".)

http://codereview.chromium.org/2828002/diff/95014/119010
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/2828002/diff/95014/119010#newcode144
net/third_party/nss/ssl/sslplatf.c:144: info->provType, CRYPT_SILENT))
I have a password-protected private key in the Windows CAPI
system key store.  (The password is empty, but I have to
approve the use of the key by each application.)

I found that CRYPT_SILENT causes this CryptAcquireContextA
call to fail with the 0x80090022 error, even though I've
approved the use of the key earlier in CryptSignHash (so
that this CryptAcquireContextA call doesn't need to get my
approval again).  It seems that CRYPT_SILENT causes
CryptAcquireContextA to give up without checking whether
the app already has the approval to use the key.

[rsleevi-old, 2010-11-03 23:41:09]

http://codereview.chromium.org/2828002/diff/95014/119010
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/2828002/diff/95014/119010#newcode144
net/third_party/nss/ssl/sslplatf.c:144: info->provType, CRYPT_SILENT))
On 2010/11/03 23:30:06, wtc wrote:
> I have a password-protected private key in the Windows CAPI
> system key store.  (The password is empty, but I have to
> approve the use of the key by each application.)
> 
> I found that CRYPT_SILENT causes this CryptAcquireContextA
> call to fail with the 0x80090022 error, even though I've
> approved the use of the key earlier in CryptSignHash (so
> that this CryptAcquireContextA call doesn't need to get my
> approval again).  It seems that CRYPT_SILENT causes
> CryptAcquireContextA to give up without checking whether
> the app already has the approval to use the key.

Just to make sure I'm testing correctly, you're talking about a private key that
was imported at Medium (Prompt me to use this key) or High (Prompt me to use
this key with a password), correct?

[Ryan Sleevi, 2010-11-04 02:05:24]

@wtc:

I was able to reproduce using medium/high security with a locally imported key.
Unfortunately, I don't have a good solution for this. The problem is that the
RSA Enhanced Provider, used to store keys on the file system, will always reject
CRYPT_SILENT in CryptAcquireContext if the key was marked CRYPT_USER_PROTECTED
in CryptGenKey - rather than waiting for something like CryptSignHash before
checking the silent flag.

This differs in behaviour from the Microsoft Base CSP - the CSP used for smart
cards - which does not attempt a UI until an operation requires it.
Unfortunately, MSDN documentation of what "an operation requires it" means is
fairly sparse. The best description of the implementation (short of CAPI
tracing) is available in the patent Microsoft obtained for their Base CSP -
Patent #7200756 and at
http://technet.microsoft.com/en-us/library/ff404300(WS.10).aspx .

The problem is that, without CRYPT_SILENT, if the card has been ejected, then
the Base CSP will prompt the user to re-insert their card, via
SCardUIDlgSelectCard, and block until they do. The problem is this would block
the IO thread on UI, until the user re-inserted their card. This risk already
exists if the user ejects their card between selecting the certificate and NSS
receiving the callback and getting to the CryptSignHash, but it just exacerbates
it.

There is unfortunately no good way to know what the CSP will do. Further, the
RSAENH CSP does not permit PP_USER_PIN as part of CryptSetProvParam, so you
can't implement your own prompt for the PIN and then use CRYPT_SILENT with the
context. The patchset I just uploaded is the closest I could get to
distinguishing between the CSPs, but really needs to be tested with more CSPs.

If the CSP does not report the key/provider as removable, then the application
checks will be skipped. The RSA Enhanced provider does not, as far as I've
tested, report it's implementation as removable - it reports it as
CRYPT_IMPL_SOFTWARE.

I believe this should be sufficient for distinguishing the MSFT servers, but I'm
a bit uncertain with the others. I've not had to deal with this particular
problem in my other work.

The alternative, which I believe you've explored, is to remove the checks
altogether as to whether the card is still present. The *existing* SSL/TLS
session will remain valid, but no new sessions should be possible. From a FIPS
standpoint, this may not be acceptable, but it does simplify the implementation.

[wtc, 2010-11-04 20:48:35]

rsleevi: thanks for the detailed reply.  CAPI CSPs behave
very differently from PKCS #11 libraries in this regard.
NSS's libSSL is written to the PKCS #11 behavior, so it may
be suboptimal for CAPI CSPs.  In addition, we'll need to move
SSL handshake off the IO thread because of potential blocking
issues like this.

http://codereview.chromium.org/2828002/diff/95014/119010
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/2828002/diff/95014/119010#newcode144
net/third_party/nss/ssl/sslplatf.c:144: info->provType, CRYPT_SILENT))
On 2010/11/03 23:41:09, rsleevi-old wrote:
>
> Just to make sure I'm testing correctly, you're talking about a private key
that
> was imported at Medium (Prompt me to use this key) or High (Prompt me to use
> this key with a password), correct?

Right.  I forgot which level I chose.  Most likely Medium,
as I don't need to enter a password.

[wtc, 2010-11-11 23:02:54]

On 2010/11/04 02:05:24, rsleevi wrote:
>
> The alternative, which I believe you've explored, is to remove the checks
> altogether as to whether the card is still present. The *existing* SSL/TLS
> session will remain valid, but no new sessions should be possible. From a FIPS
> standpoint, this may not be acceptable, but it does simplify the
implementation.

I didn't look into what ssl_PlatformAuthTokenPresent is
used for, so I didn't consider this alternative.

I just studied how NSS uses ssl3_ClientAuthTokenPresent,
and found that it actually tests more than whether the client
auth token is present; it also tests whether the token was
ever removed since it was used for client auth:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

So the current ssl_PlatformAuthTokenPresent implementation
does not fully meet the requirement.

CVS history reminded me why ssl3_ClientAuthTokenPresent was added:
https://bugzilla.mozilla.org/show_bug.cgi?id=167756

It had nothing to do with FIPS.  It was added to implement
a useful behavior, where a client-authenticated SSL connection
will be broken if NSS detects that the token has been removed.

If we don't care about this behavior, we can define
ssl_PlatformAuthTokenPresent to simply return PR_TRUE.