Re: [xmlsec] xmlsec-mscrypto code review (patch #3)

[Aleksey Sanin]

xmlSecMSCryptoX509StoreVerify() : ... The certificate chain is a list of certificates cert_0, cert_1, cert_2, ...., cert_N where        Subject(cert_k) = Issuer(cert_k+1) where k = 0, ..., N - 1 The chain is valid if and only if     1) each cert in the chain is good (i.e. it has valid signature on itself, valid time     restrictions not before/not after, it is not revoked by some CRL, ...);     2) the root of the chain (cert_0) is trusted by application You are right and xmlsec wants to use crypto engines to create certs chain. In typical case xmlsec has a certificate it is interested in and it just asks crypto engine if there is a valid chain for this certificate (using trusted certs in crypto engine and untrusted certs from the xmlsec and crypto engine). Unfortunately, MSCrypto does not provide such a function (to be precise, as I wrote in the email, this function is not available on Windows 9x). Instead it has a function "CertVerifySubjectCertificateContext(cert, issuerCert)" that (as far as I can understand) simply does a check that     - cert is good (see above what does this mean)     - Subject(issuerCert) == Issuer(cert) This check validates that issuerCert + cert can be a part of certificates chain but it does not validate the cert! Your code did not construct the certs chain longer than 2 certs and it did not verify that chain's root is trusted. Which is a big security hole from my point of view :) For example, I can sign a cert with another cert I generated. Now this function would be OK with that but would you trust an online bank that presents such certificate? I wouldn't because I don't know who is behind the server I am connecting to. Is it my bank server or some kiddie hacker who knows how to run OpenSSL to generate certificates. The whole question about trust in the X509 PKI model is based on which certs you (your application) are marked as "trusted".  xmlsec library does not involved in these descsision. It's just something your have to do yourself as a system architect. However, xmlsec command line utility provides a way to say: I trust this certificate ("--trusted" option). The exact meaning of this is         "the cert specified with --trusted option can be root cert cert_0 for a valid certificates chain". If you have no trusted certs then you have no valid certificates chain. Current xmlsec-mscrypto implementation maintains its own list of trusted certs in the keys manager. As I wrote before, certs marked as "trusted" in the MS Crypto certs store are not considered as such by xmlsec due to the MSCyrpto API restrictions. I think it's a good idea to enable this functionality but it's a separate big task from the code review I did. My change was to just plug in a security hole in the certificates processing. 2) xmlSecMSCryptoKeyDataX509AdoptCert(): What is the reason why you duplicate cert passed to the function? I removed this dup call but it might be wrong. Hmm, I did that in any case where the cert passed with this function would be deleted in the calling context of that function, assuming that any AdiptCert/AdoptKey type function takes over the control of life and dead of that key/cert. If this is not clear, could you give me pointers in the src where this is done wrong according to your opinion? The point is that your code did something like this:        xmlSecMSCryptoKeyDataX509AdoptCert(cert) {                 cert2 = DuplicateCert(cert);                 DeleteCert(cert);                 /* put cert2 in the key data */        } If the xmlSecMSCryptoKeyDataX509AdoptCert() caller would try to destroy the cert it passed to this function then there would be an error anyway. Also the name of the function (Adopt) tells the caller that if the call to this function succeeded then caller does not own passed in object anymore. My change was to remove DuplicateCert/DeleteCert calls:        xmlSecMSCryptoKeyDataX509AdoptCert(cert) {                 /* put cert in the key data */        } Aleksey