Thread safety

[sdrsdr]

The code uses global variables for key/iv this is horribly thread unsafe! I know this is supposed to be mcu implementation but yet passing arguments around should not be that big of an issue. At least the there could be an -DTHREADSAFE option to compile a thread safe version of this code.

[kokke]

Hi @sdrsdr - Thanks for taking an interest in this project and for pointing to this issue :)

Others have already mentioned this design flaw:

#62
#9

I still haven't checked to see how big a difference it makes for the binary output.
I suspect it makes a big difference on 8-bit platforms, but have not tested yet.

You should be able to fetch a thread safe version from one of the forks of this project to get an immediate fixed and thread-safe version.

[kokke]

How do you suggest the -DTHREADSAFE flag should be used?

It has to change the API as I see it - I don't like a macro that changes all of the public API.

I think the clean solution is to always parameterize the key/IV (and counter in CTR-mode) and pass along with every call. I think that is the solution in #9 .

[sdrsdr]

It can add the used global variables as parameter to the functions that uses them and see that they get called properly .. I'll have a PR ready in some days ..

[sdrsdr]

it will be ugly but it will allow for not wasting stack space on 8 bit platforms

[sdrsdr]

Did you like it this way? Shall I make a pull request?

omg@noah ~/projects/tiny-AES-c $ make clean
omg@noah ~/projects/tiny-AES-c $ make CFLAGS=-DTHREAD_SAFE --trace
Makefile:26: update target 'aes.o' due to: aes.h aes.c
# compiling aes.c
gcc -DTHREAD_SAFE -c aes.c -o aes.o
Makefile:22: update target 'test.o' due to: test.c
# compiling test.c
gcc -DTHREAD_SAFE -c test.c -o test.o
Makefile:30: update target 'test.out' due to: aes.o test.o
# linking object code to binary
gcc -DTHREAD_SAFE aes.o test.o -o test.out
Makefile:18: update target 'rom.hex' due to: test.out
# copy object-code to new image and format in hex
objcopy -j .text -O ihex test.out rom.hex
omg@noah ~/projects/tiny-AES-c $ ./test.out 

Testing AES128

CBC encrypt: SUCCESS!
CBC decrypt: SUCCESS!
CTR encrypt: SUCCESS!
CTR decrypt: SUCCESS!
ECB decrypt: SUCCESS!
ECB encrypt: SUCCESS!
ECB encrypt verbose:

plain text:
6bc1bee22e409f96e93d7e117393172a
ae2d8a571e03ac9c9eb76fac45af8e51
30c81c46a35ce411e5fbc1191a0a52ef
f69f2445df4f9b17ad2b417be66c3710

key:
2b7e151628aed2a6abf7158809cf4f3c

ciphertext:
3ad77bb40d7a3660a89ecaf32466ef97
f5d3d58503b9699de785895a96fdbaaf
43b1cd7f598ece23881b00e3ed030688
7b0c785e27e8ad3f8223207104725dd4


omg@noah ~/projects/tiny-AES-c $ make clean
omg@noah ~/projects/tiny-AES-c $ make --trace
Makefile:26: update target 'aes.o' due to: aes.h aes.c
# compiling aes.c
gcc -Wall -Os -Wl,-Map,test.map -c aes.c -o aes.o
Makefile:22: update target 'test.o' due to: test.c
# compiling test.c
gcc -Wall -Os -Wl,-Map,test.map -c test.c -o test.o
Makefile:30: update target 'test.out' due to: aes.o test.o
# linking object code to binary
gcc -Wall -Os -Wl,-Map,test.map aes.o test.o -o test.out
Makefile:18: update target 'rom.hex' due to: test.out
# copy object-code to new image and format in hex
objcopy -j .text -O ihex test.out rom.hex
omg@noah ~/projects/tiny-AES-c $ ./test.out 

Testing AES128

CBC encrypt: SUCCESS!
CBC decrypt: SUCCESS!
CTR encrypt: SUCCESS!
CTR decrypt: SUCCESS!
ECB decrypt: SUCCESS!
ECB encrypt: SUCCESS!
ECB encrypt verbose:

plain text:
6bc1bee22e409f96e93d7e117393172a
ae2d8a571e03ac9c9eb76fac45af8e51
30c81c46a35ce411e5fbc1191a0a52ef
f69f2445df4f9b17ad2b417be66c3710

key:
2b7e151628aed2a6abf7158809cf4f3c

ciphertext:
3ad77bb40d7a3660a89ecaf32466ef97
f5d3d58503b9699de785895a96fdbaaf
43b1cd7f598ece23881b00e3ed030688
7b0c785e27e8ad3f8223207104725dd4

omg@noah ~/projects/tiny-AES-c $

[kokke]

Hi again Stoian,

I really like that you've kept compatibility - but I think I will just go all the way: define a context-struct to store IV, key (counter if CTR) etc. - and then let the 8-bit people optimize the project for single-thread/session usage.

I.e. changing the API to something like AES_CBC_encrypt_buffer(struct aes* ctx, ...).

That change should also make it easier to handle buffered input in CBC-mode.
The current design is also useless for multi-session use as well.

Thanks for sharing your changes. I will take a rain check on the PR, but will have a look at refactoring the code sometime soon hopefully.

[kokke]

The current implementation of CTR-mode is also just kind of thrown in there. Currently it uses a stack-local copy of the IV/nonce. It should overwrite the input-variable nonce instead of keeping a copy, to keep up with the current destructive behavior of the other modes ;)

I would like to fix that in a change where the whole context is parameterized as mentioned in my comment above.

[namreeb]

As a user, let me just say that the introduction of a context struct for purposes of thread safety would be awesome.

[kokke]

Hi @namreeb - thanks for being so active in the issues and PR sections :) you've been contributing steadily for years now.

I don't think you would be the only user to salute the change. I just have a lot of irons in the fire - as usual - so I fear it may take a while before seeing it through, #9 has done most of the work really - I would do it almost exactly like that - but it doesn't merge cleanly any more....

[namreeb]

If I can fix the merge errors?

[kokke]

Yes then I'll merge it - with a single suggested change to #9 :

I prefer a struct instead of a typedef'd type, so the API becomes AES_CBC_encrypt_buffer(struct aes, ...) instead of AES_CBC_encrypt_buffer(aes_t, ...). I think custom structs are more generally accepted when they are not typedef'd

[kokke]

I may also have a preference to something cosmetic, but I think I can edit code in a PR, online/through the browser accepting. @namreeb I would gladly accept another of your contributions :)

[namreeb]

I spoke too soon. There have been extensive changes. I was hoping it would be a few obvious conflicts. I don't think will have time for this. Sorry :(

[kokke]

That was what I feared as well. I'll take a look at it sometime. Thanks for the effort though :]

[DamonHD]

Again, maybe there's stuff that we can help with from OTAESGCM in terms of workspace management. (There's a layer above in another library of stuff that you can't see there.) And it is running on an 8-bit MCU. Our implementation is NOT perfect, but there may be useful stuff there for you.

Rgds

Damon

[kokke]

@DamonHD besides our parallel discussion about GCM, what are you thinking about management-wise? Keys and IVs and such? Can you give me a hint for a filename or a structure specifically to look for :) ? 👍

[sdrsdr]

If you're ok with changing the public API I can rework the code even easily :)

[kokke]

Hi @sdrsdr

I wouldn't mind a change as proposed and discussed with @namreeb above in #75 (comment)

Thanks for offering your help :)

[kokke]

I'm always willing to let other people fix my code ;)

[sdrsdr]

BTW there is API calling convention mismatch AES_ECB_* are called with argument input first and all the rest are with output first can I make output first everywhere like in stdlib's memcpy/memmove, Considering nobody should be using ECB and this is major API change it might be OK time to do it

[sdrsdr]

And as we're going this way, can we make all operations "in-place" I see no point in copy input to output then ignore the input .. this way the functions themself will require half the ram ..

[sdrsdr]

also AES_ECB_encrypt/AES_ECB_decrypt does not use length in encryption itself .. only to copy input to output then decode the first 16 bytes of it .. assuming there are 16 bytes... shall we remove this param so it is more obvious that we're handling single block only?

[sdrsdr]

I'm not sure if ECB function will work with sizes not multiple of BLOCKLEN. Also the "almost working" zero padding is waste of time as there are more robust paddings as PKCS#7 (https://en.wikipedia.org/wiki/Padding_(cryptography)#PKCS7) that can be reliably reversed for any input data unlike the proposed not working zeropad

[kokke]

ECB only works for size = BLOCKLEN - it's fine with me if that is explicit in the code.
Sure you are free to make all operations in-place - I think they are currently done in-place, but in the output-buffer, no?
I think it should be up to the user to handle the padding, really. I only implemented the zero-padding because I got a lot of requests for handling the case where length is not a multiple of BLOCKLEN.

[sdrsdr]

https://github.com/sdrsdr/tiny-AES-c/tree/newapi see where this is headding..

[sdrsdr]

It is 3 am here .. tomorrow I'll switch it to in-place, the only "problem" being CBC decryption where input matters .. I'll update the test also as it needs some copy to handle in-place encryption

[DamonHD]

@kokke Maybe take a look in the first instance at OTAES128GCMGenericWithWorkspace. As I say there's a prettier layer on top, but we wanted to keep this really simple without needing header dependencies with our other libraries.

[kokke]

Hi @sdrsdr and thanks again for your great effort :)

I just checked and the code-size for ARM has even gone down with the new API.

Closing issue after merging #76