Fix issue 723 - Fixed use of ECB with Botan #724
Conversation
antoinelochet
changed the title
There was a problem hiding this comment.
Seems reasonable.
Are all the extra debug printouts needed? I seems that existing printouts are quite sparse with information, maybe for security reasons(?).
As an addition I suggest that we also disable 2 testcases when built WITH_BOTAN.
They will always fail when using Botan as I understand it. WDYT?
1) test: AESTests::testECB (F) line: 518 AESTests.cpp
assertion failed
- Expression: aes->encryptInit(&aesKey128, SymMode::ECB, IV)
2) test: DESTests::testECB (F) line: 537 DESTests.cpp
assertion failed
- Expression: des->encryptInit(&desKey56, SymMode::ECB, IV)
|
I think that you are right about the tests. I have added the debug logs because they are often useful. I agree that some security reasons, it may seem bad, but:
So I thought this was tolerable. But per your request, I deleted all the offending lines. |
src/lib/crypto/test/AESTests.cpp
Outdated
| @@ -328,6 +328,7 @@ void AESTests::testCBC() | |||
| } | |||
| } | |||
|
|
|||
| #ifndef WITH_BOTAN | |||
There was a problem hiding this comment.
I believe this will give an undefined reference to this function while linking.
Maybe its better to just remove the registration of the testcase in the header file.
#ifndef WITH_BOTAN
CPPUNIT_TEST(testECB);
#endif
There was a problem hiding this comment.
Same for the testECB() testcase in:
src/lib/crypto/test/DESTests.h
There was a problem hiding this comment.
I followed your suggestion and compilation/linking is ok now.
The tests fail on the src/lib/test though but I am pretty sure that it was the case before my changes.
|
if you revert BotanSymmetricAlgorithm.cpp 8fd89ec and uncomment if (mode == SymMode::ECB) all tests are passing. Tested with Botan 2.19.3 |
That's because ECB is not taken into account during compilation phase. It does not fix the failing tests in |
Sorry I dont understand what do you mean "ECB is not taken into account". You mean because Botan_ecb.cpp is missing in CMakelists.txt? The test was performed with dev branch and not with fix-botan-ecb |
Here is my proposed fix for issue 723