[xmlsec] xmlsec-mscrypto code review (patch #3)
Aleksey Sanin
aleksey@aleksey.com
Mon, 22 Sep 2003 13:29:10 -0700
This is a multi-part message in MIME format.
--------------000305020000030809070507
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
>>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
--------------000305020000030809070507
Content-Type: text/html; charset=us-ascii
Content-Transfer-Encoding: 7bit
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1">
<title></title>
</head>
<body text="#000000" bgcolor="#ffffff">
<br>
<blockquote type="cite" cite="mid001501c38143$b57dac50$0401a8c0@hert">
<blockquote type="cite">
<pre wrap="">xmlSecMSCryptoX509StoreVerify() :
...
</pre>
</blockquote>
</blockquote>
The certificate chain is a list of certificates cert_0, cert_1, cert_2,
...., cert_N<br>
where <br>
Subject(cert_k) = Issuer(cert_k+1) where k = 0, ..., N - 1<br>
The chain is valid if and only if <br>
1) each cert in the chain is good (i.e. it has valid signature on
itself, valid time <br>
restrictions not before/not after, it is not revoked by some CRL,
...);<br>
2) the root of the chain (cert_0) is trusted by application<br>
You are right and xmlsec wants to use crypto engines to create certs
chain. In typical <br>
case xmlsec has a certificate it is interested in and it just asks
crypto engine if there is a <br>
valid chain for this certificate (using trusted certs in crypto engine
and untrusted certs <br>
from the xmlsec and crypto engine). Unfortunately, MSCrypto does not
provide<br>
such a function (to be precise, as I wrote in the email, this function
is not available<br>
on Windows 9x). Instead it has a function
"CertVerifySubjectCertificateContext(cert, issuerCert)" <br>
that (as far as I can understand) simply does a check that <br>
- cert is good (see above what does this mean)<br>
- Subject(issuerCert) == Issuer(cert)<br>
This check validates that issuerCert + cert can be a part of
certificates chain but it does not<br>
validate the cert! Your code did not construct the certs chain longer
than 2 certs and it did not<br>
verify that chain's root is trusted. Which is a big security hole from
my point of view :)<br>
<br>
For example, I can sign a cert with another cert I generated. Now this
function would be OK with <br>
that but would you trust an online bank that presents such certificate?
I wouldn't because I don't <br>
know who is behind the server I am connecting to. Is it my bank server
or some kiddie hacker who <br>
knows how to run OpenSSL to generate certificates.<br>
<br>
The whole question about trust in the X509 PKI model is based on which
certs you (your <br>
application) are marked as "trusted". xmlsec library does not involved
in these descsision. <br>
It's just something your have to do yourself as a system architect.
However, xmlsec command line<br>
utility provides a way to say: I trust this certificate ("--trusted"
option). The exact meaning<br>
of this is <br>
"the cert specified with --trusted option can be root cert
cert_0 for a valid certificates chain".<br>
If you have no trusted certs then you have no valid certificates chain.<br>
<br>
Current xmlsec-mscrypto implementation maintains its own list of
trusted certs in the keys manager.<br>
As I wrote before, certs marked as "trusted" in the MS Crypto certs
store are not considered as such<br>
by xmlsec due to the MSCyrpto API restrictions. I think it's a good
idea to enable this functionality<br>
but it's a separate big task from the code review I did. My change was
to just plug in a security hole<br>
in the certificates processing. <br>
<br>
<blockquote type="cite" cite="mid001501c38143$b57dac50$0401a8c0@hert">
<blockquote type="cite">
<pre wrap="">2) xmlSecMSCryptoKeyDataX509AdoptCert():
What is the reason why you duplicate cert passed to the function? I
removed this dup call
but it might be wrong.
</pre>
</blockquote>
<pre wrap=""><!---->
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?</pre>
</blockquote>
The point is that your code did something like this:<br>
xmlSecMSCryptoKeyDataX509AdoptCert(cert) {<br>
cert2 = DuplicateCert(cert);<br>
DeleteCert(cert);<br>
/* put cert2 in the key data */<br>
}<br>
If the xmlSecMSCryptoKeyDataX509AdoptCert() caller would try to destroy
the cert it passed to <br>
this function then there would be an error anyway. Also the name of the
function (Adopt) tells the<br>
caller that if the call to this function succeeded then caller does not
own passed in object anymore.<br>
<br>
My change was to remove DuplicateCert/DeleteCert calls:<br>
<br>
xmlSecMSCryptoKeyDataX509AdoptCert(cert) {<br>
/* put cert in the key data */<br>
}<br>
<br>
Aleksey<br>
<br>
<br>
</body>
</html>
--------------000305020000030809070507--