Implement constant time AES CBC #14

kristapsk · 2018-08-01T13:49:36Z

Tried to not change anything in existing code, except for making AES_encrypt() and AES_decrypt() public.

Related to #11.

Empact · 2018-11-18T20:01:48Z

ctaes.h

@@ -38,4 +38,7 @@ void AES256_init(AES256_ctx* ctx, const unsigned char* key32);
 void AES256_encrypt(const AES256_ctx* ctx, size_t blocks, unsigned char* cipher16, const unsigned char* plain16);
 void AES256_decrypt(const AES256_ctx* ctx, size_t blocks, unsigned char* plain16, const unsigned char* cipher16);

+void AES_encrypt(const AES_state* rounds, int nrounds, unsigned char* cipher16, const unsigned char* plain16);
+void AES_decrypt(const AES_state* rounds, int nrounds, unsigned char* plain16, const unsigned char* cipher16);


nit: perhaps better to declare these in the -cbc.c, as I take it's not a part of the public interface.

This could also be avoided by just merging this into ctaes.c, as it's a tiny amount of code, so I doubt any user would care. Having them in the same module may also enable better interprocedural optimization.

sipa

This looks very good. I'm very sorry I didn't notice this earlier (or maybe I did, and forgot).

I can't expect you're still interested in this after all this time, so if you aren't, or I don't get a response here, I'll address the comments myself.

sipa · 2020-08-14T22:48:56Z

ctaes-cbc.c

+    unsigned char buf[16];
+
+    for (i = 0; i < blocks; i++) {
+        for (j = 0; j < 16; j++) {


This could use memcpy.

sipa · 2020-08-14T22:49:07Z

ctaes-cbc.c

+        }
+        Xor128(buf, iv);
+        AES_encrypt(rounds, nk, encrypted, buf);
+        for (j = 0; j < 16; j++) {


Use memcpy.

sipa · 2020-08-14T22:49:36Z

ctaes-cbc.c

+    uint8_t next_iv[16];
+
+    for (i = 0; i < blocks; i++) {
+        for (j = 0; j < 16; j++) {


sipa · 2020-08-14T22:49:44Z

ctaes-cbc.c

+        }
+        AES_decrypt(rounds, nk, plain, encrypted);
+        Xor128(plain, iv);
+        for (j = 0; j < 16; j++) {


sipa · 2020-08-14T22:49:56Z

ctaes-cbc.c

+void AES128_CBC_init(AES128_CBC_ctx* ctx, const unsigned char* key16, const uint8_t* iv) {
+    size_t i;
+    AES128_init(&(ctx->ctx), key16);
+    for (i = 0; i < 16; i++) {


sipa · 2020-08-14T22:50:08Z

ctaes-cbc.c

+void AES192_CBC_init(AES192_CBC_ctx* ctx, const unsigned char* key16, const uint8_t* iv) {
+    size_t i;
+    AES192_init(&(ctx->ctx), key16);
+    for (i = 0; i < 16; i++) {


sipa · 2020-08-14T22:50:14Z

ctaes-cbc.c

+void AES256_CBC_init(AES256_CBC_ctx* ctx, const unsigned char* key16, const uint8_t* iv) {
+    size_t i;
+    AES256_init(&(ctx->ctx), key16);
+    for (i = 0; i < 16; i++) {


sipa · 2020-08-14T22:55:17Z

ctaes-cbc.h

+
+typedef struct {
+    AES128_ctx ctx;
+    uint8_t iv[16];


Perhaps document that this isn't exactly the IV (after the first block, it's the carried-forward previous ciphertext).

sipa · 2020-08-14T22:56:53Z

ctaes.h

@@ -38,4 +38,7 @@ void AES256_init(AES256_ctx* ctx, const unsigned char* key32);
 void AES256_encrypt(const AES256_ctx* ctx, size_t blocks, unsigned char* cipher16, const unsigned char* plain16);
 void AES256_decrypt(const AES256_ctx* ctx, size_t blocks, unsigned char* plain16, const unsigned char* cipher16);

+void AES_encrypt(const AES_state* rounds, int nrounds, unsigned char* cipher16, const unsigned char* plain16);
+void AES_decrypt(const AES_state* rounds, int nrounds, unsigned char* plain16, const unsigned char* cipher16);


This could also be avoided by just merging this into ctaes.c, as it's a tiny amount of code, so I doubt any user would care. Having them in the same module may also enable better interprocedural optimization.

kristapsk · 2020-08-15T15:56:28Z

Addressed review comments.

kristapsk · 2020-08-15T17:02:46Z

Just remembered why I choose to not use memcpy() back then - didn't want to introduce dependency on additional libc function and string.h. But it doesn't matter actually, as it is hard to think about any piece of software that will use this and not memcpy() anywhere.

sipa · 2020-08-15T17:05:37Z

Yeah, memcpy is C89, so it should be ~everywhere. It may also be easier to optimize for the compiler (using something faster than byte per byte copy).

Any reason to keep the CBC tests in a separate file?

kristapsk · 2020-08-15T17:16:12Z

memcpy is C89, so it should be ~everywhere.

It's not only about availability, if libc is implemented correct way, linker will not need to add code of unused functions in a final executable, so any extra function may increase size. I've done some embedded programming in the past, where sometimes every byte counts, so that thinking by default is from there. :) But memcpy is actually used by a lot of other libc stuff itself, so likely will be in almost any piece of software anyway.

Any reason to keep the CBC tests in a separate file?

Guess not. Will move them to test.c.

kristapsk · 2020-08-15T17:28:20Z

Guess not. Will move them to test.c.

Done.

sipa · 2020-08-16T01:52:54Z

ctaes.h

@@ -26,6 +26,21 @@ typedef struct {
    AES_state rk[15];
 } AES256_ctx;

+typedef struct {
+    AES128_ctx ctx;
+    uint8_t iv[16]; // iv is updated after each use


Please use /* */ comments (// only exists in C++ and C99+).

gmaxwell · 2020-08-16T21:23:24Z

test.c

@@ -101,6 +129,51 @@ int main(void) {
            fail++;
        }
    }
+    for (i = 0; i < sizeof(ctaes_cbc_tests) / sizeof(ctaes_cbc_tests[0]); i++) {
+        const ctaes_cbc_test* test = &ctaes_cbc_tests[i];
+        unsigned char key[32], iv[16], plain[test->nblocks * 16], cipher[test->nblocks * 16], ciphered[test->nblocks * 16], deciphered[test->nblocks * 16];


No VLAs in C89.

gmaxwell · 2020-08-16T21:24:24Z

Generally looks good-- just the C99isms need to be fixed otherwise I think it's good to go, thanks for submitting this!

kristapsk · 2020-08-18T21:03:14Z

Addressed additional review comments. Tested compiling tests with --std=c89 -Wall.

sipa · 2020-08-18T21:37:49Z

ACK 8835446

gmaxwell · 2020-08-18T22:12:24Z

ACK 8835446

f0177f2 Reinstate endif comment (Pieter Wuille) Pull request description: This was inadvertently removed in #14. Top commit has no ACKs. Tree-SHA512: dafed550a7ddfaff331473b5469781247ea34bbcd7060bab71047dcb76665bd2e64498552e02882ffef41b955294ef5c3d3d6203c7252155728ffb746963b39d

sipa · 2020-08-18T22:54:19Z

I just realized that AES-CBC in practice also includes a padding scheme, and as that e.g. needs to be checked in constant time, it seems appropriate to also have that implemented here.

Unfortunately, it doesn't quite match the API implemented here very well. The problem is that if the padding is incorrect, the decryption has to be rejected, which doesn't work well with a mutable state that lets you decrypt initial blocks before checking the padding. In case where the amount of data is large, and you want to use it in a streaming fashion, there is little that can be done about that, but I'm not sure there is a need for that. Instead, there could just be a function that takes a const AES<n>_ctx , and an input and an output byte array, and does the whole thing at once.

@kristapsk Are you interested in tackling that as well, or would you like me to do that instead?

Before, the submodule pointed to https://github.com/digitalbitbox/ctaes/tree/cbc, which was a re-host of bitcoin-core/ctaes#14 before it was merged. It has since been merged. https://github.com/digitalbitbox/ctaes/tree/master is now at 8012b062ea4931f10cc2fd2075fddc3782a57ee4, which is up to date with upstream https://github.com/bitcoin-core/ctaes. The submodule is updated to this commit revision.

Empact reviewed Nov 18, 2018

View reviewed changes

sipa reviewed Aug 14, 2020

View reviewed changes

kristapsk force-pushed the CBC branch 2 times, most recently from 2890e41 to 4cb6dcc Compare August 15, 2020 15:55

kristapsk force-pushed the CBC branch from 4cb6dcc to a6a5d56 Compare August 15, 2020 17:27

sipa reviewed Aug 16, 2020

View reviewed changes

gmaxwell reviewed Aug 16, 2020

View reviewed changes

Implement constant time AES CBC

8835446

kristapsk force-pushed the CBC branch from a6a5d56 to 8835446 Compare August 18, 2020 21:02

sipa merged commit a1caf29 into bitcoin-core:master Aug 18, 2020

sipa mentioned this pull request Aug 18, 2020

Reinstate endif comment #19

Merged

sipa mentioned this pull request Aug 18, 2020

AES modes #11

Open

benma mentioned this pull request Jun 12, 2022

external: update the ctaes dependency BitBoxSwiss/bitbox02-firmware#927

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement constant time AES CBC #14

Implement constant time AES CBC #14

kristapsk commented Aug 1, 2018

Empact Nov 18, 2018

sipa Aug 14, 2020

sipa left a comment

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

sipa Aug 14, 2020

kristapsk commented Aug 15, 2020

kristapsk commented Aug 15, 2020

sipa commented Aug 15, 2020

kristapsk commented Aug 15, 2020 •

edited

Loading

kristapsk commented Aug 15, 2020

sipa Aug 16, 2020

gmaxwell Aug 16, 2020

gmaxwell commented Aug 16, 2020

kristapsk commented Aug 18, 2020

sipa commented Aug 18, 2020

gmaxwell commented Aug 18, 2020

sipa commented Aug 18, 2020 •

edited

Loading

Implement constant time AES CBC #14

Implement constant time AES CBC #14

Conversation

kristapsk commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristapsk commented Aug 15, 2020

kristapsk commented Aug 15, 2020

sipa commented Aug 15, 2020

kristapsk commented Aug 15, 2020 • edited Loading

kristapsk commented Aug 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Aug 16, 2020

kristapsk commented Aug 18, 2020

sipa commented Aug 18, 2020

gmaxwell commented Aug 18, 2020

sipa commented Aug 18, 2020 • edited Loading

kristapsk commented Aug 15, 2020 •

edited

Loading

sipa commented Aug 18, 2020 •

edited

Loading