Skip to content

Make PSA Crypto structs actually opaque #4

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

Closed
wants to merge 6 commits into from
Closed

Make PSA Crypto structs actually opaque #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4efc888
Don't use global includes for library
Patater Dec 21, 2018
5b78b22
Move internal headers to library/include
Patater Dec 20, 2018
28284c8
Move STATIC_ASSERT_EXPR to utils.h
Patater Dec 21, 2018
3c09f55
Add a static assert macro
Patater Dec 21, 2018
847f171
Make crypto_struct internal
Patater Dec 21, 2018
e1fbc05
Make crypto structs opaque
Patater Dec 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ else()
endif()

include_directories(include/)
include_directories(library/)

if(ENABLE_ZLIB_SUPPORT)
find_package(ZLIB)
Expand Down
147 changes: 6 additions & 141 deletions include/psa/crypto_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* include psa/crypto.h.
*
* This file contains the definitions of some data structures with
* implementation-specific definitions.
* implementation-specific sizes.
*
* In implementations with isolation between the application and the
* cryptography module, it is expected that the front-end and the back-end
Expand Down Expand Up @@ -35,167 +35,32 @@
#ifndef PSA_CRYPTO_STRUCT_H
#define PSA_CRYPTO_STRUCT_H

/* Include the Mbed TLS configuration file, the way Mbed TLS does it
* in each of its header files. */
#if !defined(MBEDTLS_CONFIG_FILE)
#include "../mbedtls/config.h"
#else
#include MBEDTLS_CONFIG_FILE
#endif

#include "mbedtls/cipher.h"
#include "mbedtls/cmac.h"
#include "mbedtls/gcm.h"
#include "mbedtls/md.h"
#include "mbedtls/md2.h"
#include "mbedtls/md4.h"
#include "mbedtls/md5.h"
#include "mbedtls/ripemd160.h"
#include "mbedtls/sha1.h"
#include "mbedtls/sha256.h"
#include "mbedtls/sha512.h"

struct psa_hash_operation_s
{
psa_algorithm_t alg;
union
{
unsigned dummy; /* Make the union non-empty even with no supported algorithms. */
#if defined(MBEDTLS_MD2_C)
mbedtls_md2_context md2;
#endif
#if defined(MBEDTLS_MD4_C)
mbedtls_md4_context md4;
#endif
#if defined(MBEDTLS_MD5_C)
mbedtls_md5_context md5;
#endif
#if defined(MBEDTLS_RIPEMD160_C)
mbedtls_ripemd160_context ripemd160;
#endif
#if defined(MBEDTLS_SHA1_C)
mbedtls_sha1_context sha1;
#endif
#if defined(MBEDTLS_SHA256_C)
mbedtls_sha256_context sha256;
#endif
#if defined(MBEDTLS_SHA512_C)
mbedtls_sha512_context sha512;
#endif
} ctx;
uint8_t reserved[217];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Blocker: the fact that the RAM size depends on the configuration is a feature, not a bug. We do want to save RAM when certain algorithms are not used. For example, when people select SHA256 over SHA512, they expect to save RAM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, understood that it's desirable to save RAM with certain configurations. However, the exact mechanism of how the size changes based on the configuration should not be exposed outside the implementation. The current suggested implementation (as present in this PR) is a compromise, preferring not to leak implementation details over the ability to save RAM. Suggestions welcome as to how to allow users to allocate structures statically without leaking the exact mechanism of how the size changes based on the configuration.

My current best idea is that this would require a more advanced configuration system, where the configuration creates a define for the minimum necessary context size based on your configuration. Creating such an advanced configuration system would be out of scope of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The mechanism should not be exposed, but the values need to be exposed. We have a product management decision to keep RAM usage down, so increasing the RAM usage is only acceptable if there is a strong advantage. Making backward compatibility slightly more robust against misuse is not a good enough advantage for a library whose primary target is microcontrollers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just about being more robust against misuse, but making it easier to test for API and ABI breakage, being more clear about the API is, which headers we ship with library files (via make install), and so forth. RAM usage is a big deal, so this needs to be properly balanced.

I'll have a think as to how else we could get the benefits of statically allocatable opaque structs without increasing RAM requirements for minimal configurations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do all compilers guarantee maximum alignment for a struct that only contains byte-aligned fields in all circumstances? This is not explicitly guaranteed by the C standard. It's often indirectly implied by the ability to make a struct opaque, but a whole-program optimizer is allowed to notice that, after suitable application of as-if rules, the struct is never made opaque, and therefore it can be aligned any old way.

};

#if defined(MBEDTLS_MD_C)
typedef struct
{
/** The hash context. */
struct psa_hash_operation_s hash_ctx;
/** The HMAC part of the context. */
uint8_t opad[PSA_HMAC_MAX_HASH_BLOCK_SIZE];
} psa_hmac_internal_data;
#endif /* MBEDTLS_MD_C */

struct psa_mac_operation_s
{
psa_algorithm_t alg;
unsigned int key_set : 1;
unsigned int iv_required : 1;
unsigned int iv_set : 1;
unsigned int has_input : 1;
unsigned int is_sign : 1;
uint8_t mac_size;
union
{
unsigned dummy; /* Make the union non-empty even with no supported algorithms. */
#if defined(MBEDTLS_MD_C)
psa_hmac_internal_data hmac;
#endif
#if defined(MBEDTLS_CMAC_C)
mbedtls_cipher_context_t cmac;
#endif
} ctx;
uint8_t reserved[353];
};

struct psa_cipher_operation_s
{
psa_algorithm_t alg;
unsigned int key_set : 1;
unsigned int iv_required : 1;
unsigned int iv_set : 1;
uint8_t iv_size;
uint8_t block_size;
union
{
mbedtls_cipher_context_t cipher;
} ctx;
uint8_t reserved[97];
};

#if defined(MBEDTLS_MD_C)
typedef struct
{
uint8_t *info;
size_t info_length;
psa_hmac_internal_data hmac;
uint8_t prk[PSA_HASH_MAX_SIZE];
uint8_t output_block[PSA_HASH_MAX_SIZE];
#if PSA_HASH_MAX_SIZE > 0xff
#error "PSA_HASH_MAX_SIZE does not fit in uint8_t"
#endif
uint8_t offset_in_block;
uint8_t block_number;
} psa_hkdf_generator_t;
#endif /* MBEDTLS_MD_C */

#if defined(MBEDTLS_MD_C)
typedef struct psa_tls12_prf_generator_s
{
/* The TLS 1.2 PRF uses the key for each HMAC iteration,
* hence we must store it for the lifetime of the generator.
* This is different from HKDF, where the key is only used
* in the extraction phase, but not during expansion. */
unsigned char *key;
size_t key_len;

/* `A(i) + seed` in the notation of RFC 5246, Sect. 5 */
uint8_t *Ai_with_seed;
size_t Ai_with_seed_len;

/* `HMAC_hash( prk, A(i) + seed )` in the notation of RFC 5246, Sect. 5. */
uint8_t output_block[PSA_HASH_MAX_SIZE];

#if PSA_HASH_MAX_SIZE > 0xff
#error "PSA_HASH_MAX_SIZE does not fit in uint8_t"
#endif

/* Indicates how many bytes in the current HMAC block have
* already been read by the user. */
uint8_t offset_in_block;

/* The 1-based number of the block. */
uint8_t block_number;

} psa_tls12_prf_generator_t;
#endif /* MBEDTLS_MD_C */

struct psa_crypto_generator_s
{
psa_algorithm_t alg;
size_t capacity;
union
{
struct
{
uint8_t *data;
size_t size;
} buffer;
#if defined(MBEDTLS_MD_C)
psa_hkdf_generator_t hkdf;
psa_tls12_prf_generator_t tls12_prf;
#endif
} ctx;
uint8_t reserved[497];
};

#define PSA_CRYPTO_GENERATOR_INIT {0, 0, {{0, 0}}}
#define PSA_CRYPTO_GENERATOR_INIT {0, 0, {0}}
static inline struct psa_crypto_generator_s psa_crypto_generator_init( void )
{
const struct psa_crypto_generator_s v = PSA_CRYPTO_GENERATOR_INIT;
Expand Down
2 changes: 2 additions & 0 deletions library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ if(USE_STATIC_MBEDTLS_LIBRARY)
target_link_libraries(${mbedcrypto_static_target} ${libs})
target_include_directories(${mbedcrypto_static_target}
PUBLIC ${CMAKE_SOURCE_DIR}/include/
PRIVATE ${CMAKE_SOURCE_DIR}/library/include/
PUBLIC ${CMAKE_SOURCE_DIR}/crypto/include/)

if(USE_CRYPTO_SUBMODULE)
Expand Down Expand Up @@ -177,6 +178,7 @@ if(USE_SHARED_MBEDTLS_LIBRARY)
target_link_libraries(mbedcrypto ${libs})
target_include_directories(mbedcrypto
PUBLIC ${CMAKE_SOURCE_DIR}/include/
PRIVATE ${CMAKE_SOURCE_DIR}/library/include/
PUBLIC ${CMAKE_SOURCE_DIR}/crypto/include/)

if(USE_CRYPTO_SUBMODULE)
Expand Down
2 changes: 1 addition & 1 deletion library/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CFLAGS ?= -O2
WARNING_CFLAGS ?= -Wall -W -Wdeclaration-after-statement
LDFLAGS ?=

CRYPTO_INCLUDES ?= -I../include
CRYPTO_INCLUDES ?= -I../include -Iinclude
LOCAL_CFLAGS = $(WARNING_CFLAGS) $(CRYPTO_INCLUDES) -D_FILE_OFFSET_BITS=64
LOCAL_LDFLAGS =

Expand Down
Loading