TCipher_SCOP out-of-bounds read

[decfpc]

DEC's SCOP implementation is broken. I noticed this while porting the older version of the library (the "unofficial fork", as you call it) to PowerPC.

The array indices in these 3 lines all need to be + 1 like the first one, else they read out-of-bounds data when I >= 4. Also refer to the original spec.

On big endian processors the byte order of the following 32-bit array in the struct is different, which is why it was causing different results than on little endian processors. It had me quite confused for a couple hours.

PS: I've seen your issue in my repository and will provide my views on the matter soon-ish.

[MHumm]

Thanks for reporting this. I'll have a look at it as soon as time permits. If you detected this but the current unit tests don't, (they run 100% correct) you might have further test data I could use for validating the correctness and completeness of a fix. Is this the case? Would you like to share that?

[MHumm]

PS: have you seen that somebody provided code changes for DEC which made it FPC compatible? (at least it should be, I don't actively test that).

[decfpc]

Here you go. Each OutputData value was computed by the original C program for SCOP.

  SetLength(FTestData, 3);

  // Standard test vector
  FTestData[0].OutputData  := 'ca29853fb7eec7f958931ff185c0b415c944c22f13e34423aba1a84fb3101f19';
  FTestData[0].InputData   := TFormat_ESCAPE.Decode('\x30\x44\xED\x6E\x45\xA4' +
                                                    '\x96\xF5\xF6\x35\xA2\xEB' +
                                                    '\x3D\x1A\x5D\xD6\xCB\x1D' +
                                                    '\x09\x82\x2D\xBD\xF5\x60' +
                                                    '\xC2\xB8\x58\xA1\x91\xF9' +
                                                    '\x81\xB1');
  FTestData[0].Key        := 'TCipher_SCOP';
  FTestData[0].InitVector := '';
  FTestData[0].Filler     := $FF;
  FTestData[0].Mode       := cmECBx;

  // Standard test vector, key with 'odd' bit set
  FTestData[1].OutputData  := '18be1fff893de279b5768b21307c5c52436835c83ed5c96ea589884b61c69dc4';
  FTestData[1].InputData   := TFormat_ESCAPE.Decode('\x30\x44\xED\x6E\x45\xA4' +
                                                    '\x96\xF5\xF6\x35\xA2\xEB' +
                                                    '\x3D\x1A\x5D\xD6\xCB\x1D' +
                                                    '\x09\x82\x2D\xBD\xF5\x60' +
                                                    '\xC2\xB8\x58\xA1\x91\xF9' +
                                                    '\x81\xB1');
  FTestData[1].Key        := 'TCipher_SCOPa';
  FTestData[1].InitVector := '';
  FTestData[1].Filler     := $FF;
  FTestData[1].Mode       := cmECBx;

  // Full 48 bytes key length
  FTestData[2].OutputData := '8dbc6579ac264ccfbb0f7aea';
  FTestData[2].InputData  := '12bytesbytes';
  FTestData[2].Key        := 'TCipher_SCOPTCipher_SCOPTCipher_SCOPTCipher_SCOP';
  FTestData[2].InitVector := '';
  FTestData[2].Filler     := $FF;
  FTestData[2].Mode       := cmECBx;

If you apply the key generation fix, the second test should fail because I found another bug while working with the C code: https://github.com/MHumm/DelphiEncryptionCompendium/blob/a554c0f1d48445decbd36d49b62c305a2d8cfbf8/Source/DECCiphers.pas#L3157 This needs to say P[I] := P[I] or 1;. I + 3 would only be accurate if P had already been set to FUser/FAdditionalBuffer, which is not the case.

PS: have you seen that somebody provided code changes for DEC which made it FPC compatible? (at least it should be, I don't actively test that).

Yes, but I haven't checked it out either. I can tell at a glance many of the cryptographic functions will segfault and/or produce incorrect results on non-Intel targets. I can contribute fixes for that once I'm done with the work on my fork.

[MHumm]

Thanks for the test data and the other fix. My result: when I add your test data and your fixes the unit tests run fine for your test data but fail for my existing test vector. Does this mean the existing test vector (stemming from the original creator of DEC) is completely wrong? It uses mode cmCTSx.

Where does your test data come from? And what's the source of mine (if it still can be traced)?

Just to make sure we're both on the right track. Wouldn't be the first failing algorithm in DEC which passed the test data we have implemented as unit tests. :-(

[MHumm]

You may notice that I pushed these changes now to the development branch, even if there's still the original test vector in which makes the unit tests fail. I'll try to find time soon for checking the test vectors.

[decfpc]

Does this mean the existing test vector (stemming from the original creator of DEC) is completely wrong? It uses mode cmCTSx.

Where does your test data come from? And what's the source of mine (if it still can be traced)?

Yes, it is wrong. Unfortunately I think many of the test vectors from the original DEC were created by running DEC's own algorithms. This is further underlined by the fact that none of the programs written by the original cipher authors support CTS mode. I guess it was simply assumed it was operating correctly because encrypting and then decrypting produced valid output, despite the fact that the key scheduling algorithm violated the spec.

Like I said, my test vectors were generated using the original implementation. You can find the C code for SCOP in the spec I linked in the first comment. I compiled it with GCC, changed the key/plaintext and had it print the generated ciphertext. That's why all of my vectors are mode ECB.

Unfortunately I've also found what looks like multiple bugs in the key scheduling of TCipher_Shark. I'll report my findings in a new issue or directly create a pull request when I'm done with my investigation.

[MHumm]

I saw the code in the SCOP spec, but I'm not fluent enough in C to completely understand its meaning in my first short reading. We should try to use original vectors where available. I second that. DECs test vectors were originally stored in a text file processed by some "arcane" test program which was hard to understand. I continued the creation of DUnit tests the 2nd maintainer had begun. But his attempts only covered a few helper routines. I simply took the vectors found in this text file.

About them modes: if you look at the unit tests you'll see that I tried to start tests for the modes as such, but I'm no expert in all of this so I got stuck there. I tried to learn how these modes work by reading their Wikipedia articles, where existing but I often was not sure how to "test them in vitro".

I'd be grateful for a collaboration on all of this. I also think (and for the SHA3 unit tests you can already see this) that adding the source for the test data as comments to the test might help. Such "decorated" test data from "official" sources should provide good evidence that the particular algorithm works correctly.

[MHumm]

Once again to your provided SCOP test data: I tried to add another test vector by using the C-program in the original specification. I took that and made it compilable with C++ Builder. I shortend MESSAGE_WORDS from 1024 to 10 and had it print out the contents of buf after encryption. Then I set up the following unit test definition, but that fails and gives a completely different and way longer output:

FTestData[3].OutputData := '195f5dce419d3b3d35616cf06df6a3c38d790dbe5d0d20587b72a221af98b9a6deb04be07dbb3c8a'; FTestData[3].InputData := '00000000000000000000000000000000000000000000000000000000000000000000000000000000'; FTestData[3].Key := #0#1#2#3#4#5#6#7#8#9#$A#$B#$C#$D#$E#$F; FTestData[3].InitVector := ''; FTestData[3].Filler := $FF; FTestData[3].Mode := cmECBx;

Unit test message shown: TestEncode: ETestFailure at $006BC65D expected: <195f5dce419d3b3d35616cf06df6a3c38d790dbe5d0d20587b72a221af98b9a6deb04be07dbb3c8a> but was:

Do you have any clue about this? I just like to verify the test data used as much as possible with original test vectors. But my C is not good so automatically creating the output is hard for me, thus my reduction of MESSAGE_WORDS.

[decfpc]

Here's a modified C program that's easier to work with: https://gist.github.com/decfpc/5e6f5e299a0747fb5f50c1167748980f I didn't touch any logic in the algorithms themselves (apart from variable types), I only restructured main a bit. I'm using GCC under WSL2, but C++ Builder should compile it with no (or at least less) modifications than the original program as well.

  FTestData[3].OutputData  := 'ce5d5f193d3b9d41f06c6135c3a3f66dbe0d798d58200d5d21a2727ba6b998afe04bb0de8a3cbb7d';
  FTestData[3].InputData   := #0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0#0;
  FTestData[3].Key        := #0#1#2#3#4#5#6#7#8#9#$A#$B#$C#$D#$E#$F;
  FTestData[3].InitVector := '';
  FTestData[3].Filler     := $FF;
  FTestData[3].Mode       := cmECBx;

Your test data had 2 issues: 1) Input data must be null chars, not ASCII 0. 2) When printing out dwords for use in a byte vector, they must be swapped for correct little endian representation (e.g., 195f5dce vs. ce5d5f19 for the first dword).

[MHumm]

Will try to check this as soon as possible. I see where your improvements head to and I welcome that. When that's finished I can make sure verifiable SCOPE test vectors are implemented and close that chapter. Unfortunately I run in an error trying to compile your code. C++ Builder complains about not knowing byteswap.h. But I'm already investigating there. I'll go to bed now.

[decfpc]

Urgh, I guess that header is GNU specfic. You can remove that include and replace the line printf("%08lx", __bswap_32(buf[i])); with printf("%02x%02x%02x%02x", buf[i] & 0xFF, buf[i] >> 8 & 0xFF, buf[i] >> 16 & 0xFF, buf[i] >> 24);.

[MHumm]

Since I implemented your fix and generated some test data with this C program I got working now I'd like to close this issue as fixed. Ok?