3DES DoEncode()/DoDecode() Source overwriting Dest problem
VideoRipper opened this issue · 10 comments
Preface
I'm updating very (very!) old code that used DEC3 by using the latest version available at time of writing.
For backward compatibility reasons I have to make sure (for now) that ciphers encoded/decoded are identical to DEC3, so I enabled compatibility mode (DEC3_CMCTS) for 3-DES and fixed the DecodeCTS3 method which was missing the TDECCipherModes class in the implementation (which is also a minor bug in current DECCipherModes unit).
When I compared the output of the new DEC6 cipher implementation against that of DEC3 I saw they were not identical.
After some investigation I noticed something that strikes me odd in the implementation of the 2DES and 3DES cipher DoEncode and DoDecode methods and after changing it in the library, the results were identical to DEC3 again.
Also, when comparing the implementation of 2DES and 3DES to other DES ciphers (like 2DDES and 3DDES) it appears that both 2DES and 3DES contain the same bug.
Describe the bug
Implementation of 3DES DoEncode- and DoDecode-methods (for 2DES it's similar, but not checked myself):
procedure TCipher_3DES.DoEncode(Source, Dest: Pointer; Size: Integer);
begin
Assert(Size = Context.BlockSize);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[ 0]);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[32]);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[64]);
end;
procedure TCipher_3DES.DoDecode(Source, Dest: Pointer; Size: Integer);
begin
Assert(Size = Context.BlockSize);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[96]);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[128]);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[160]);
end;
If I correctly understand the code, the first call to DES_Func will take the Source and put the result in Dest, but the second and third call to DES_Func will also take the original Source and will each overwrite Dest again, resulting in only the last call being effective on the output, resulting in an erroneous result.
Expected and actual behavior
When I change the code to the following, the result is as expected:
procedure TCipher_3DES.DoEncode(Source, Dest: Pointer; Size: Integer);
begin
Assert(Size = Context.BlockSize);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[ 0]);
DES_Func(Dest, Dest, @PUInt32Array(FAdditionalBuffer)[32]);
DES_Func(Dest, Dest, @PUInt32Array(FAdditionalBuffer)[64]);
end;
procedure TCipher_3DES.DoDecode(Source, Dest: Pointer; Size: Integer);
begin
Assert(Size = Context.BlockSize);
DES_Func(Source, Dest, @PUInt32Array(FAdditionalBuffer)[96]);
DES_Func(Dest, Dest, @PUInt32Array(FAdditionalBuffer)[128]);
DES_Func(Dest, Dest, @PUInt32Array(FAdditionalBuffer)[160]);
end;
I haven't checked 2DES myself, but the problem appears to be there as well.
Since there's no progress, I've solved my problem for now by creating an inherited class that will move the source to dest before calling the inherited methods.
uses
DECCiphers;
type
TCipher_3DES_Fix = class(TCipher_3DES)
protected
procedure DoEncode(Source, Dest: Pointer; Size: Integer); override;
procedure DoDecode(Source, Dest: Pointer; Size: Integer); override;
end;
implementation
{ TCipher_3DES_Fix }
procedure TCipher_3DES_Fix.DoEncode(Source, Dest: Pointer; Size: Integer);
begin
// Copy source data to destination
Move(Source^, Dest^, Size);
// Call original method, but only use destination buffer
inherited DoEncode(Dest, Dest, Size);
end;
procedure TCipher_3DES_Fix.DoDecode(Source, Dest: Pointer; Size: Integer);
begin
// Copy source data to destination
Move(Source^, Dest^, Size);
// Call original method, but only use destination buffer
inherited DoDecode(Dest, Dest, Size);
end;
Sorry for not having responded yet, but the last few weeks were simply fully packed and my maind evelopment PC went defect as well (a warranty case). Now it is back and I hope to be able to look at this in the near future. We have to check for unit test test data for these algorithms to check your proposed modifications. I think I can follow you and I think you have a pojnt, but we need some test data to ensure we don't break this again somehow in a future version.
Ok I implemented your proposed changes now and you should see this in the development branch now. But I still do not have unit test data I can use to verify this one actually fixes something (as plausible as your report reads). Can you provide such data? So I could add unit tests for this and finally close the issue?
Is it possible to provide verifyable unit test data (from a trusted source) so I can add some test(s)?
I'm not sure how to determine where to find "A trusted source".
I noticed, while upgrading the suite from a very old version, that the results were way off and previously encrypted data we had couldn't be decrypted anymore, after which I started my investigation (and finally found the culprit).
The only thing I can imagine is encrypting random strings with the old DEC suite and decrypting it again with the new one (and vice versa): that is what I did to test the my change; nothing fancy (no unit test), just a list of strings from a file being parsed and comparing the outputs.
Ok, if you could share those strings (at least 2-3 or so would be sufficient I guess) I could implement a unit test for this and close the issue afterwards.
I've created a small utility that can cipher and decipher strings using the old and the new component 3DESTester.zip
With it I made a sample file that contains both the deciphered and a ciphered version of 10 strings in an INI-file style format (also included in the ZIP), I hope it's useful to you.
The key/password used is "DECPassword" (without the quotes). and there's no IV
[DECIPHERED]
1=Must you with him from him her were more. In eldest be it result should remark vanity square. Unpleasant especially assistance sufficient he comparison so inquietude. Branch one shy edward stairs turned has law wonder horses. Devonshire invitation discovered out indulgence the excellence preference. Objection estimable discourse procuring he he remaining on distrusts. Simplicity affronting inquietude for now sympathize age. She meant new their sex could defer child. An lose at quit to life do dull.
2=Fat new smallness few supposing suspicion two. Course sir people worthy horses add entire suffer. How one dull get busy dare far. At principle perfectly by sweetness do. As mr started arrival subject by believe. Strictly numerous outlived kindness whatever on we no on addition.
3=Full he none no side. Uncommonly surrounded considered for him are its. It we is read good soon. My to considered delightful invitation announcing of no decisively boisterous. Did add dashwoods deficient man concluded additions resources. Or landlord packages overcame distance smallest in recurred. Wrong maids or be asked no on enjoy. Household few sometimes out attending described. Lain just fact four of am meet high.
4=Surprise steepest recurred landlord mr wandered amounted of. Continuing devonshire but considered its. Rose past oh shew roof is song neat. Do depend better praise do friend garden an wonder to. Intention age nay otherwise but breakfast. Around garden beyond to extent by.
5=Prevailed sincerity behaviour to so do principle mr. As departure at no propriety zealously my. On dear rent if girl view. First on smart there he sense. Earnestly enjoyment her you resources. Brother chamber ten old against. Mr be cottage so related minuter is. Delicate say and blessing ladyship exertion few margaret. Delight herself welcome against smiling its for. Suspected discovery by he affection household of principle perfectly he.
6=Old education him departure any arranging one prevailed. Their end whole might began her. Behaved the comfort another fifteen eat. Partiality had his themselves ask pianoforte increasing discovered. So mr delay at since place whole above miles. He to observe conduct at detract because. Way ham unwilling not breakfast furniture explained perpetual. Or mr surrounded conviction so astonished literature. Songs to an blush woman be sorry young. We certain as removal attempt.
7=Building mr concerns servants in he outlived am breeding. He so lain good miss when sell some at if. Told hand so an rich gave next. How doubt yet again see son smart. While mirth large of on front. Ye he greater related adapted proceed entered an. Through it examine express promise no. Past add size game cold girl off how old.
8=Certainty determine at of arranging perceived situation or. Or wholly pretty county in oppose. Favour met itself wanted settle put garret twenty. In astonished apartments resolution so an it. Unsatiable on by contrasted to reasonable companions an. On otherwise no admitting to suspicion furniture it.
9=Rendered her for put improved concerns his. Ladies bed wisdom theirs mrs men months set. Everything so dispatched as it increasing pianoforte. Hearing now saw perhaps minutes herself his. Of instantly excellent therefore difficult he northward. Joy green but least marry rapid quiet but. Way devonshire introduced expression saw travelling affronting. Her and effects affixed pretend account ten natural. Need eat week even yet that. Incommode delighted he resolving sportsmen do in listening.
10=Difficulty on insensible reasonable in. From as went he they. Preference themselves me as thoroughly partiality considered on in estimating. Middletons acceptance discovered projecting so is so or. In or attachment inquietude remarkably comparison at an. Is surrounded prosperous stimulated am me discretion expression. But truth being state can she china widow. Occasional preference fat remarkably now projecting uncommonly dissimilar. Sentiments projection particular companions interested do at my delightful. Listening newspaper in advantage frankness to concluded unwilling.
[ENCIPHERED]
1=MEVzAZo4iVBMZQUP+3KhEV/n2RpivD7qPQeutlMq+BkvwuLumhgy+/hCmNpPm4gtG1VXAx7hEmHeD1SN7KyMfaw3iaxX1QbH+b3QBGKGS/Oxol37+nu8O+L/9lXQD4OI5oW2ytiuJEJYKOmwo8UaUTgmJ6DAow/XBwr3NUEDs8+QIDRB6Za+evySyqTGQ+aL6t+yf+xa4jvFJOswk3aQjjowaK4efCuQ5szhBe9x1eu1v65GSy5zoaZfYKXl6irR05JihaT1UZ2FWwDVlvytSILJQCL1YpUduarElW6CB+TT+M+imKfuCyF5smURgpZ2k1QlvKr9QUTvKndkobWL+qjmZIOWp8mCJGPaKbxRHh74/gwY340V3EZS1ZkynVtpoDVPw1BKjXW38qONUu5tS7bexbeS5TYUX3X3AG98WNPyeuzl9LL6qGniVKnNoSWx6xluUsnZOM142lX9bf/WMTDjQIQoBl9BMK9khuDoBDhmSjFEj4wpe2qLW9YSl9QfvczLsbf6ZX1Bm+jOgc8wM2p5uHxuNPrM7GputyYVXoGVwF7zb1yt5+1UeJYncutMLHUdD93OA/OoETU1gM9kWMCfj+S/JsM/O00GvYKDr21gWZO9VuAl9ZRs2HzMnxcN9wl2PzbWY7f7fD94wN3NP9gmcAOKVWg=
2=MBzgE2lFvUc7LABV2lRZ6jfdnO2lHavDYYrm8N094fm1FCm3CvQ0fG/n4cYRtLh6QstmjYq68D/aR5ib0e2X4LgAxlATCrAMur1wEwRX4uj7B6G9kE7XCq+PRF4I+Fp2/kiL+qKuxW7nT08TvGWY2KysXoqJddn6h1VcfgRjkel0++zNrefD7TVN6Qvyzwi/M0IpyHkUWEcHJ2fMAJTokZwHH22HaPOmXHZV1yxU3Bv2XnSGlRTNCFanDm3pmlMev2hHeHj0W9RukCAqjUOgf44t2NeFpxnMCNObuw0FdpgR4NWf3I8ICCHQLSa9K9KMutIMM8rYc0MIt7jmO2Dgaed6rwZoTwuGnqzcTYQJvnSdRt8VY5U=
3=4Ppzz7YGvJDaOEIRkgasR92ic1PD6tlXmoYyjuX70rXdY0gFooViCyXH0QhGSaWseKPp2kwVDhoDg3X8+jwsznnA9geknd1VD3ub03UYx1Kk5Teel/hDi/reEvYumMme2NkfaYtGCXIhkgPQ//6s0eyBYE9U0N2CdLIMCnaH+6yLQ+cCTy32/AWeLYZjBBGdj33Ngp0R8caE21huoPnOb3Cdq56d/PstknQPqv3XDiQVtAjp6a0ruDPmYxK0DVcqPZA4J5LGEwoVaCd0e5uGl2h+yEc6SQo8Ki14nVB/AviIz5mYUITXq7zORN3jDyZ+bbB0lxzcwreLGHZrBfttH5UcRH04SIoUlz+UQjuAevyXx9dxPEwTvFk+0qLhTsGqWAMRb7SDbgJTLNwD5oj4MAHHV6cSMkcGpgX2qjJfbSmJnHdQE1gclPRRBf7t+2+MpqyUK4jKB8BCfbaZAPXVQj6HluNnbt03Ldg4Ty4KsovA6iFyp9Njr71HdWuZBY3rM46OyI3g9/xa9S4GXlhScS7ac13qfgP48nBDkiTrkUkoRT5Gr+Q=
4=QxYajRr/PCkg8R2rVI2s9QkiSexRwG+A/oOsmwhN43uRfX63y7tws4hQnwZTJGLftohaTohzAV0nosLFMgUubN1hAmxVUV6YrmA8iqrQ4MaM9Pl08W5lkAGsSSwXJ2aaNzEkUQ3P2V8R9E+Q2VZWrWAWH3QcDM6lSMHrOcgFdG3/9zchVuKOtI3akPfAz+vnyIvGhpfpYSLcA+zgPyuU6qbbUOMtivfP/sk7FBQnWTiaf1Qx6fibIagxKxOe+Vm7xjDIB05Rs2H2vDaMrDnGrDPT2ahPTlTByGZxgkhHpC79a9UY2a6QMXuzbkU5j8RQ755JGT/z0w/aFI/atm+7WeHv7jd5TebiNqtXxcyIh+k=
5=vFPC5BZee5tOpGc8uOuWXn/gsDU31t3xA7PLYJKUNhiAkL5ZbWfA0uXflhpZPDyJiFFK5+WX4JrIgNPKKV8uC08ngapg84cFhuNYEcDMNwUUWsdlk1kHV8o0uHu6OqhWH1Io1CP5VRKoLicgblNlt/6E18893FXXykImSFjrSgSLktDrfxweYuNUealbfIPBg3tYIIbaTxUAk6iXT6puK3VsJujw9OJXRrcQJn4xF1sMvgxE8mI1WjgT76BI4SS/B5MiD/ofWXF3ggUVzVjKV0JK+B/kw0QaG3MIX572ZSdUI4DmawvHwpcuquMO/zLKJAsAe5eDVNfA8BBwz0ZOMBGT/HOzEmBQBOYRQ5Ispi+ASy16s/DLKVOPxTm54omda0BZLSpfM6ussvUn2F2WLl/UjuRSOuD4yQJ7ki2IQceG8T/JpLDHBEik7YOnSC9TTu0uz4d9Idahcus2nhEPq5+LQwTI6k6TrxB94roCKK+gv4pZbg1jZk2U/mEo2QE31/2epuPV6NJAWUH72LLBmvjn8KHdzHkipBVdVR1jtarCFa5yq0z6whos71sxcMkzSX7KYqxyH7wJgw==
6=u1Fg9aroAmswSo8Q/NI5Vm7AlBw4FVg6p85SOySMdvwxxasis22bSIl2P+rkB5qm09fp6m5rzjCv/WMDpl9wXdl+JCap7MfpMIAiEr1NqYdF78KQAtBp1SyI9EJNHcTaBE24K3vHXvMaI9wDxgAy6EAB7PA3cptqeNhOcFCOeITW2LOL25uZ9rOnCU5Mw1lRGLipiKvTz3NT1IafF5tg0GCd0eyTvUNkVpIGrilUS8+GIBqIMTXvmAuTFO6nqgb/fXwDfwHFdASUt3TAxIitz+8rutc41JZODwMoG1t1ppjMPQf/op9luvFTwp0QPak3yp0kRZd7JRabavkoyYq8Fex/jGz2T8zJdci+YEsGM/ixNoWAEOSxgnDrK4+f8Qxwfz+ehRxgoU7mc67p3GLw7UDi1/SEo/ZHise0nRNS4esDmT6xhAByRRO23BCHd8Z5xwamNbuD7qzW/wBVr9KKDOtww5t/taNE8GfUvs4B+uo3jWI0YMZlAEYpESqlCtQE+CLM0IsVroPrO+qNTc9F93X7+zphmqqxLZEx498QezGGsQl8md9U4n5AioUIDFfoBGkO1T5mtlnaIq20Q4/UROCLcTSkc54wfhc3NFWjZwPfWoMpZk7SOWYI
7=Am6rj0tuUN+wrYPug9/qPQ93GUBcCeSxg3+o258KqEaqzLeMvmBR41GSt0wbApBDfaaWLdapQZFcm4lb0A3EU4rX7c+aSag8KNATBm3gJ/CRPghMwvA/0zfubX5Pd9dKXYIXMJI4UpnL4/qLkLF3Ic/DQAlYHtTMscZYXvrvqhoApLrPZkr85nU6elrvoWUH1chZSBYGVgpQickC/hICncl7wfgPdUkw9gJpp/ijpxtJd+KBEjFvQ2zSifW5OZvQL6vrvOb0ap30dbApMcHmejuEXaI2CcZRS93n2OzUYE77wR2FSqw8mLEAEpd+L9pHtq3ZqXGQopTZepeazXrBDwIF4A8rG/JiDt8OOhAPRClET5dutzOJn047jYEfsjtZ/nvEIimec14lfU823Z+NhsdFYZGAXhAjhd17nwFpRuCFmj0xcbFF8T0=
8=mBw/TpmiOdHCOa1TW7Yt9vqEicuDXE5fxeGZgE8w54Lt0NI2mpCSotkbpyCFdjwPX9mrGtuSoWDXGOHbKojVcFKmeR6ljpTIsvAGXrPPB20JTQotFBdkwMeDIiL/PmUZQg5ZyfSBaKfhvDd09PqaiLdQ9iJRVXQQ7BdsbtC/GfomQmRDmhPycqHCvGkClMPFYtgKnFFRmrCF3RPhK4XbzdBn9fTi0g8cGyQ/L5A/vxyW7ZDcLxZ1k8Ie4CZdpn9ztgI7Fr1RAHzhldQSYkwleqosbL9eQzaZi2kt3aM3uKMfJJ1zDGj6Fwe9FNmflm+op8GRyczelMeEzEYsf2W8LyTaY/QNrALnsOmOi+DKSXMKERnpIPH/WwA5/c+zH/T29cB3wJm98ioeODOuJA==
9=18MKDNAHesgaqrINfzRKI1w/r2e2yh4H1sO9C0khJgCgJ5/lzLtLsbWxMsnJOcyzOh2DyCZry8Uo4LX663YyOVukMlRTxuhZ3UTSNojfuYybIVYP9Yc0RazxCd3mdWjWx3ph56FDgQiJfNme/curAU8t5v3RvgM7rQR/01Ae5xJv9HpoxEgSKvKTXmCyblABs7lwFnkvLvNuWQKh297wXNS1c/Wj37QrWDvSFpDTmTbUNZM6Cwa3KdmSDNZPAlJ9R7mJBdouk2u2pkgZGxPiFbx8lIsMWG0DSdBkzRhEeY3fgCt/Br6Smsbv63q+gOtWc6WngawgOwpCAkyi0+59b6vPTjXW8tIowqsd20w9rzWZ5uNw+GpU05XaqnurQyx7UbCJmaXRYiHBVPmI2Zi4i2wgxVtjCvB1HDmzLwyG1fmdnEfFOUMjorpEGhrOQqh3jG4teDkKVD+k9hXtWjQ2stPUvSvMXis3KH7rSkCRMM7JnFr+eR/YrG2j4k+ceF1LBpadzRbD+lUvm7cHOUy2CXvizSer4/EX33srmwaXM/wZbom6HSpG5RffWMnTMKAkXvy5p3lRAWPrdllfkZ4eMWxMJmbrfqW0Lk5BjUCaAx5sYxASB1sZbjU99SmNcx2NIX0ba0QVEUYTROFI4g==
10=apWPN89IDN6S8meWOYdFpH3n8siJuzW8+1imMuLt0RbK/OmUaiv4Ir+rrDvlZFq8fUZiped23cutxtzXv40T1R7+o6PNuhyXLOsTBo2RAV8Ikp+6sB7X83GgTRR3tG7be+cwl3SNMfYmjS13YnJ2/TddeBzYnNgVAs5w4BvnAQXRNK9ncJOSUlY5IULLqUr9QwDmQy/72M4cBRf7yCEGXPW6yM0fbeyaxrEO4o9SVGEwjmoM5ugYkQzajVctdJ1Nysd1D0ccnzJPwrYK5O90yu9G2Waw2QmJ3FjmKAx1aFsB5G0AL6UMiuAhAQqdPEvOpvlpZkUa+P0VJ9UUHkwAMston6p0S5N83hCsv1Fl1QqidhG4oaoj64epDje0SkV+fXW9p3j2lYHAqWySKP8YJaVli4XN+aQrrts5ZBcZYb/8ottEHa0MiEeV9OyTyW0lf5qrAIwEBEJD3Yo36IEVMwzekm04SLOZeT2r2RLQ7UMTTAREZ3+SZg+aHbqV+KfTDQdc9kwb3NJRYLPRzVchR4JTi/8Ig9i7121pUP6dat/rVeKl3lFW9CbA57IT9C3+BcFLbIxV2jlBty0iL5i0mzTg+BG8hEj10c2GxxscvcDia6vZ7SfWdjbYWi3cweVvDlUT3oaBSUpM70LwAdjEJ5BJtxEiswa4HBqDphrn9KcRgxTjn1NidjAPqcgzqw7LRNkf0faGZD8bjYMJVIhl4mxNF6lj/SfbyDczdcYuGP6kkzJAW8IcGw3DkjlztSZ0fisE8g==
I added the missing class name for TDECCipherModes.DecodeCTS3 now. Thanks!
Thanks for your test data but I won't use that, as that requires me to enable this nowadays outdated padding sheme, which I doubt I can do only in the unit tests. Since the currently used test vector (used with cmCTSx mode) passes the test I'd close the issue as fixed, if you don't object. I can understand why you need that test data the way you do, but for moving forward that's not really what I'd like. I hope that's ok for you.
I've got no problems with it at all.
I only noticed that, while upgrading code from the old component to the latest version, the results weren't identical. 3DES with CTS indeed is considered unsafe for years and we will be switching to more up to date ciphers in the future, but as a first step I had to make sure current code (and the results) is backwards compatible,
Thanks for all your work and effort!
Kind regards,
Peter (the Netherlands)
I close this now in accordance with the issue creator.