Re: [xmlsec] Mscrypto IS patch

[Aleksey Sanin]

Hi, Wouter!

Thanks for the patch! I have reviewed it and commited to mscrypto 
branch. I would suggest
to wait and do cvs update from the branch because I did some changes 
that can cause
conflicts for you.

Also I've fixed a few checks in the (new) xmlSecBinaryToHexString
function. This was needed to get the function work properly for me,
could special attention be given to this to see if I haven't changed the
function wrongly?
 

There are two changes and one seems wrong to me (see bellow) .

In mscrypto/bignum.h/c I've added routines that convert hex numbers to
decimal format and vice versa. I have to review these functions based
upon new info I received from a friend, however at the end there is a
possibility that perhaps they can be moved to buffer.c/h, if there is a
need for that.
I rewrote these functions all together (see bellow for reasons). I would 
appreciate if you can
take a look at the result and try it with your tests.


So after quite some merging time tonight, I made this patch a
bit in a hurry to avoid more merging, and getting too much changes at
once...
There are some places where I feel that you did mistakes with merging. 
For example, see
comments about xmlSecMSCryptoKeysStoreFindKey() function bellow.

Finally I did also take a look at the keyDuplicate function, and it
looks good, but I cannot garantuee this is the way to handle duplicating
of a cryptoprovider context when the flag fCallerFreeContext is set to
FALSE. Possibly somebody else does have a clue in this?
 

Not me :)

I have checked in the patch with some changes listed bellow. I also have 
renamed file src/mscrypto/msx509.c
to src/mscrypto/x509.c to make it the same with other crypto engines.

Aleksey

My comments to the patch:
-----------------------------------------------------------------------------------------------------------------------------------------------------------
0) renamed file src/mscrypto/msx509.c to src/mscrypto/x509.c
1) include/xmlsec/mscrypto/app.h
"extern char *xmlSecMSCryptoCertStoreName;" is made static and replaced 
with xmlSecMSCryptoAppGetCertStoreName()
function. Accessing variables in DLLs is a huge pain

2) src/buffer.c
   second change seems OK to me but the first one is not. If j == size 
after
   processing the first char then the would have problems with res[j] 
for second char:

   for(i = j = 0, iCol = 0; ((i < bufSize) && (j < size)); ++i, iCol += 
2) {
   ...

       res[j++] = xmlSecBinaryHexChar1(ch);
-       if(j >= size) {
+       if(j > size) {
           break;
       }
                                                                               

        res[j++] = xmlSecBinaryHexChar2(ch);
-       if(j >= size) {
+       if(j > size) {
           break;
       }
     }
   I have reverted the first change back.

3) src/mscrypto/app.c
I've replaced strdup/free with xmlStrdup/xmlFree.
4) src/mscrypto/app.c, xmlSecMSCryptoAppKeyLoad() function
Added error messages if key load fails and calls to xmlSecBufferFinalize 
(any initialized buffer MUST be
finalized in all possible execution  paths)

5) src/mscrypto/app.c, xmlSecMSCryptoAppKeyLoadMemory() function
I found a double code similar to one from item #3 in my email with 
subject "xmlsec-mscrypto
code review (patch #2)". I removed second copy again. Wouter, can you 
take a look and check
that I did not break something because of my stupidity, please?

6) src/mscrypto/app.c, xmlSecMSCryptoAppKeyLoadMemory() function
Fixed memory leaks with pCert and tmpcert
      
7) src/mscrypto/app.c, xmlSecMSCryptoAppKeyCertLoad() function
Missed xmlSecBufferFinalize() calls again

8) src/mscrypto/app.c, xmlSecMSCryptoAppPkcs12Load() function
Missed xmlSecBufferFinalize() calls again
9)  src/mscrypto/app.c, xmlSecMSCryptoAppKeysMngrCertLoad() function
Not sure I understand why you've removed asserts at the beggining. I've 
restored then and
added missed xmlSecBufferFinalize() calls again.

10) src/mscrypto/app.c,  xmlSecMSCryptoAppKeysMngrCertLoadMemory() function
You don't use buffer in this function, there is no need to finalize it
11) src/mscrypto/bignum.c, xmlSecMSCryptoDecToHex(), 
xmlSecMSCryptoHexToDec() and friends
Sorry, I don't understand  what you wrote in these functions. IMHO, 
there were too many code and
too many mallocs for such a simple task. I rewrote both functions. 
Please check that I did not break
break your tests.

12) src/mscrypto/bignum.c, xmlSecMSCryptoWordbaseSwap() functions
Again, I don't understand why do you need to copy a string in order to 
swap it. Rewritten.

13) src/mscrypto/keysstore.c, xmlSecMSCryptoKeysStoreFindKey() function
I have separated the cert search function,  this made the FindKey 
function readable.

14) src/mscrypto/keysstore.c,  xmlSecMSCryptoKeysStoreFindKey() function
Your patch removes creation of x509Data element for found key. I don't 
think it's right, Thus I kept it
(also see item #5 from this list).

15) Changes to files src/mscrypto/x509.c and src/mscrypto/x509vfy.c were 
not reviewed because
these files needs to be reviewed from beggining to end.







_______________________________________________
xmlsec mailing list
[EMAIL PROTECTED]
http://www.aleksey.com/mailman/listinfo/xmlsec