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

[Wouter]

> 
> xmlSecMSCryptoX509StoreVerify() :
> The function did check only one thing: there is a certificate that is 
> signed by another
> one in the chain. IMHO, it's just a *BIG* security hole 
> because the code 
> did not check
> that cert is actually trusted (not ention that chains with 
> more than two 
> certs should be
> supported).

Hmm, how odd, since I did write code to do additional checks to see if
the cert was trusted. Could you tell me what needs to be done to have
the certificate trusted in your opinion (see also another comment below
from me).

> I re-wrote the code and now it does right thing. However, it 
> still has a 
> couple problems:
>    - Currently the only "trusted" certs are ones loaded to xmlsec 
> directly (for
>    example, using xmlsec command line utility "--trusted" 
> option). This 
> means that
>    code does not accept trusted certs in MS Crypto store as 
> such. There 
> are some functions
>    that allow to check against trusted certs in MS Crypto store. But 
> MSDN says that
>    these functions are not available in Windows 95/98/Me and 
> partially 
> not available
>    in NT 4.0 and Windows 2000. Anyone interested in more details feel 
> free to search
>    for "Certificate Chain Verification Functions" article in 
> MSDN. For 
> me, it sounds
>    like it would be possible to use these new functions but I 
> think it 
> would be good
>    to have a runtime version check:
>        - if it's old Windows then use current code;
>        - if it's new Windows with new functions then use some 
> new code.
>    If you are interested in this functionality, feel free to 
> contribute

I did see that too, however I felt there were a few angles in merging
these functionalities with the xmlsec library, since I had a feeling it
was hard to control the trust chain creation and validation (at least
it's consistent with the rest of MS Crypto API ;)
However I did not try this out in the real world, so it might very well
be that there are possibilities here. Also because the exact
functionalty the function must proivide is not entirely clear to me (see
next remark).

What also is not clear for me exactly when xmlsec lib regards a
certificate to be trusted. Does xmlsec relies on third party libraries
woth their cert stores for this, or are there rules to be followed here?

>    - Curerntly the cert chain construction/verification function is 
> recursive. It's
>    not too difficult to use a loop instead but I don't think this is 
> important too
>    much. If you do then feel free to submit a patch
>  
> 
> 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?

Wouter