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

[Aleksey Sanin]

Attached (and commited) is the last patch for src/mscrtypto/x509.c and
src/mscrtypto/x509vfy.c. I feel that I would need to do one more pass in
a week or so because there were too many changes on the first pass.
Meantime, I am going to think about hex<->dec conversion code.

Wouter, I would appreciate if you can take a look at my comments for
all 3 patches.  There are few things that need clarification (see item 2)
bellow as an example).

And thanks for all your work! I can confirm that writing good code with
MS Crypto API is a hard job!

Aleksey


My comments
----------------------------------------------------------------------------------------------------------------------------------------------------------- 

1)  Fixed the problem with "check" in Windows Makefile and relative 
paths in some tests
(raw x509 cert test, for example)

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).
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

   - 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.

3) xmlSecMSCryptoKeyDataX509GetCert():
ctx->hMemCert might not be intialized. I changed code to always create 
this certs store
in the initialization function.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: mscrypto3.diff.gz
Type: application/gzip
Size: 8059 bytes
Desc: not available
Url : http://www.aleksey.com/pipermail/xmlsec/attachments/20030921/c2a9a382/mscrypto3.diff.bin