Skip to content
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

aes: add initial AES module #2823

Merged
merged 2 commits into from
May 6, 2020
Merged

aes: add initial AES module #2823

merged 2 commits into from
May 6, 2020

Conversation

xobs
Copy link
Collaborator

@xobs xobs commented Apr 28, 2020

This adds initial support for an AES cipher module. This
implementation supports only a subset of AES modes, namely
ECB, CBC, and CTR modes. These are part of the TinyAES library.

Example usage adapted from the PyCrypto README for AES:

>>> import aesio
>>>
>>> key = b'Sixteen byte key'
>>> cipher = aesio.AES(key, aesio.MODE_ECB)
>>> output = bytearray(16)
>>> cipher.encrypt_into(b'Circuit Python!!', output)
>>> output
bytearray(b'E\x14\x85\x18\x9a\x9c\r\x95>\xa7kV\xa2`\x8b\n')
>>>

This key is 16-bytes, so it uses AES128. If your key is 24- or 32-
bytes long, it will switch to AES192 or AES256 respectively.

This adds additional functions to keep the number of reallocations to a minimum. For example, rekey() can be used to reset a CBC stream or set a new IV. There are also functions to perform operations on existing buffers:

>>> out_buffer = bytearray(16)
>>> in_buffer = cipher.encrypt(b'Circuit Python!!')
>>> in_buffer
bytearray(b'E\x14\x85\x18\x9a\x9c\r\x95>\xa7kV\xa2`\x8b\n')
>>> cipher.decrypt_into(in_buffer, out_buffer)
>>> out_buffer
bytearray(b'Circuit Python!!')
>>>

This has been tested with many of the official NIST test vectors,
such as those used in pycryptodome at
https://github.com/Legrandin/pycryptodome/tree/39626a5b01ce5c1cf51d022be166ad0aea722177/lib/Crypto/SelfTest/Cipher/test_vectors/AES

CTR has not been tested as NIST does not provide test vectors for it.

@xobs xobs force-pushed the crypto-aes branch 4 times, most recently from 3144e9f to 617afae Compare April 28, 2020 11:39
@xobs
Copy link
Collaborator Author

xobs commented Apr 28, 2020

Note that this uses the capitalised module name Crypto to be compatible with the pycrypto module at https://www.dlitz.net/software/pycrypto/api/current/Crypto.Cipher.AES-module.html

I've added documentation, but I'm not sure if I documented it correctly.

@ladyada
Copy link
Member

ladyada commented Apr 28, 2020

@brentru fyi, maybe there's other crypto algs we could add that you needed? https://www.dlitz.net/software/pycrypto/api/current/Crypto-module.html

@Dar-Scott
Copy link

The protocol used by the Covid Watch anonymous exposure notification app to fight Covid-19 uses SHA256 and Ed25519 sig/ver. If a "kid-mode" of the app is implemented in CLUE or CircuitPlayground Bluefruit, those would be handy. I don't know what would be needed to implement the Apple/Google protocol; that might be a little different.

@tannewt
Copy link
Member

tannewt commented Apr 28, 2020

Note that this uses the capitalised module name Crypto to be compatible with the pycrypto module at https://www.dlitz.net/software/pycrypto/api/current/Crypto.Cipher.AES-module.html

I've added documentation, but I'm not sure if I documented it correctly.

Let's not match pycrypto at the package level. Lets only have the aes module API match. That way we can turn on and off crypto algorithms at the module level. So, the cross-platform import would be:

import sys
if sys.implementation.name == "circuitpython":
  import aes as AES
else:
  from Crypto.Cipher import AES

It'd be awesome to have port-specific implementations that use hardware peripherals for the computation too. This is a great start so everything can have it.

@tannewt tannewt self-requested a review April 28, 2020 19:12
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The doc style looks right to me. Just a couple API comments.

//| :class:`AESCipher` -- Encrypt and decrypt AES streams
//| =====================================================
//|
//| An object that decodes MP3 files for playback on an audio device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this.

//| ==================================================================
//|
//| .. module:: Crypto.Cipher
//| :synopsis: Embedded implementation of PyCrypto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to mimic pycrypto. I got the wrong python module. (PyCrypto was last released in 2013.) It looks like cryptography is the recommended package: https://docs.python-guide.org/scenarios/crypto/

Instead, I think we want our own APIs that only feature encrypt_into and decrypt_into that both take two arguments, the source and destination buffers. They can be the same or different. Omitting the methods that return new memory means folks are less likely to generate a bunch of garbage they don't want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an API reference for that library? I used to have a simpler api with an encrypt() and decrypt() call, but I threw that away to create API compatibility with PyCrypto.

What should the API look like when it comes to instantiation, key and IV management, and actual usage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! It is here: https://cryptography.io/en/latest/

It's not super clear to me how you arbitrarily assemble pieces from the hazmat layer.

@hierophect
Copy link
Collaborator

@tannewt I'm interested in this idea of port-level implementations. A lot of the STM32s have optional Crypto processors as a sort of mini-upgrade to the chip - the F415 is one such upgrade to the F405, with AES support.

@xobs
Copy link
Collaborator Author

xobs commented Apr 30, 2020

@hierophect some parts in the nRF52 family, for example, also contain crypto accelerators for certain primitives. For example, AES128.

@tannewt that new library looks like it would be complicated to implement. They decouple modes from algorithms from encryptors from backends. The best example appears to be from https://cryptography.io/en/latest/hazmat/primitives/symmetric-encryption/#cryptography.hazmat.primitives.ciphers.Cipher:

>>> import os
>>> from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes
>>> from cryptography.hazmat.backends import default_backend
>>> backend = default_backend()
>>> key = os.urandom(32)
>>> iv = os.urandom(16)
>>> cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=backend)
>>> encryptor = cipher.encryptor()
>>> ct = encryptor.update(b"a secret message") + encryptor.finalize()
>>> decryptor = cipher.decryptor()
>>> decryptor.update(ct) + decryptor.finalize()
b'a secret message'

@hathach
Copy link
Member

hathach commented Apr 30, 2020

I chime in a bit, since I have just added an Arduino library supported for ARM CryptoCell CC310 on nrf52840. We should use CC310 instead of AES for nrf52840, since AES (on all nrf52) is shard resource with SoftDevice, CC310 subsystem is separated and available to use, it is more powerful and could do lost of crypto with arm/nordic a.lib . I have used it for computing the ECC DH for LESC pairing.

https://github.com/adafruit/Adafruit_nRFCrypto
https://infocenter.nordicsemi.com/topic/ps_nrf52840/cryptocell.html?cp=4_0_0_5_5

image

@hathach
Copy link
Member

hathach commented Apr 30, 2020

PS: mbedtls and wolfssl does support CC310 on nrf52840 ( and other ARMs) as back-end for their API as well, we can just use them if needed. Sorry if I am out of context here for this PR :D

https://github.com/ARMmbed/mbed-os/tree/master/features/cryptocell/FEATURE_CRYPTOCELL310

https://github.com/wolfSSL/wolfssl/blob/master/wolfcrypt/src/port/arm/cryptoCell.chttps://github.com/wolfSSL/wolfssl/blob/master/wolfcrypt/src/port/arm/cryptoCell.c

@tannewt
Copy link
Member

tannewt commented Apr 30, 2020

@tannewt that new library looks like it would be complicated to implement.

Agree. Go ahead and do an API you think is best. (Remember that it shouldn't return new bytestrings because that forces an allocation.)

@xobs xobs force-pushed the crypto-aes branch 2 times, most recently from 3788e13 to 8760f11 Compare May 5, 2020 06:22
@xobs xobs changed the title crypto: add initial Crypto module with AESCipher aes: add initial AES module May 5, 2020
@xobs
Copy link
Collaborator Author

xobs commented May 5, 2020

I switched to using our own API under the aes namespace. As requested, I added functions such as decrypt_into() and encrypt_into() that will operate on a predefined buffer. I also added rekey() which can be used to set a new key or a new IV without reallocating the object.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! I think the module name should be aesio instead of aes to reduce the likelihood of conflicting with another library.

Is the in-place encryption a speed optimization or standard practice? I don't understand why it is preferred over a two-buffer option. I think we only need one of the two APIs, either in_place or into.

shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-bindings/aes/aes.c Outdated Show resolved Hide resolved
shared-module/aes/__init__.c Outdated Show resolved Hide resolved
shared-module/aes/__init__.c Outdated Show resolved Hide resolved
xobs added 2 commits May 6, 2020 17:40
This adds initial support for an AES module named aesio.  This
implementation supports only a subset of AES modes, namely
ECB, CBC, and CTR modes.

Example usage:

```
>>> import aesio
>>>
>>> key = b'Sixteen byte key'
>>> cipher = aesio.AES(key, aesio.MODE_ECB)
>>> output = bytearray(16)
>>> cipher.encrypt_into(b'Circuit Python!!', output)
>>> output
bytearray(b'E\x14\x85\x18\x9a\x9c\r\x95>\xa7kV\xa2`\x8b\n')
>>>
```

This key is 16-bytes, so it uses AES128.  If your key is 24- or 32-
bytes long, it will switch to AES192 or AES256 respectively.

This has been tested with many of the official NIST test vectors,
such as those used in `pycryptodome` at
https://github.com/Legrandin/pycryptodome/tree/39626a5b01ce5c1cf51d022be166ad0aea722177/lib/Crypto/SelfTest/Cipher/test_vectors/AES

CTR has not been tested as NIST does not provide test vectors for it.

Signed-off-by: Sean Cross <sean@xobs.io>
This is the result of running `make translate` after creating aesio.

Signed-off-by: Sean Cross <sean@xobs.io>
@xobs
Copy link
Collaborator Author

xobs commented May 6, 2020

Changes:

  • Rebased onto current master
  • Removed non-_into routines (encrypt, decrypt, encrypt_in_place, and decrypt_in_place)
  • Renamed to aesio
  • Fixed documentation as requested
  • Removed checks from the shared-modules
  • Made mode a property
  • Moved checks into their own function
  • Enable variable-length CTS operations (previously was limited to % 16 bytes)

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you!

I think we'll want to move the memcpy into shared-modules so common_hals are given both buffers but we can do that change later.

@tannewt tannewt merged commit 241ef52 into adafruit:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants