_Crypt_DecryptData With Blank string?

[Jos]

Superiority, my ass:

Big Ass issues?

[Melba23]

jchd,

 

you fly well above us most of the time!

I certainly did today. Although I paid for it in sweat - a Nomex suit with full length underwear plus a drab green helmet and leather gloves makes for a really hot boy when placed under a nice perspex greenhouse in near 30C temperatures. But it was worth it in spades for an hour on my own in the wild blue yonder.

M23

[jchd]

Please stop this torture!

[guinness]

guinness,

Why not? It solves the proximate problem immediately and is so simple that it will not be difficult to remove when the actual problem is resolved. Personally I have no problem either way - I do not use RC4 encryption - but I wonder why you seem so against it.

M23

The reason I said it was because _Crypt_EncryptData() returns a binary string and not a string, but I see jchd provided the appropriate solution for oth functions which I wasn't able to do at the time of posting.

[Melba23]

guinness,

Understood. I will take a look at the UDF tomorrow and post a suggested change here.

M23

[guinness]

This fixes it.

If $vData = "" Then $vData = Null ; Add above the first DllCall() in _Crypt_EncryptData()

[jchd]

Sorry but I don't see a fix here: *_Encrypt fails for CR4 this way as well.

Worse it makes other CALG* fail, which where OK before.

Edited July 24, 2014 by jchd

[guinness]

Need to revise my testing methinks.

[guinness]

I am seeing no issue...or am I missing something?

expandcollapse popup

#include <Crypt.au3>
#include <MsgBoxConstants.au3>

Example()

Func Example()
    Local Const $sUserKey = "CryptPassword" ; Declare a password string to decrypt/encrypt the data.
    Local $sData = "Original" ; Data that will be encrypted.

    Local $aAlgs = [$CALG_3DES, $CALG_AES_128, $CALG_AES_192, $CALG_AES_256, $CALG_DES, $CALG_RC2, $CALG_RC4], _
            $bEncrypted = 0

    For $iAlg In $aAlgs
        $bEncrypted = _Crypt_DecryptDataEx(_Crypt_EncryptDataEx($sData, $sUserKey, $iAlg), $sUserKey, $iAlg) ; Decrypt the data using the generic password string. The return value is a binary string.
        ConsoleWrite(BinaryToString($bEncrypted) & @CRLF) ; Convert the binary string using BinaryToString to display the initial data we encrypted.
    Next
EndFunc   ;==>Example

Func _Crypt_DecryptDataEx($vData, $vCryptKey, $iALG_ID, $bFinal = True)
    Local $aRet = 0, _
            $hBuff = 0, $hTempStruct = 0, _
            $iError = 0, $iExtended = 0, $iPlainTextSize = 0, _
            $vReturn = 0

    _Crypt_Startup()

    Do
        If $iALG_ID <> $CALG_USERKEY Then
            $vCryptKey = _Crypt_DeriveKey($vCryptKey, $iALG_ID)
            If @error Then
                $iError = @error + 100
                $iExtended = @extended
                $vReturn = -1
                ExitLoop
            EndIf
        EndIf

        $hBuff = DllStructCreate("byte[" & BinaryLen($vData) + 1000 & "]")
        DllStructSetData($hBuff, 1, $vData)
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptDecrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "struct*", $hBuff, "dword*", BinaryLen($vData))
        If @error Or Not $aRet[0] Then
            $iError = @error + 20
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf

        $iPlainTextSize = $aRet[6]
        $hTempStruct = DllStructCreate("byte[" & $iPlainTextSize & "]", DllStructGetPtr($hBuff))
        $vReturn = DllStructGetData($hTempStruct, 1)
    Until True

    If $iALG_ID <> $CALG_USERKEY Then _Crypt_DestroyKey($vCryptKey)
    _Crypt_Shutdown()

    Return SetError($iError, $iExtended, $vReturn)
EndFunc   ;==>_Crypt_DecryptDataEx


Func _Crypt_EncryptDataEx($vData, $vCryptKey, $iALG_ID, $bFinal = True)
    Local $iReqBuffSize = 0, _
            $aRet = 0, _
            $hBuff = 0, _
            $iError = 0, $iExtended = 0, _
            $vReturn = 0

    _Crypt_Startup()

    Do
        If $iALG_ID <> $CALG_USERKEY Then
            $vCryptKey = _Crypt_DeriveKey($vCryptKey, $iALG_ID)
            If @error Then
                $iError = @error + 100
                $iExtended = @extended
                $vReturn = -1
                ExitLoop
            EndIf
        EndIf

        If $vData = "" Then $vData = Null
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptEncrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "ptr", 0, _
                "dword*", BinaryLen($vData), "dword", 0)
        If @error Or Not $aRet[0] Then
            $iError = @error + 20
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf

        $iReqBuffSize = $aRet[6]
        $hBuff = DllStructCreate("byte[" & $iReqBuffSize & "]")
        DllStructSetData($hBuff, 1, $vData)
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptEncrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "struct*", $hBuff, _
                "dword*", BinaryLen($vData), "dword", DllStructGetSize($hBuff))
        If @error Or Not $aRet[0] Then
            $iError = @error + 30
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf
        $vReturn = DllStructGetData($hBuff, 1)
    Until True

    If $iALG_ID <> $CALG_USERKEY Then _Crypt_DestroyKey($vCryptKey)
    _Crypt_Shutdown()

    Return SetError($iError, $iExtended, $vReturn)
EndFunc   ;==>_Crypt_EncryptDataEx

Edited July 24, 2014 by guinness

[jchd]

Try encrypting an empty string, which is the issue we have.

[guinness]

I am still not seeing the issue.

 

expandcollapse popup

#include <Crypt.au3>
#include <MsgBoxConstants.au3>

Example()

Func Example()
    Local Const $sUserKey = "CryptPassword" ; Declare a password string to decrypt/encrypt the data.
    Local $sData = "" ; Data that will be encrypted.

    Local $aAlgs = [$CALG_3DES, $CALG_AES_128, $CALG_AES_192, $CALG_AES_256, $CALG_DES, $CALG_RC2, $CALG_RC4], _
            $dDecrypted = 0, $dEncrypted = 0

    For $iAlg In $aAlgs
        $dDecrypted = _Crypt_EncryptDataEx($sData, $sUserKey, $iAlg)
        ConsoleWrite($dDecrypted & @CRLF)
        $dEncrypted = _Crypt_DecryptDataEx($dDecrypted, $sUserKey, $iAlg) ; Decrypt the data using the generic password string. The return value is a binary string.
        ConsoleWrite((BinaryToString($dEncrypted) = "") & @CRLF) ; Convert the binary string using BinaryToString to display the initial data we encrypted.
    Next
EndFunc   ;==>Example

Func _Crypt_DecryptDataEx($vData, $vCryptKey, $iALG_ID, $bFinal = True)
    Local $aRet = 0, _
            $hBuff = 0, $hTempStruct = 0, _
            $iError = 0, $iExtended = 0, $iPlainTextSize = 0, _
            $vReturn = 0

    _Crypt_Startup()

    Do
        If $iALG_ID <> $CALG_USERKEY Then
            $vCryptKey = _Crypt_DeriveKey($vCryptKey, $iALG_ID)
            If @error Then
                $iError = @error + 100
                $iExtended = @extended
                $vReturn = -1
                ExitLoop
            EndIf
        EndIf

        $hBuff = DllStructCreate("byte[" & BinaryLen($vData) + 1000 & "]")
        DllStructSetData($hBuff, 1, $vData)
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptDecrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "struct*", $hBuff, "dword*", BinaryLen($vData))
        If @error Or Not $aRet[0] Then
            $iError = @error + 20
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf

        $iPlainTextSize = $aRet[6]
        $hTempStruct = DllStructCreate("byte[" & $iPlainTextSize & "]", DllStructGetPtr($hBuff))
        $vReturn = DllStructGetData($hTempStruct, 1)
    Until True

    If $iALG_ID <> $CALG_USERKEY Then _Crypt_DestroyKey($vCryptKey)
    _Crypt_Shutdown()

    Return SetError($iError, $iExtended, $vReturn)
EndFunc   ;==>_Crypt_DecryptDataEx


Func _Crypt_EncryptDataEx($vData, $vCryptKey, $iALG_ID, $bFinal = True)
    Local $iReqBuffSize = 0, _
            $aRet = 0, _
            $hBuff = 0, _
            $iError = 0, $iExtended = 0, _
            $vReturn = 0

    _Crypt_Startup()

    Do
        If $iALG_ID <> $CALG_USERKEY Then
            $vCryptKey = _Crypt_DeriveKey($vCryptKey, $iALG_ID)
            If @error Then
                $iError = @error + 100
                $iExtended = @extended
                $vReturn = -1
                ExitLoop
            EndIf
        EndIf

        If $vData = "" Then $vData = Null
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptEncrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "ptr", 0, _
                "dword*", BinaryLen($vData), "dword", 0)
        If @error Or Not $aRet[0] Then
            $iError = @error + 20
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf

        $iReqBuffSize = $aRet[6]
        $hBuff = DllStructCreate("byte[" & $iReqBuffSize & "]")
        DllStructSetData($hBuff, 1, $vData)
        $aRet = DllCall(__Crypt_DllHandle(), "bool", "CryptEncrypt", "handle", $vCryptKey, "handle", 0, "bool", $bFinal, "dword", 0, "struct*", $hBuff, _
                "dword*", BinaryLen($vData), "dword", DllStructGetSize($hBuff))
        If @error Or Not $aRet[0] Then
            $iError = @error + 30
            $iExtended = @extended
            $vReturn = -1
            ExitLoop
        EndIf
        $vReturn = DllStructGetData($hBuff, 1)
    Until True

    If $iALG_ID <> $CALG_USERKEY Then _Crypt_DestroyKey($vCryptKey)
    _Crypt_Shutdown()

    Return SetError($iError, $iExtended, $vReturn)
EndFunc   ;==>_Crypt_EncryptDataEx

[jchd]

I now get it: I didn't place your fix at the place you said: I placed it at top (before the first call!).

Anyway there is something that makes me seriously question the new behavior: there are now two distinct binary plaintexts which have the same ciphertext and I see that a problem even if I agree that my rebutal may seem merely puristic at first (but it isn't). An empty string and an Int with value 0 (or any equivalent at binary level).

An empty string has length 0 and an Int has length 4 or 8 depending on x86/x64. From both a purely security and practical points of view this is inacceptable.

To see that, compare the ciphertext of an empty string and an Int = 0 thru any block cipher before and after the patch: they are distinct. If that data is a partial input followed by more input blocks, then the following results will be completely different from what it was before the patch. A cipher doesn't always process strings.

Not that I'm against your views, I'm trying to get it fixed in the best way. I still believe that testing head first in both functions for RC4 and BinaryLen = 0 then bypassing the whole functions by returning Binary('') is the cleanest way without unexpected side-effect I can see (or something equivalent).

[guinness]

I respect your knowledge about this subject too. If I understood your comments, what you're saying about the fix is using an int of 0 will cause the decrypted result not to be 0, which is the results I was seeing. So I figured since the decrypted result is converted back to a string and not the original datatype e.g. int32, then why no parse as a string before passing to the appropriate Win32 API call. Let me know if I missed the point. Thanks.

Replace with this...

$vData = String($vData)
If $vData = "" Then $vData = Null

[jchd]

Give me another chance to make things clearer. Let me begin with general considerations.

First, any encryption engine/method only processes binary data. It's the user's responsability to pass his data in a form which will reliably sustain encryption/decryption. Strings are universally encoded in UTF8 then the resulting binary is passed to encryption. Similarly at receipt: receiver has to reformat the recovered binary into something sensible for him, either by convention or on-band (or off-band!) signalling. The crypt engine can't do that and decide that all data passed to encryption or resulting from decryption will be strings.

Also crypt engines need to be interroperable across all available hardware or software platforms. That forces UTF8 for strings.

Then most uses of encryption is inside protocols, SSL being a common example. Protocols obey strict rules and when a binary data block of size N needs to be sent by the rules, it just can't decide to change N because it finds that easier. Stream ciphers formally allow N to be zero. Obviously it has no right either to change the binary content of the data! Deviation from these no-nos spoils compliance.

Finally and since encryption is a key feature in security, it makes sense to protect encrypted exchanges from obvious attacks. For instance it is natural to compute a running hash on the binary passed to encryption and sign it so that the recipient can check that no part of the recieved and decrypted binary is missing, out of order, modified or added.

With all this in mind, please have a look at this simulation, which compare implementations of RC4 and 3DES with the fix I propose and yours. Again there is nothing personal here, you know that.

You can observe that there are significant differences in binary recovered vs binary encrypted and in running MD5.

In short your patch can only reliably work with itself. Depending on context, it's likely that it won't successfully interroperate with any other implementation in the general case, provided the engines are not only used to implicitely exchange strings.

[guinness]

Okay, I see your point. Are you able to submit the fix to svn? If not then would you be so kind as to provide me with modified functions so I can do it myself.

I am not taking your posts as a personal attack. It's good to get things right the first time.

[jchd]

Sorry for the text wall

It's tempting to short-ciruit the whole function with a return, but things may not be as simple as they seem. For instance should we silently return, say Binary(''), when the key invalid or should we go thru parts of the functions to discriminate that case even if they key is irrelevant then?

[Melba23]

jchd,

Or let me have the amended functions as I know guinness has no SVN access at the moment.

As to your question, I would suggest returning Binary('') but also setting @extended to show that this is an automatic "shortcut" return which prevents a bigger problem.

M23

[jchd]

Good suggestion and also my initial idea.

EDIT: in fact the fix needed is a little more subtle than primilarly expected.Ticket #2814 has been created and the fix is on its way.

Edited July 27, 2014 by jchd

[Proph]

Thanks for looking into this devs! I know it isn't much of a bug... but I do think people will appreciate the fix to simplify the udf in regards to this minor glitch.