[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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.aleksey.com/pipermail/xmlsec/attachments/20030922/5f21c361/attachment.htm