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.

Sign up for GitHub

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

Jump to bottom

crypto: Add Galois/Counter Mode (GCM) support #22407

Merged

jhedberg merged 3 commits into zephyrproject-rtos:master from mrfuchs:crypto-gcm

Feb 8, 2020

Contributor

mrfuchs commented Feb 2, 2020

Add support for AES Galois/Counter Mode of Operation to the Crypto API,
implement GCM in the mbed TLS shim driver and show its usage in the
crypto sample project.

[00:00:05.269,000] <inf> main: Cipher Sample
[00:00:05.270,000] <inf> main: ECB Mode
[00:00:05.270,000] <inf> main: Output length (encryption): 16
[00:00:05.270,000] <inf> main: ECB mode ENCRYPT - Match
[00:00:05.270,000] <inf> main: Output length (decryption): 16
[00:00:05.270,000] <inf> main: ECB mode DECRYPT - Match
[00:00:05.271,000] <inf> main: CBC Mode
[00:00:05.271,000] <inf> main: Output length (encryption): 80
[00:00:05.271,000] <inf> main: CBC mode ENCRYPT - Match
[00:00:05.271,000] <inf> main: Output length (decryption): 64
[00:00:05.271,000] <inf> main: CBC mode DECRYPT - Match
[00:00:05.272,000] <inf> main: CTR Mode
[00:00:05.272,000] <err> mbedtls: Unsupported mode
[00:00:05.272,000] <inf> main: CCM Mode
[00:00:05.272,000] <inf> main: Output length (encryption): 31
[00:00:05.272,000] <inf> main: CCM mode ENCRYPT - Match
[00:00:05.273,000] <inf> main: Output length (decryption): 31
[00:00:05.273,000] <inf> main: CCM mode DECRYPT - Match
[00:00:05.273,000] <inf> main: GCM Mode
[00:00:05.274,000] <inf> main: Output length (encryption): 58
[00:00:05.274,000] <inf> main: GCM mode ENCRYPT - Match
[00:00:05.279,000] <inf> main: Output length (decryption): 58
[00:00:05.279,000] <inf> main: GCM mode DECRYPT - Match

mrfuchs requested review from carlescufi, galak, MaureenHelm and nashif as code owners

February 2, 2020 21:35

zephyrbot added area: API area: Samples labels

Collaborator

zephyrbot commented Feb 2, 2020 •

edited

Loading

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

carlescufi requested review from tejlmand, ceolin and dleach02

February 2, 2020 21:40

mrfuchs force-pushed the crypto-gcm branch from f9ea714 to 7e8b60c Compare

February 2, 2020 22:09

mrfuchs mentioned this pull request

mbed TLS: Inconsistent Kconfig option names #22421

Closed

mrfuchs force-pushed the crypto-gcm branch 2 times, most recently from 15b54e8 to 7e8b60c Compare

February 3, 2020 20:29

ceolin reviewed

View reviewed changes

Member

ceolin left a comment

Looks ok for me, just a minor comment

drivers/crypto/crypto_mtls_shim.c

    
            @@ -21,6 +21,7 @@
          
              #endif /* CONFIG_MBEDTLS_CFG_FILE */

              #include <mbedtls/ccm.h>

              #include <mbedtls/gcm.h>

Member

ceolin Feb 3, 2020

is this header available when mbedTLS is built without gcm support ? Shouldn't you put this include under ifdef ?

Contributor Author

mrfuchs Feb 3, 2020

Yes, it's available - but you are right, it's probably better to include gcm.h conditionally. That would mean that the mbedtls_gcm_context in the session would also have to be surrounded by an ifdef, though. Will change it.

dleach02 reviewed

View reviewed changes

drivers/crypto/crypto_mtls_shim.c Outdated Show resolved Hide resolved

mrfuchs force-pushed the crypto-gcm branch from 7e8b60c to d79cb86 Compare

February 3, 2020 21:39

tejlmand reviewed

View reviewed changes

Collaborator

tejlmand left a comment

A couple of small comments, but generally looking good.

drivers/crypto/crypto_mtls_shim.c Outdated

    
              			LOG_ERR("Message authentication failed");

              			return -EFAULT;

              		}

              		LOG_ERR("Could non decrypt/auth (%d)", ret);

Collaborator

tejlmand Feb 3, 2020

I know you didn't touch this line of code, but I couldn't help to spot this spelling mistake, could you include in your fix ?

Suggested change

      
            		LOG_ERR("Could non decrypt/auth (%d)", ret);
          
            		LOG_ERR("Could not decrypt/auth (%d)", ret);

Contributor Author

mrfuchs Feb 4, 2020

This has been fixed.

drivers/crypto/crypto_mtls_shim.c

    
            @@ -325,6 +398,25 @@ static int mtls_session_setup(struct device *dev, struct cipher_ctx *ctx,
          
              			ctx->ops.ccm_crypt_hndlr = mtls_ccm_decrypt_auth;

              		}

              		break;

              	case CRYPTO_CIPHER_MODE_GCM:

              #ifdef CONFIG_MBEDTLS_CIPHER_MODE_GCM_ENABLED

Collaborator

tejlmand Feb 3, 2020

a test for CRYPTO_CIPHER_MODE_GCM is already done i L423, which means if CONFIG_MBEDTLS_CIPHER_MODE_GCM_ENABLED=n then code never reaches here when mode is CRYPTO_CIPHER_MODE_GCM, so any reason not to include the case CRYPTO_CIPHER_MODE_GCM: inside the #ifdef ?

Contributor Author

mrfuchs Feb 4, 2020

Okay. Should we add a default case to the switch statement then? I don't know, if all compilers and static code analysis tools will accept the previous mode check without complaining about unhandled case values.

Contributor Author

mrfuchs Feb 4, 2020 •

edited

Loading

This has been changed. As GCC issued a

warning: enumeration value 'CRYPTO_CIPHER_MODE_GCM' not handled in switch

I decided to add a default case. It also seems to be more future-proof this way when new modes are added to the Crypto API.

drivers/crypto/crypto_mtls_shim.c Outdated

    
              		ret = mbedtls_gcm_setkey(gcm_ctx, MBEDTLS_CIPHER_ID_AES,

              					 ctx->key.bit_stream, ctx->keylen * 8U);

              		if (ret) {

              			LOG_ERR("Could not setup the key (%d)", ret);

Collaborator

tejlmand Feb 4, 2020

could you follow same style as in:
https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/crypto/crypto_mtls_shim.c#L284
https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/crypto/crypto_mtls_shim.c#L305

Suggested change

      
            			LOG_ERR("Could not setup the key (%d)", ret);
          
            			LOG_ERR("AES_GCM: failed at setkey (%d)", ret);

Contributor Author

mrfuchs Feb 4, 2020

This has been changed. Thanks for spotting and the suggestion!

mrfuchs force-pushed the crypto-gcm branch from d79cb86 to 737995f Compare

February 4, 2020 16:08

ceolin approved these changes

View reviewed changes

mrfuchs force-pushed the crypto-gcm branch from 737995f to 1c1cab5 Compare

February 4, 2020 22:43

mrfuchs added 3 commits

February 7, 2020 22:11


          include: crypto: Add Galois/Counter Mode (GCM) support

c0b306a

Add support for AES Galois/Counter Mode of Operation to the Crypto API.

Signed-off-by: Markus Fuchs <markus.fuchs@de.sauter-bc.com>


          drivers: crypto: crypto_mtls_shim: Add AES-GCM support

c689ec2

Add support for AES Galois/Counter Mode (GCM) of operation to the
mbed TLS shim driver.

Signed-off-by: Markus Fuchs <markus.fuchs@de.sauter-bc.com>


          samples: crypto: Add AES-GCM sample

86e6989

Add sample for AES Galois/Counter Mode (GCM) of operation with a MACsec
GCM-AES test vector.
Also improve existing code by declaring expected ciphertext arrays as
constant.

Signed-off-by: Markus Fuchs <markus.fuchs@de.sauter-bc.com>

mrfuchs force-pushed the crypto-gcm branch from 1c1cab5 to 86e6989 Compare

February 7, 2020 21:14

jhedberg approved these changes

View reviewed changes

jhedberg merged commit 29630cc into zephyrproject-rtos:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dleach02 dleach02 left review comments

tejlmand tejlmand left review comments

jhedberg jhedberg approved these changes

ceolin ceolin approved these changes

carlescufi Awaiting requested review from carlescufi

galak Awaiting requested review from galak

MaureenHelm Awaiting requested review from MaureenHelm

nashif Awaiting requested review from nashif

Labels

area: API area: Samples