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