-
Notifications
You must be signed in to change notification settings - Fork 485
KangarooTwelve #708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
KangarooTwelve #708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're missing something there.
hash_state is a union, i.e. it would be valid to do the following:
struct sha3_state st;
sha3_shake_init((hash_state*)&st, 128);https://godbolt.org/z/Wq3oaETcE
Please revert this change, I'd prefer to keep the codebase of all the algorithms similar.
|
Yes, that works, but. I need to keep the two SHAKE states inside a KT state between invocations of the init/process/done functions. And because of the union thing, the KT state would be unnecessary big. Keeping state for other hashes that we don't care about inside KT logic. Do you prefer me reverting this or changing every other algorithm to use its own state instead? // Edit: Oh, I misunderstood. Now I get it. I will do the change you propose. |
sjaeckel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Very good PR :)
Thanks for updating the docs ❤️
What do you think of my suggestions?
src/hashes/sha3_test.c
Outdated
| static LTC_INLINE unsigned char s_turbo_shake_hex_to_nibble(char hex) | ||
| { | ||
| LTC_ARGCHK((hex >= '0' && hex <= '9') || (hex >= 'a' && hex <= 'f') || (hex >= 'A' && hex <= 'F')); | ||
|
|
||
| return (hex >= '0' && hex <= '9') ? (hex - '0') : ((hex >= 'a' && hex <= 'f') ? (10 + hex - 'a') : (10 + hex - 'A')); | ||
| } | ||
| static LTC_INLINE int s_turbo_shake_hex_to_bin(char const* hex, unsigned char* bin, long amount) | ||
| { | ||
| long i; | ||
|
|
||
| LTC_ARGCHK(hex != NULL || amount == 0); | ||
| LTC_ARGCHK(bin != NULL || amount == 0); | ||
| LTC_ARGCHK(amount >= 0); | ||
|
|
||
| for(i = 0; i != amount; ++i) | ||
| { | ||
| bin[i] = | ||
| (s_turbo_shake_hex_to_nibble(hex[i * 2 + 0]) << 4) | | ||
| (s_turbo_shake_hex_to_nibble(hex[i * 2 + 1]) << 0); | ||
| } | ||
| return CRYPT_OK; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this maybe be done with the base16_decode() API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
src/hashes/sha3_test.c
Outdated
| #endif | ||
|
|
||
| #ifdef LTC_TURBO_SHAKE | ||
| static LTC_INLINE int s_turbo_shake_test_one(int bits_count, long input_bytes_count, long skip_digest_bytes, long digest_bytes_count, int counter, char const *expected_digest_hex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much like the idea where this is going to! 👍 💯
How about:
| static LTC_INLINE int s_turbo_shake_test_one(int bits_count, long input_bytes_count, long skip_digest_bytes, long digest_bytes_count, int counter, char const *expected_digest_hex) | |
| typedef struct turbo_shake_test { | |
| int bits_count; | |
| unsigned long input_bytes_count, skip_digest_bytes, digest_bytes_count; | |
| const char *expected_digest_hex; | |
| } turbo_shake_test; | |
| static LTC_INLINE int s_turbo_shake_test_one(const turbo_shake_test *testcase, int counter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
src/hashes/sha3_test.c
Outdated
|
|
||
| return (hex >= '0' && hex <= '9') ? (hex - '0') : ((hex >= 'a' && hex <= 'f') ? (10 + hex - 'a') : (10 + hex - 'A')); | ||
| } | ||
| static LTC_INLINE int s_turbo_shake_hex_to_bin(char const* hex, unsigned char* bin, long amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usual way in C is to use const char* as type. They're equivalent but I'd prefer to keep it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in favor of base16_decode.
src/hashes/sha3_test.c
Outdated
| counter = 1; | ||
| if ((err = s_turbo_shake_test_one(128, 0, 0, 32, counter++, "1e415f1c5983aff2169217277d17bb538cd945a397ddec541f1ce41af2c1b74c")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 0, 0, 64, counter++, "1e415f1c5983aff2169217277d17bb538cd945a397ddec541f1ce41af2c1b74c3e8ccae2a4dae56c84a04c2385c03c15e8193bdf58737363321691c05462c8df")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 0, 10000, 32, counter++, "a3b9b0385900ce761f22aed548e754da10a5242d62e8c658e3f3a923a7555607")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 1, 0, 32, counter++, "55cedd6f60af7bb29a4042ae832ef3f58db7299f893ebb9247247d856958daa9")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17, 0, 32, counter++, "9c97d036a3bac819db70ede0ca554ec6e4c2a1a4ffbfd9ec269ca6a111161233")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17*17, 0, 32, counter++, "96c77c279e0126f7fc07c9b07f5cdae1e0be60bdbe10620040e75d7223a624d2")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17*17*17, 0, 32, counter++, "d4976eb56bcf118520582b709f73e1d6853e001fdaf80e1b13e0d0599d5fb372")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17*17*17*17, 0, 32, counter++, "da67c7039e98bf530cf7a37830c6664e14cbab7f540f58403b1b82951318ee5c")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17*17*17*17*17, 0, 32, counter++, "b97a906fbf83ef7c812517abf3b2d0aea0c4f60318ce11cf103925127f59eecd")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(128, 17*17*17*17*17*17, 0, 32, counter++, "35cd494adeded2f25239af09a7b8ef0c4d1ca4fe2d1ac370fa63216fe7b4c2b1")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 0, 0, 64, counter++, "367a329dafea871c7802ec67f905ae13c57695dc2c6663c61035f59a18f8e7db11edc0e12e91ea60eb6b32df06dd7f002fbafabb6e13ec1cc20d995547600db0")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 0, 10000, 32, counter++, "abefa11630c661269249742685ec082f207265dccf2f43534e9c61ba0c9d1d75")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 1, 0, 64, counter++, "3e1712f928f8eaf1054632b2aa0a246ed8b0c378728f60bc970410155c28820e90cc90d8a3006aa2372c5c5ea176b0682bf22bae7467ac94f74d43d39b0482e2")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17, 0, 64, counter++, "b3bab0300e6a191fbe6137939835923578794ea54843f5011090fa2f3780a9e5cb22c59d78b40a0fbff9e672c0fbe0970bd2c845091c6044d687054da5d8e9c7")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17*17, 0, 64, counter++, "66b810db8e90780424c0847372fdc95710882fde31c6df75beb9d4cd9305cfcae35e7b83e8b7e6eb4b78605880116316fe2c078a09b94ad7b8213c0a738b65c0")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17*17*17, 0, 64, counter++, "c74ebc919a5b3b0dd1228185ba02d29ef442d69d3d4276a93efe0bf9a16a7dc0cd4eabadab8cd7a5edd96695f5d360abe09e2c6511a3ec397da3b76b9e1674fb")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17*17*17*17, 0, 64, counter++, "02cc3a8897e6f4f6ccb6fd46631b1f5207b66c6de9c7b55b2d1a23134a170afdac234eaba9a77cff88c1f020b73724618c5687b362c430b248cd38647f848a1d")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17*17*17*17*17, 0, 64, counter++, "add53b06543e584b5823f626996aee50fe45ed15f20243a7165485acb4aa76b4ffda75cedf6d8cdc95c332bd56f4b986b58bb17d1778bfc1b1a97545cdf4ec9f")) != CRYPT_OK) return err; | ||
| if ((err = s_turbo_shake_test_one(256, 17*17*17*17*17*17, 0, 64, counter++, "9e11bc59c24e73993c1484ec66358ef71db74aefd84e123f7800ba9c4853e02cfe701d9e6bb765a304f0dc34a4ee3ba82c410f0da70e86bfbd90ea877c2d6104")) != CRYPT_OK) return err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then instead of those n calls
const turbo_shake_test testcases[] = {
{ 128, 0, 0, 32, "1e415f1c5983aff2169217277d17bb538cd945a397ddec541f1ce41af2c1b74c" },
{ 128, 0, 0, 64, "1e415f1c5983aff2169217277d17bb538cd945a397ddec541f1ce41af2c1b74c3e8ccae2a4dae56c84a04c2385c03c15e8193bdf58737363321691c05462c8df" },
...
};
for (counter = 0; counter < LTC_ARRAY_SIZE(testcases); counter++) {
if ((err = s_turbo_shake_test_one(&testcases[counter], counter)) != CRYPT_OK) {
return err;
}
}
return CRYPT_OK;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
7de08be to
63bf8b1
Compare
sjaeckel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I overlooked that at the first review.
Thanks for reverting the first commit, the changes look a lot better now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #if defined(LTC_TURBO_SHAKE) && !defined(LTC_SHA3) | |
| #error LTC_TURBO_SHAKE requires LTC_SHA3 | |
| #endif | |
| #if defined(LTC_KANGAROO_TWELVE) && !defined(LTC_TURBO_SHAKE) | |
| #error LTC_KANGAROO_TWELVE requires LTC_TURBO_SHAKE | |
| #endif | |
I already forgot that in the TurboShake PR ... we should check that our dependencies are fulfilled and highlight what's maybe missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
src/headers/tomcrypt_custom.h
Outdated
| #define LTC_SHA3 | ||
| #define LTC_KECCAK | ||
| #define LTC_TURBO_SHAKE | ||
| #define LTC_KT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #define LTC_KT | |
| #define LTC_KANGAROO_TWELVE |
Even though I'm a huge fan of short names, KT is even too short for me 😜
And the API should also be renamed accordingly please s/kt_/kangaroo_twelve_/g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed. What about the function names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed. What about the function names?
The API should also be renamed accordingly please s/kt_/kangaroo_twelve_/g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
| \subsection{KangarooTwelve} | ||
| Another variation of SHA3 SHAKE is KangarooTwelve, which has been specified in \href{https://datatracker.ietf.org/doc/rfc9861/}{\texttt{RFC 9861}}. | ||
|
|
||
| The API works equivalent to the one of SHA3 SHAKE, where the APIs only have a different name. Additionally, KangarooTwelve supports customization. You can append any or none customization bytes after all input bytes and before squeezing any output digest bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can one also append customization bytes in multiple calls?
I'm just asking.
And is the result of kt_customization("foobar"); the same as of kt_customization("foo"); kt_customization("bar");?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so the RFC specifies that we MAY propose an incremental interface, for either M or C, so we should also document that somewhere what we support.
I just had a look at the RFC for the first time ... the D isn't fixed to 0x1F, maybe it'd make sense to add support for variable D in a future PR. But now we focus on KT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the customization could be appended multiple times. And yes, about the foobar==foo+bar. You can append unlimited amount of customization bytes, up to unsigned long count, if you want to append more, we should change the variable type holding the number of customization bytes added. To 128 bit number or even larger one. The project https://github.com/kerukuro/digestpp is quite limited in this regard, it holds the entire customization string in memory at once. My implementation holds only the SHAKE state. They also hold a 8 kB buffer, I hold a SHAKE state instead.
| int err; | ||
| hash_state md; | ||
| long offset; | ||
| long rem; | ||
| long count; | ||
| unsigned char input[1024]; | ||
| unsigned char digest[64]; | ||
| char const *expected_hex; | ||
| unsigned char expected_digest_bin[sizeof(digest)]; | ||
|
|
||
| LTC_ARGCHK(bits_count == 128 || bits_count == 256); | ||
| LTC_ARGCHK(input_bytes_count >= 0); | ||
| LTC_ARGCHK(skip_digest_bytes >= 0); | ||
| LTC_ARGCHK(digest_bytes_count >= 1); | ||
| LTC_ARGCHK(counter >= 0); | ||
| LTC_ARGCHK(expected_digest_hex && expected_digest_hex[0] != '\0'); | ||
|
|
||
| if ((err = turbo_shake_init(&md, bits_count)) != CRYPT_OK) return err; | ||
| offset = 0; | ||
| rem = input_bytes_count; | ||
| do | ||
| { | ||
| count = rem < ((long)(sizeof(input))) ? rem : ((long)(sizeof(input))); | ||
| if ((err = s_turbo_shake_generate_ptn(&input[0], offset, count)) != CRYPT_OK) return err; | ||
| if ((err = turbo_shake_process(&md, &input[0], count)) != CRYPT_OK) return err; | ||
| offset += count; | ||
| rem -= count; | ||
| }while(rem != 0); | ||
| rem = skip_digest_bytes; | ||
| do | ||
| { | ||
| count = rem < ((long)(sizeof(digest))) ? rem : ((long)(sizeof(digest))); | ||
| if ((err = turbo_shake_done(&md, &digest[0], count)) != CRYPT_OK) return err; | ||
| rem -= count; | ||
| }while(rem != 0); | ||
| rem = digest_bytes_count; | ||
| expected_hex = expected_digest_hex; | ||
| do | ||
| { | ||
| count = rem < ((long)(sizeof(digest))) ? rem : ((long)(sizeof(digest))); | ||
| if ((err = s_turbo_shake_hex_to_bin(&expected_hex[0], &expected_digest_bin[0], count)) != CRYPT_OK) return err; | ||
| if ((err = turbo_shake_done(&md, &digest[0], count)) != CRYPT_OK) return err; | ||
| LTC_COMPARE_TESTVECTOR(digest, count, expected_digest_bin, count, "TurboSHAKE", counter); | ||
| rem -= count; | ||
| expected_digest_hex += count * 2; | ||
| }while(rem != 0); | ||
| return CRYPT_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int err; | |
| hash_state md; | |
| long offset; | |
| long rem; | |
| long count; | |
| unsigned char input[1024]; | |
| unsigned char digest[64]; | |
| char const *expected_hex; | |
| unsigned char expected_digest_bin[sizeof(digest)]; | |
| LTC_ARGCHK(bits_count == 128 || bits_count == 256); | |
| LTC_ARGCHK(input_bytes_count >= 0); | |
| LTC_ARGCHK(skip_digest_bytes >= 0); | |
| LTC_ARGCHK(digest_bytes_count >= 1); | |
| LTC_ARGCHK(counter >= 0); | |
| LTC_ARGCHK(expected_digest_hex && expected_digest_hex[0] != '\0'); | |
| if ((err = turbo_shake_init(&md, bits_count)) != CRYPT_OK) return err; | |
| offset = 0; | |
| rem = input_bytes_count; | |
| do | |
| { | |
| count = rem < ((long)(sizeof(input))) ? rem : ((long)(sizeof(input))); | |
| if ((err = s_turbo_shake_generate_ptn(&input[0], offset, count)) != CRYPT_OK) return err; | |
| if ((err = turbo_shake_process(&md, &input[0], count)) != CRYPT_OK) return err; | |
| offset += count; | |
| rem -= count; | |
| }while(rem != 0); | |
| rem = skip_digest_bytes; | |
| do | |
| { | |
| count = rem < ((long)(sizeof(digest))) ? rem : ((long)(sizeof(digest))); | |
| if ((err = turbo_shake_done(&md, &digest[0], count)) != CRYPT_OK) return err; | |
| rem -= count; | |
| }while(rem != 0); | |
| rem = digest_bytes_count; | |
| expected_hex = expected_digest_hex; | |
| do | |
| { | |
| count = rem < ((long)(sizeof(digest))) ? rem : ((long)(sizeof(digest))); | |
| if ((err = s_turbo_shake_hex_to_bin(&expected_hex[0], &expected_digest_bin[0], count)) != CRYPT_OK) return err; | |
| if ((err = turbo_shake_done(&md, &digest[0], count)) != CRYPT_OK) return err; | |
| LTC_COMPARE_TESTVECTOR(digest, count, expected_digest_bin, count, "TurboSHAKE", counter); | |
| rem -= count; | |
| expected_digest_hex += count * 2; | |
| }while(rem != 0); | |
| return CRYPT_OK; | |
| int err; | |
| hash_state md; | |
| unsigned long offset, rem, count; | |
| unsigned char input[1024]; | |
| unsigned char digest[64]; | |
| char const *expected_hex; | |
| unsigned char expected_digest_bin[sizeof(digest)]; | |
| LTC_ARGCHK(bits_count == 128 || bits_count == 256); | |
| LTC_ARGCHK(input_bytes_count >= 0); | |
| LTC_ARGCHK(skip_digest_bytes >= 0); | |
| LTC_ARGCHK(digest_bytes_count >= 1); | |
| LTC_ARGCHK(counter >= 0); | |
| LTC_ARGCHK(expected_digest_hex && expected_digest_hex[0] != '\0'); | |
| if ((err = turbo_shake_init(&md, bits_count)) != CRYPT_OK) return err; | |
| offset = 0; | |
| rem = input_bytes_count; | |
| do { | |
| count = rem < sizeof(input) ? rem : sizeof(input); | |
| if ((err = s_turbo_shake_generate_ptn(input, offset, count)) != CRYPT_OK) return err; | |
| if ((err = turbo_shake_process(&md, input, count)) != CRYPT_OK) return err; | |
| offset += count; | |
| rem -= count; | |
| } while(rem != 0); | |
| rem = skip_digest_bytes; | |
| do { | |
| count = rem < sizeof(digest) ? rem : sizeof(digest); | |
| if ((err = turbo_shake_done(&md, digest, count)) != CRYPT_OK) return err; | |
| rem -= count; | |
| } while(rem != 0); | |
| rem = digest_bytes_count; | |
| expected_hex = expected_digest_hex; | |
| do { | |
| count = rem < sizeof(digest) ? rem : sizeof(digest); | |
| if ((err = s_turbo_shake_hex_to_bin(expected_hex, expected_digest_bin, count)) != CRYPT_OK) return err; | |
| if ((err = turbo_shake_done(&md, digest, count)) != CRYPT_OK) return err; | |
| LTC_COMPARE_TESTVECTOR(digest, count, expected_digest_bin, count, "TurboSHAKE", counter); | |
| rem -= count; | |
| expected_digest_hex += count * 2; | |
| } while(rem != 0); | |
| return CRYPT_OK; |
Three more things I just saw, which I also missed in the first two reviews 😬 (I should focus more, but it's a lot of changes, so it's hard to do that while also doing my regular work).
- we usually use the nearest correct type, so for sizes it should be
unsigned longmost of the time. (The correct type would besize_t, but that move didn't happen yet...) - we try to use the implicit meaning of statements. If you use
&foo[0]in an API call, wherefoois an array of something and the API expects a pointer, simply usefoothere. Keep the usage of&foo[n]for the cases where you explicitly index into the array. It makes reading the code easier. - Try to mimick the code style used in the recent changes. I know, we should have an auto-formatter, but that's something for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed.
cb1e0d8 to
f588b38
Compare
f588b38 to
a89025e
Compare
KangarooTwelve 128bits and KangarooTwelve 256 bits. https://keccak.team/files/TurboSHAKE.pdf https://www.rfc-editor.org/rfc/rfc9861.txt https://datatracker.ietf.org/doc/html/rfc9861
a89025e to
82f13dd
Compare
Checklist