From abeb58e8145a09b83f20aeda2c8409dc64081b24 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Jul 2020 13:22:27 +0200 Subject: [PATCH 01/55] Add a directory for proposed specifications Signed-off-by: Gilles Peskine --- docs/.gitignore | 3 +++ docs/architecture/.gitignore | 2 -- docs/proposed/Makefile | 21 +++++++++++++++++++++ docs/proposed/README | 4 ++++ 4 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 docs/.gitignore delete mode 100644 docs/architecture/.gitignore create mode 100644 docs/proposed/Makefile create mode 100644 docs/proposed/README diff --git a/docs/.gitignore b/docs/.gitignore new file mode 100644 index 000000000..33ae5acf6 --- /dev/null +++ b/docs/.gitignore @@ -0,0 +1,3 @@ +*.html +*.pdf +!PSACryptoDriverModelSpec.pdf diff --git a/docs/architecture/.gitignore b/docs/architecture/.gitignore deleted file mode 100644 index 23f832b73..000000000 --- a/docs/architecture/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -*.html -*.pdf diff --git a/docs/proposed/Makefile b/docs/proposed/Makefile new file mode 100644 index 000000000..97f288ac8 --- /dev/null +++ b/docs/proposed/Makefile @@ -0,0 +1,21 @@ +PANDOC = pandoc + +default: all + +all_markdown = \ + # This line is intentionally left blank + +html: $(all_markdown:.md=.html) +pdf: $(all_markdown:.md=.pdf) +all: html pdf + +.SUFFIXES: +.SUFFIXES: .md .html .pdf + +.md.html: + $(PANDOC) -o $@ $< +.md.pdf: + $(PANDOC) -o $@ $< + +clean: + rm -f *.html *.pdf diff --git a/docs/proposed/README b/docs/proposed/README new file mode 100644 index 000000000..09eae9aec --- /dev/null +++ b/docs/proposed/README @@ -0,0 +1,4 @@ +The documents in this directory are proposed specifications for Mbed +TLS features. They are not implemented yet, or only partially +implemented. Please follow activity on the `development` branch of +Mbed TLS if you are interested in these features. From 2e66aca372ad00e0a2d943e923a8a3382805a239 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Jul 2020 11:27:13 +0200 Subject: [PATCH 02/55] PSA unified driver interface Working draft of the PSA cryptography unified interface specification. Eventually this document will be under Arm PSA architecture ownership, but for the time being this draft is maintained in Mbed TLS. Signed-off-by: Gilles Peskine --- docs/proposed/Makefile | 1 + docs/proposed/psa-driver-interface.md | 532 ++++++++++++++++++++++++++ 2 files changed, 533 insertions(+) create mode 100644 docs/proposed/psa-driver-interface.md diff --git a/docs/proposed/Makefile b/docs/proposed/Makefile index 97f288ac8..cf656dd79 100644 --- a/docs/proposed/Makefile +++ b/docs/proposed/Makefile @@ -3,6 +3,7 @@ PANDOC = pandoc default: all all_markdown = \ + psa-driver-interface.md \ # This line is intentionally left blank html: $(all_markdown:.md=.html) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md new file mode 100644 index 000000000..0e24ea3ec --- /dev/null +++ b/docs/proposed/psa-driver-interface.md @@ -0,0 +1,532 @@ +PSA Cryptoprocessor Driver Interface +==================================== + +This document describes an interface for cryptoprocessor drivers in the PSA cryptography API. This interface complements the PSA Cryptography API specification, which describes the interface between a PSA Cryptography implementation and an application. + +This specification is work in progress and should be considered to be in a beta stage. There is ongoing work to implement this interface in Mbed TLS, which is the reference implementation of the PSA Cryptography API. At this stage, Arm does not expect major changes, but minor changes are expected based on experience from the first implementation and on external feedback. + +Time-stamp: "2020/07/13 08:07:14 GMT" + +## Introduction + +### Purpose of the driver interface + +The PSA Cryptography API defines an interface that allows applications to perform cryptographic operations in a uniform way regardless of how the operations are performed. Under the hood, different keys may be processed in different hardware or in different logical partitions, and different algorithms may involve different hardware or software components. + +The driver interface allows implementations of the PSA Crypytography API to be built compositionally. An implementation of the PSA Cryptography API is composed of a **core** and zero or more **drivers**. The core handles key management, enforces key usage policies, and dispatches cryptographic operations either to the applicable driver or to built-in code. + +Functions in the PSA Cryptography API invoke functions in the core. Code from the core calls drivers as described in the present document. + +### Types of drivers + +The PSA Cryptography driver interface supports two types of cryptoprocessors, and accordingly two types of drivers. + +* **Transparent** drivers implement cryptographic operations on keys that are provided in cleartext at the beginning of each operation. They are typically used for hardware **accelerators**. When a transparent driver is available for a particular combination of parameters (cryptographic algorithm, key type and size, etc.), it is used instead of the default software implementation. Transparent drivers can also be pure software implementations that are distributed as plug-ins to a PSA Crypto implementation. +* **Opaque** drivers implement cryptographic operations on keys that can only be used inside a protected environment such as a **secure element**, a hardware security module, a smartcard, a secure enclave, etc. An opaque driver is invoked for the specific key location that the driver is registered for: the dispatch is based on the key's lifetime. + +### Requirements + +The present specification was designed to fulfil the following high-level requirements. + +[Req.plugins] It is possible to combine multiple drivers from different providers into the same implementation, without any prior arrangement other than choosing certain names and values from disjoint namespaces. + +[Req.compile] It is possible to compile the code of each driver and of the core separately, and link them together. A small amount of glue code may need to be compiled once the list of drivers is available. + +[Req.types] Support drivers for the following types of hardware: accelerators that operate on keys in cleartext; cryptoprocessors that can wrap keys with a built-in keys but not store user keys; and cryptoprocessors that store key material. + +[Req.portable] The interface between drivers and the core does not involve any platform-specific consideration. Driver calls are simple C functions. Interactions between driver code and hardware happen inside the driver (and in fact a driver need not involve any hardware at all). + +[Req.location] Applications can tell which location values correspond to which secure element drivers. + +[Req.fallback] Accelerator drivers can specify that they do not fully support a cryptographic mechanism and that a fallback to core code may be necessary. Conversely, if an accelerator fully supports cryptographic mechanism, the core does not need to include code for this mechanism. + +[Req.mechanisms] Drivers can specify which mechanisms they support. A driver's code will not be invoked for cryptographic mechanisms that it does not support. + +## Overview of drivers + +### Deliverables for a driver + +To write a driver, you need to implement some functions with C linkage, and to declare these functions in a **driver description file**. The driver description file declares which functions the driver implements and what cryptographic mechanisms they support. Depending on the driver type, you may also need to define some C types and macros in a header file. + +The concrete syntax for a driver description file is JSON. The structure of this JSON file is specified in the section [“Driver description syntax”](#driver-description-syntax). + +A driver therefore consists of: + +* A driver description file (in JSON format). +* C header files defining the types required by the driver description. The names of these header files is declared in the driver description file. +* An object file compiled for the target platform defining the functions required by the driver description. Implementations may allow drivers to be provided as source files and compiled with the core instead of being pre-compiled. + +How to provide the driver description file, the C header files and the object code is implementation-dependent. + +Implementations should support multiple drivers. + +### Driver description syntax + +The concrete syntax for a driver description file is JSON. + +#### Driver description top-level element + +A driver description is a JSON object containing the following properties: + +* `"prefix"` (mandatory, string). This must be a valid prefix for a C identifier. All the types and functions provided by the driver have a name that starts with this prefix unless overridden with a `"name"` element in the applicable capability as described below. +* `"type"` (mandatory, string). One of `"transparent"` or `"opaque"`. +* `"headers"` (optional, array of strings). A list of header files. These header files must define the types provided by the driver and may declare the functions provided by the driver. They may include other PSA headers and standard headers of the platform. Whether they may include other headers is implementation-specific. If omitted, the list of headers is empty. +* `"capabilities"` (mandatory, array of [capabilities](#driver-description-capability)). +A list of **capabilities**. Each capability describes a family of functions that the driver implements for a certain class of cryptographic mechanisms. +* `"key_context"` (not permitted for transparent drivers, mandatory for opaque drivers): information about the [representation of keys](#key-format-for-opaque-drivers). +* `"persistent_state_size"` (not permitted for transparent drivers, optional for opaque drivers, integer or string). The size in bytes of the [persistent state of the driver](#opaque-driver-persistent-state). This may be either a non-negative integer or a C constant expression of type `size_t`. +* `"location"` (not permitted for transparent drivers, optional for opaque drivers, integer or string). The location value for which this driver is invoked. This may be either a non-negative integer or a C constant expression of type `psa_key_location_t`. + +#### Driver description capability + +A capability declares a family of functions that the driver implements for a certain class of cryptographic mechanisms. The capability specifies which key types and algorithms are covered and the names of the types and functions that implement it. + +A capability is a JSON object containing the following properties: + +* `"functions"` (optional, list of strings). Each element is the name of a [driver function](#driver-functions) or driver function family. If specified, the core will invoke this capability of the driver only when performing one of the specified operations. If omitted, the `"algorithms"` property is mandatory and the core will invoke this capability of the driver for all operations that are applicable to the specified algorithms. The driver must implement all the specified or implied functions, as well as the types if applicable. +* `"algorithms"` (optional, list of strings). Each element is an [algorithm specification](#algorithm-specifications). If specified, the core will invoke this capability of the driver only when performing one of the specified algorithms. If omitted, the core will invoke this capability for all applicable algorithms. +* `"key_types"` (optional, list of strings). Each element is a [key type specification](#key-type-specifications). If specified, the core will invoke this capability of the driver only for operations involving a key with one of the specified key types. If omitted, the core will invoke this capability of the driver for all applicable key types. +* `"key_sizes"` (optional, list of integers). If specified, the core will invoke this capability of the driver only for operations involving a key with one of the specified key sizes. If omitted, the core will invoke this capability of the driver for all applicable key sizes. Key sizes are expressed in bits. +* `"names"` (optional, object). A mapping from function names described by the `"functions"` property, to the name of the C function in the driver that implements the corresponding function. If a function is not listed here, name of the driver function that implements it is the driver's prefix followed by an underscore (`_`) followed by the function name. If this property is omitted, it is equivalent to an empty object (so each function *suffix* is implemented by a function with called *prefix*`_`*suffix*). +* `"fallback"` (optional for transparent drivers, not permitted for opaque drivers, boolean). If present and true, the driver may return `PSA_ERROR_NOT_SUPPORTED`, in which case the core should call another driver or use built-in code to perform this operation. If absent or false, the core should not include built-in code to perform this particular cryptographic mechanism. + +Example: the following capability declares that the driver can perform deterministic ECDSA signatures using SHA-256 or SHA-384 with a SECP256R1 or SECP384R1 private key (with either hash being possible in combinatio with either curve). If the prefix of this driver is `"acme"`, the function that performs the signature is called `acme_sign_hash`. +``` +{ + "functions": ["sign_hash"], + "algorithms": ["PSA_ALG_DETERMINISTIC_ECDSA(PSA_ALG_SHA_256)", + "PSA_ALG_DETERMINISTIC_ECDSA(PSA_ALG_SHA_384)"], + "key_types": ["PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_CURVE_SECP_R1)"], + "key_sizes": [256, 384] +} +``` + +### Algorithm and key specifications + +#### Algorithm specifications + +An algorithm specification is a string consisting of a `PSA_ALG_xxx` macro that specifies a cryptographic algorithm defined by the PSA Cryptography API. If the macro takes arguments, the string must have the syntax of a C macro call and each argument must be an algorithm specification or a decimal or hexadecimal literal with no suffix, depending on the expected type of argument. + +Spaces are optional after commas. Whether other whitespace is permitted is implementation-specific. + +Valid examples: +``` +PSA_ALG_SHA_256 +PSA_ALG_HMAC(PSA_ALG_SHA_256) +PSA_ALG_KEY_AGREEMENT(PSA_ALG_ECDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)) +``` + +#### Key type specifications + +An algorithm specification is a string consisting of a `PSA_KEY_TYPE_xxx` macro that specifies a key type defined by the PSA Cryptography API. If the macro takes an argument, the string must have the syntax of a C macro call and each argument must be the name of a constant of suitable type (curve or group). + +The name `_` may be used instead of a curve or group to indicate that the capability concerns all curves or groups. + +Valid examples: +``` +PSA_KEY_TYPE_AES +PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_CURVE_SECP_R1) +PSA_KEY_TYPE_ECC_KEY_PAIR(_) +``` + +### Driver functions + +#### Overview of driver functions + +Drivers define functions, each of which implements an aspect of a capability of a driver, such as a cryptographic operation, a part of a cryptographic operation, or a key management action. Most driver functions correspond to a particular function in the PSA Cryptography API. For example, if a call to `psa_sign_hash()` is dispatched to a driver, it invokes the driver's `sign_hash` function. + +All driver functions return a status of type `psa_status_t` which should use the status codes documented for PSA services in general and for PSA Crypto in particular: `PSA_SUCCESS` indicates that the function succeeded, and `PSA_ERROR_xxx` values indicate that an error occurred. + +The signature of a driver function generally looks like the signature of the PSA Crypto API that it implements, with some modifications. This section gives an overview of modifications that apply to whole classes of functions. Refer to the reference section for each function or function family for details. + +* For functions that operate on an existing key, the `psa_key_id_t` parameter is replaced by a sequence of three parameters that describe the key: + 1. `const psa_key_attributes_t *attributes`: the key attributes. + 2. `const uint8_t *key_buffer`: a key material or key context buffer. + 3. `size_t key_buffer_size`: the size of the key buffer in bytes. + + For transparent drivers, the key buffer contains the key material, in the same format as defined for `psa_export_key()` and `psa_export_public_key()` in the PSA Cryptography API. For opaque drivers, the content of the key buffer is entirely up to the driver. + +* For functions that involve a multi-part operation, the operation state type (`psa_XXX_operation_t`) is replaced by a driver-specific operation state type (*prefix*`_XXX_operation_t`). + +Some functions are grouped in families that must be implemented as a whole. If a driver supports a function family, it must provide all the functions in the family. + +#### General considerations on driver function parameters + +Buffer parameters for driver functions obey the following conventions: + +* An input buffer has the type `const uint8_t *` and is immediately followed by a parameter of type `size_t` that indicates the buffer size. +* An output buffer has the type `uint8_t *` and is immediately followed by a parameter of type `size_t` that indicates the buffer size. A third parameter of type `size_t *` is provided to report the actual buffer size if the function succeeds. +* An in-out buffer has the type `uint8_t *` and is immediately followed by a parameter of type `size_t` that indicates the buffer size. Note that the buffer size does not change. + +Buffers of size 0 may be represented with either a null pointer or a non-null pointer. + +Input buffers and other input-only parameters (`const` pointers) may be in read-only memory. Overlap is possible between input buffers, and between an input buffer and an output buffer, but not between two output buffers or between a non-buffer parameter and another parameter. + +#### Driver functions for single-part cryptographic operations + +The following driver functions perform a cryptographic operation in one shot (single-part operation): + +* `"hash_compute"` (transparent drivers only): calculation of a hash. Called by `psa_hash_compute()` and `psa_hash_compare()`. To verify a hash with `psa_hash_compare()`, the core calls the driver's `"hash_compute"` function and compares the result with the reference hash value. +* `"mac_compute"`: calculation of a MAC. Called by `psa_mac_compute()` and possibly `psa_mac_verify()`. To verify a mac with `psa_mac_verify()`, the core calls an applicable driver's `"mac_verify"` function if there is one, otherwise the core calls an applicable driver's `"mac_compute"` function and compares the result with the reference MAC value. +* `"mac_verify"`: verification of a MAC. Called by `psa_mac_verify()`. This function is mainly useful for drivers of secure elements that verify a MAC without revealing the correct MAC. Although transparent drivers may implement this function in addition to `"mac_compute"`, it is generally not useful because the core can call the `"mac_compute"` function and compare with the expected MAC value. +* `"cipher_encrypt"`: unauthenticated symmetric cipher encryption. Called by `psa_cipher_encrypt()`. +* `"cipher_decrypt"`: unauthenticated symmetric cipher decryption. Called by `psa_cipher_decrypt()`. +* `"aead_encrypt"`: authenticated encryption with associated data. Called by `psa_aead_encrypt()`. +* `"aead_decrypt"`: authenticated decryption with associated data. Called by `psa_aead_decrypt()`. +* `"asymmetric_encrypt"`: asymmetric encryption. Called by `psa_asymmetric_encrypt()`. +* `"asymmetric_decrypt"`: asymmetric decryption. Called by `psa_asymmetric_decrypt()`. +* `"sign_hash"`: signature of an already calculated hash. Called by `psa_sign_hash()` and possibly `psa_sign_message()`. To sign a message with `psa_sign_message()`, the core calls an applicable driver's `"sign_message"` function if there is one, otherwise the core calls an applicable driver's `"hash_compute"` function followed by an applicable driver's `"sign_hash"` function. +* `"verify_hash"`: verification of an already calculated hash. Called by `psa_verify_hash()` and possibly `psa_verify_message()`. To verify a message with `psa_verify_message()`, the core calls an applicable driver's `"verify_message"` function if there is one, otherwise the core calls an applicable driver's `"hash_compute"` function followed by an applicable driver's `"verify_hash"` function. +* `"sign_message"`: signature of a message. Called by `psa_sign_message()`. +* `"verify_message"`: verification of a message. Called by `psa_verify_message()`. +* `"key_agreement"`: key agreement without a subsequent key derivation. Called by `psa_raw_key_agreement()` and possibly `psa_key_derivation_key_agreement()`. + +### Driver functions for multi-part operations + +#### General considerations on multi-part operations + +The functions that implement each step of a multi-part operation are grouped into a family. A driver that implements a multi-part operation must define all of the functions in this family as well as a type that represents the operation context. The lifecycle of a driver operation context is similar to the lifecycle of an API operation context: + +1. The core initializes operation context objects to either all-bits-zero or to logical zero (`{0}`), at its discretion. +1. The core calls the `xxx_setup` function for this operation family. If this fails, the core destroys the operation context object without calling any other driver function on it. +1. The core calls other functions that manipulate the operation context object, respecting the constraints. +1. If any function fails, the core calls the driver's `xxx_abort` function for this operation family, then destroys the operation context object without calling any other driver function on it. +1. If a “finish” function fails, the core destroys the operation context object without calling any other driver function on it. The finish functions are: *prefix*`_mac_sign_finish`, *prefix*`_mac_verify_finish`, *prefix*`_cipher_fnish`, *prefix*`_aead_finish`, *prefix*`_aead_verify`. + +If a driver implements a multi-part operation but not the corresponding single-part operation, the core calls the driver's multipart operation functions to perform the single-part operation. + +#### Multi-part operation function family `"hash_multipart"` + +This family corresponds to the calculation of a hash in one multiple parts. + +This family applies to transparent drivers only. + +This family requires the following type and functions: + +* Type `"hash_operation_t"`: the type of a hash operation context. It must be possible to copy a hash operation context byte by byte, therefore hash operation contexts must not contain any embedded pointers (except pointers to global data that do not change after the setup step). +* `"hash_setup"`: called by `psa_hash_setup()`. +* `"hash_update"`: called by `psa_hash_update()`. +* `"hash_finish"`: called by `psa_hash_finish()` and `psa_hash_verify()`. +* `"hash_abort"`: called by all multi-part hash functions. + +To verify a hash with `psa_hash_verify()`, the core calls the driver's *prefix`_hash_finish` function and compares the result with the reference hsah value. + +For example, a driver with the prefix `"acme"` that implements the `"hash_multipart"` function family must define the following type and functions (assuming that the capability does not use the `"names"` property to declare different type and function names): + +``` +typedef ... acme_hash_operation_t; +psa_status_t acme_hash_setup(acme_hash_operation_t *operation, + psa_algorithm_t alg); +psa_status_t acme_hash_update(acme_hash_operation_t *operation, + const uint8_t *input, + size_t input_length); +psa_status_t acme_hash_finish(acme_hash_operation_t *operation, + uint8_t *hash, + size_t hash_size, + size_t *hash_length); +psa_status_t acme_hash_abort(acme_hash_operation_t *operation); +``` + +#### Operation family `"mac_multipart"` + +TODO + +#### Operation family `"mac_verify_multipart"` + +TODO + +#### Operation family `"cipher_encrypt_multipart"` + +TODO + +#### Operation family `"cipher_decrypt_multipart"` + +TODO + +#### Operation family `"aead_encrypt_multipart"` + +TODO + +#### Operation family `"aead_decrypt_multipart"` + +TODO + +#### Operation family `"key_derivation"` + +This family requires the following type and functions: + +* Type `"key_derivation_operation_t"`: the type of a key derivation operation context. +* `"key_derivation_setup"`: called by `psa_key_derivation_setup()`. +* `"key_derivation_set_capacity"`: called by `psa_key_derivation_set_capacity()`. The core will always enforce the capacity, therefore this function does not need to do anything for algorithms where the output stream only depends on the effective generated length and not on the capacity. +* `"key_derivation_input_bytes"`: called by `psa_key_derivation_input_bytes()` and `psa_key_derivation_input_key()`. For transparent drivers, when processing a call to `psa_key_derivation_input_key()`, the core always calls the applicable driver's `"key_derivation_input_bytes"` function. +* `"key_derivation_input_key"` (opaque drivers only) +* `"key_derivation_output_bytes"`: called by `psa_key_derivation_output_bytes()`; also by `psa_key_derivation_output_key()` for transparent drivers. +* `"key_derivation_abort"`: called by all key derivation functions. + +TODO: key input and output for opaque drivers; deterministic key generation for transparent drivers + +TODO + +### Driver functions for key management + +The driver functions for key management differs significantly between [transparent drivers](#key-management-with-transparent-drivers) and [opaque drivers](#key-management-with-transparent-drivers). Refer to the applicable section for each driver type. + +### Miscellaneous driver functions + +#### Driver initialization + +An opaque driver may declare an `"init"` function in a capability with no algorithm, key type or key size. If so, the driver calls this function once during the initialization of the PSA Crypto subsystem. If the init function of any driver fails, the initialization of the PSA Crypto subsystem fails. + +When multiple drivers have an init function, the order in which they are called is unspecified. It is also unspecified whether other drivers' init functions are called if one or more init function fails. + +On platforms where the PSA Crypto implementation is a subsystem of a single application, the initialization of the PSA Crypto subsystem takes place during the call to `psa_crypto_init()`. On platforms where the PSA Crypto implementation is separate from the application or applications, the initialization the initialization of the PSA Crypto subsystem takes place before or during the first time an application calls `psa_crypto_init()`. + +For transparent drivers, the init function does not take any parameter. + +For opaque drivers, the init function takes the following parameters: + +* `uint8_t *persistent_state`: the driver's persistent state. On the first boot of the device, this contains all-bits-zero. On subsequent boots, the core loads the last saved state. +* `size_t persistent_state_size`: the size of the persistent state in bytes. + +For an opaque driver, if the init function succeeds, the core saves the updated persistent state. If the init function fails, the persistent state is unchanged. + +### Combining multiple drivers + +To declare a cryproprocessor can handle both cleartext and plaintext keys, you need to provide two driver descriptions, one for a transparent driver and one for an opaque driver. You can use the mapping in capabilities' `"names"` property to arrange for driver functions to map to the same C function. + +## Transparent drivers + +### Key format for transparent drivers + +The format of a key for transparent drivers is the same as in applications. Refer to the documentation of `psa_export_key()` and `psa_export_public_key()`. + +### Key management with transparent drivers + +Transparent drivers may provide the following key management functions: + +* `"generate_key"`: called by `psa_generate_key()`, only when generating a key pair (key such that `PSA_KEY_TYPE_IS_ASYMMETRIC` is true). +* `"derive_key"`: called by `psa_key_derivation_output_key()`, only when deriving a key pair (key such that `PSA_KEY_TYPE_IS_ASYMMETRIC` is true). +* `"export_public_key"`: called by the core to obtain the public key of a key pair. The core may call this function at any time to obtain the public key, which can be for `psa_export_public_key()` but also at other times, including during a cryptographic operation that requires the public key such as a call to `psa_verify_message()` on a key pair object. + +Transparent drivers are not involved when importing, exporting, copying or destroying keys, or when generating or deriving symmetric keys. + +### Fallback + +If a transparent driver function is part of a capability which has a true `"fallback"` property and returns `PSA_ERROR_NOT_SUPPORTED`, the built-in software implementation will be called instead. Any other value (`PSA_SUCCESS` or a different error code) is returned to the application. + +If there are multiple available transparent drivers, the core tries them in turn until one is declared without a true `"fallback"` property or returns a status other than `PSA_ERROR_NOT_SUPPORTED`. + +If a transparent driver function is part of a capability where the `"fallback"` property is false or omitted, the core should not include any other code for this capability, whether built in or in another transparent driver. + +## Opaque drivers + +Opaque drivers allow a PSA Cryptography implementation to delegate cryptographic operations to a separate environment that might not allow exporting key material in cleartext. The opaque driver interface is designed so that the core never inspects the representation of a key. The opaque driver interface is designed to support two subtypes of cryptoprocessors: + +* Some cryptoprocessors do not have persistent storage for individual keys. The representation of a key is the key material wrapped with a master key which is located in the cryptoprocessor and never exported from it. The core stores this wrapped key material on behalf of the cryptoprocessor. +* Some cryptoprocessors have persistent storage for individual keys. The representation of a key is an identifier such as label or slot number. The core stores this identifier. + +### Key format for opaque drivers + +The format of a key for opaque drivers is an opaque blob. The content of this blob is fully up to the driver. The core merely stores this blob. + +Note that since the core stores the key context blob as it is in memory, it must only contain data that is meaningful after a reboot. In particular, it must not contain any pointers or transient handles. + +The `"key_context"` property in the [driver description](#driver-description-top-level-element) specifies how to calculate the size of the key context as a function of the key type and size. This is an object with the following properties: + +* `"base_size"` (integer or string, optional): this many bytes are included in every key context. If omitted, this value defaults to 0. +* `"key_pair_size"` (integer or string, optional): this many bytes are included in every key context for a key pair. If omitted, this value defaults to 0. +* `"public_key_size"` (integer or string, optional): this many bytes are included in every key context for a public key. If omitted, this value defaults to 0. +* `"symmetric_factor"` (integer or string, optional): every key context for a symmetric key includes this many times the key size. If omitted, this value defaults to 0. +* `"store_public_key"` (boolean, optional): If specified and true, for a key pair, the key context includes space for the public key. If omitted or false, no additional space is added for the public key. +* `"size_function"` (string, optional): the name of a function that returns the number of bytes that the driver needs in a key context for a key. This may be a pointer to function. This must be a C identifier; more complex expressions are not permitted. If the core uses this function, it supersedes all the other properties. + +The integer properties must be C language constants. A typical value for `"base_size"` is `sizeof(acme_key_context_t)` where `acme_key_context_t` is a type defined in a driver header file. + +#### Size of a dynamically allocated key context + +If the core supports dynamic allocation for the key context and chooses to use it, and the driver specification includes the `"size_function"` property, the size of the key context is at least +``` +size_function(key_type, key_bits) +``` +where `size_function` is the function named in the `"size_function"` property, `key_type` is the key type and `key_bits` is the key size in bits. The prototype of the size function is +``` +size_t size_function(psa_key_type_t key_type, size_t key_bits); +``` + +#### Size of a statically allocated key context + +If the core does not support dynamic allocation for the key context or chooses not to use it, or if the driver specification does not include the `"size_function"` property, the size of the key context for a key of type `key_type` and of size `key_bits` bits is: + +* For a key pair (`PSA_KEY_TYPE_IS_KEY_PAIR(key_type)` is true): + ``` + base_size + key_pair_size + public_key_overhead + ``` + where `public_key_overhead = PSA_EXPORT_PUBLIC_KEY_MAX_SIZE(key_type, key_bits)` if the `"store_public_key"` property is true and `public_key_overhead = 0` otherwise. + +* For a public key (`PSA_KEY_TYPE_IS_PUBLIC_KEY(key_type)` is true): + ``` + base_size + public_key_size + ``` + +* For a symmetric key (not a key pair or public key): + ``` + base_size + symmetric_factor * key_bytes + ``` + where `key_bytes = ((key_bits + 7) / 8)` is the key size in bytes. + +#### Key context size for a secure element with storage + +If the key is stored in the secure element and the driver only needs to store a label for the key, use `"base_size"` as the size of the label plus any other metadata that the driver needs to store, and omit the other properties. + +If the key is stored in the secure element, but the secure element does not store the public part of a key pair and cannot recompute it on demand, additionally use the `"store_public_key"` property with the value `true`. Note that this only influences the size of the key context: the driver code must copy the public key to the key context and retrieve it on demand in its `export_public_key` function. + +#### Key context size for a secure element without storage + +If the key is stored in wrapped form outside the secure element, and the wrapped form of the key plus any metadata has up to *N* bytes of overhead, use *N* as the value of the `"base_size"` property and set the `"symmetric_factor"` property to 1. Set the `"key_pair_size"` and `"public_key_size"` properties appropriately for the largest supported key pair and the largest supported public key respectively. + +### Key management with opaque drivers + +Transparent drivers may provide the following key management functions: + +* `"allocate_key"`: called by `psa_import_key()`, `psa_generate_key()`, `psa_key_derivation_output_key()` or `psa_copy_key()` before creating a key in the location of this driver. +* `"import_key"`: called by `psa_import_key()`, or by `psa_copy_key()` when copying a key from another location. +* `"export_key"`: called by `psa_export_key()`, or by `psa_copy_key()` when copying a key from to location. +* `"export_public_key"`: called by the core to obtain the public key of a key pair. The core may call this function at any time to obtain the public key, which can be for `psa_export_public_key()` but also at other times, including during a cryptographic operation that requires the public key such as a call to `psa_verify_message()` on a key pair object. +* `"copy_key"`: called by `psa_copy_key()` when copying a key within the same location. +* `"destroy_key"`: called by `psa_destroy_key()`. +* `"generate_key"`: called by `psa_generate_key()`. +* `"derive_key"`: called by `psa_key_derivation_output_key()`. + +#### Key creation in a secure element without storage + +This section describes the key creation process for secure elements that do not store the key material. The driver must obtain a wrapped form of the key material which the core will store. A driver for such a secure element has no `"allocate_key"` function. + +When creating a key with an opaque driver which does not have an `"allocate_key"` function: + +1. The core allocates memory for the key context. +2. The core calls the driver's import, generate, derive or copy function. +3. The core saves the resulting wrapped key material and any other data that the key context may contain. + +#### Key creation in a secure element with storage + +This section describes the key creation process for secure elements that have persistent storage for the key material. A driver for such a secure element has an `"allocate_key"` function whose intended purpose is to obtain an identifier for the key. This may be, for example, a unique label or a slot number. + +When creating a persistent key with an opaque driver which does not have an `"allocate_key"` function: + +1. The core calls the driver's `"allocate_key"` function. This function typically allocates an identifier for the key without modifying the state of the secure element and stores the identifier in the key context. This function should not modify the state of the secure element. + +1. The core saves the key context to persistent storage. + +1. The core saves the driver's persistent state. + +1. The core calls the driver's key creation function. + +If a failure occurs after the `"allocate_key"` step but before the call to the second driver function, the core will do one of the following: + +* Fail the creation of the key without indicating this to the driver. This can happen, in particular, if the device loses power immediately after the key allocation function returns. +* Call the driver's `"destroy_key"` function. + +TODO: explain the individual key management functions + +### Opaque driver persistent state + +The core maintains persistent state on behalf of an opaque driver. This persistent state consists of a single byte array whose size is given by the `"persistent_state_size"` property in the [driver description](#driver-description-top-level-element). + +TODO: how the state is passed to the driver; which driver functions can modify the state and how; when the core saves the updated state + +## How to use drivers from an application + +### Declaring which cryptographic mechanism an application needs + +TODO: an application requirements description, broadly similar to driver capabilities. + +### Using transparent drivers + +Transparent drivers linked into the library are automatically used for the mechanisms that they implement. + +### Using opaque drivers + +Each opaque driver is assigned a location. The driver is invoked for all actions that use a key in that location. A key's location is indicated by its lifetime. The application chooses the key's lifetime when it creates the key. + +For example, the following snippet creates an AES-GCM key which is only accessible inside a secure element. +``` +psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; +psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( + PSA_KEY_PERSISTENCE_DEFAULT, PSA_KEY_LOCATION_ACME_SECURE_ELEMENT)); +psa_set_key_identifer(&attributes, 42); +psa_set_key_type(&attributes, PSA_KEY_TYPE_AES); +psa_set_key_size(&attributes, 128); +psa_set_key_algorithm(&attributes, PSA_ALG_GCM); +psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT); +psa_key_handle_t handle = 0; +psa_generate_key(&attributes, &handle); +``` + +TODO: how does the application know which location value to use? + +## Open questions + +### Driver declarations + +#### Declaring driver functions + +The core may want to provide declarations for the driver functions so that it can compile code using them. At the time of writing this paragraph, the driver headers must define types but there is no obligation for them to declare functions. The core knows what the function names and argument types are, so it can generate prototypes. + +It should be ok for driver functions to be function-like macros or function pointers. + +#### Driver location values + +How does a driver author decide which location values to use? It should be possible to combine drivers from different sources. Use the same vendor assignment as for PSA services? + +Can the driver assembly process generate distinct location values as needed? This can be convenient, but it's also risky: if you upgrade a device, you need the location values to be the same between builds. + +### Driver function interfaces + +#### Driver function parameter conventions + +Should 0-size buffers be guaranteed to have a non-null pointers? + +Should drivers really have to cope with overlap? + +Should the core guarantee that the output buffer size has the size indicated by the applicable buffer size macro (which may be an overestimation)? + +### Partial computations in drivers + +#### Substitution points + +Earlier drafts of the driver interface had a concept of _substitution points_: places in the calculation where a driver may be called. Some hardware doesn't do the whole calculation, but only the “main” part. This goes both for transparent and opaque drivers. Some common examples: + +* A processor that performs the RSA exponentiation, but not the padding. The driver should be able to leverage the padding code in the core. +* A processor that performs a block cipher operation only for a single block, or only in ECB mode, or only in CTR mode. The core would perform the block mode (CBC, CTR, CCM, ...). + +This concept, or some other way to reuse portable code such as specifying inner functions like `psa_rsa_pad` in the core, should be added to the specification. + +### Key management + +#### Mixing drivers in key derivation + +How does `psa_key_derivation_output_key` work when the extraction part and the expansion part use different drivers? + +#### Public key not stored + +ECC key pairs are stored as the private key value only. The public key needs to be calculated from that. + +It's fairly common for secure elements to also store only the private value. When a key is generated randomly or derived, the hardware reports the public key, and it is up to the software to store it. The current design makes this the job of the driver. Should it be the work of the core instead? + +Note that a solution also has to work for transparent keys, and when importing a private key into a secure element. If the core already has code to calculate the public key, it would make sense for the driver to be able to use it, especially in these cases. + +### Opaque drivers + +#### Opaque driver persistent state + +Should the driver be able to update it at any time? + + From 278e5ebf7810aa5ce4dcd18e4d11aa1e72805418 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Jul 2020 11:28:20 +0200 Subject: [PATCH 03/55] Start Mbed TLS guides to PSA drivers Driver developer's guide: introduction on how to write a driver. Driver integration guide: how to build Mbed TLS with drivers. Signed-off-by: Gilles Peskine --- docs/proposed/Makefile | 2 + docs/proposed/psa-driver-developer-guide.md | 49 +++++++++++++++++++ docs/proposed/psa-driver-integration-guide.md | 45 +++++++++++++++++ 3 files changed, 96 insertions(+) create mode 100644 docs/proposed/psa-driver-developer-guide.md create mode 100644 docs/proposed/psa-driver-integration-guide.md diff --git a/docs/proposed/Makefile b/docs/proposed/Makefile index cf656dd79..2132b0843 100644 --- a/docs/proposed/Makefile +++ b/docs/proposed/Makefile @@ -3,6 +3,8 @@ PANDOC = pandoc default: all all_markdown = \ + psa-driver-developer-guide.md \ + psa-driver-integration-guide.md \ psa-driver-interface.md \ # This line is intentionally left blank diff --git a/docs/proposed/psa-driver-developer-guide.md b/docs/proposed/psa-driver-developer-guide.md new file mode 100644 index 000000000..ca24441d5 --- /dev/null +++ b/docs/proposed/psa-driver-developer-guide.md @@ -0,0 +1,49 @@ +PSA Cryptoprocessor driver developer's guide +============================================ + +**This is a specification of work in progress. The implementation is not yet merged into Mbed TLS.** + +This document describes how to write drivers of cryptoprocessors such as accelerators and secure elements for the PSA cryptography subsystem of Mbed TLS. + +This document focuses on behavior that is specific to Mbed TLS. For a reference of the interface between Mbed TLS and drivers, refer to the [PSA Cryptoprocessor Driver Interface specification](architecture/psa-driver-interface.md). + +The interface is not fully implemented in Mbed TLS yet and is disabled by default. You can enable the experimental work in progress by setting `MBEDTLS_PSA_CRYPTO_DRIVERS` in the compile-time configuration. Please note that the interface may still change: until further notice, we do not guarantee backward compatibility with existing driver code when `MBEDTLS_PSA_CRYPTO_DRIVERS` is enabled. + +## Introduction + +### Purpose + +The PSA cryptography driver interface provides a way to build Mbed TLS with additional code that implements certain cryptographic primitives. This is primarily intended to support platform-specific hardware. + +There are two types of drivers: + +* **Transparent** drivers implement cryptographic operations on keys that are provided in cleartext at the beginning of each operation. They are typically used for hardware **accelerators**. When a transparent driver is available for a particular combination of parameters (cryptographic algorithm, key type and size, etc.), it is used instead of the default software implementation. Transparent drivers can also be pure software implementations that are distributed as plug-ins to a PSA Crypto implementation. +* **Opaque** drivers implement cryptographic operations on keys that can only be used inside a protected environment such as a **secure element**, a hardware security module, a smartcard, a secure enclave, etc. An opaque driver is invoked for the specific key location that the driver is registered for: the dispatch is based on the key's lifetime. + +### Deliverables for a driver + +To write a driver, you need to implement some functions with C linkage, and to declare these functions in a **driver description file**. The driver description file declares which functions the driver implements and what cryptographic mechanisms they support. Depending on the driver type, you may also need to define some C types and macros in a header file. + +The concrete syntax for a driver description file is JSON. The structure of this JSON file is specified in the section [“Driver description syntax”](architecture/psa-driver-interface.md#driver-description-syntax) of the PSA cryptography driver interface specification. + +A driver therefore consists of: + +* A driver description file (in JSON format). +* C header files defining the types required by the driver description. The names of these header files is declared in the driver description file. +* An object file compiled for the target platform defining the functions required by the driver description. Implementations may allow drivers to be provided as source files and compiled with the core instead of being pre-compiled. + +## Driver C interfaces + +Mbed TLS calls [driver functions as specified in the PSA Cryptography Driver Interface specification](architecture/psa-driver-interface.md#) except as otherwise indicated in this section. + +### Key handles + +Mbed TLS currently implements the interface for opening and closing persistent keys from version 1.0 beta 3 of the PSA Crypto specification. As a consequence, functions that operate on an existing key take an argument of type `psa_key_handle_t` instead of `psa_key_id_t`. Functions that create a new key take an argument of type `psa_key_handle_t *` instead of `psa_key_id_t *`. + +## Building and testing your driver + + + +## Dependencies on the Mbed TLS configuration + + diff --git a/docs/proposed/psa-driver-integration-guide.md b/docs/proposed/psa-driver-integration-guide.md new file mode 100644 index 000000000..bfd765ea5 --- /dev/null +++ b/docs/proposed/psa-driver-integration-guide.md @@ -0,0 +1,45 @@ +Building Mbed TLS with PSA cryptoprocessor drivers +================================================== + +**This is a specification of work in progress. The implementation is not yet merged into Mbed TLS.** + +This document describes how to build Mbed TLS with additional cryptoprocessor drivers that follow the PSA cryptoprocessor driver interface. + +The interface is not fully implemented in Mbed TLS yet and is disabled by default. You can enable the experimental work in progress by setting `MBEDTLS_PSA_CRYPTO_DRIVERS` in the compile-time configuration. Please note that the interface may still change: until further notice, we do not guarantee backward compatibility with existing driver code when `MBEDTLS_PSA_CRYPTO_DRIVERS` is enabled. + +## Introduction + +The PSA cryptography driver interface provides a way to build Mbed TLS with additional code that implements certain cryptographic primitives. This is primarily intended to support platform-specific hardware. + +Note that such drivers are only available through the PSA cryptography API (crypto functions beginning with `psa_`, and X.509 and TLS interfaces that reference PSA types). + +Concretely speaking, a driver consists of one or more **driver description files** in JSON format and some code to include in the build. The driver code can either be provided in binary form as additional object file to link, or in source form. + +## How to build Mbed TLS with drivers + +To build Mbed TLS with drivers: + +1. Activate `MBEDTLS_PSA_CRYPTO_DRIVERS` in the library configuration. + + ``` + cd /path/to/mbedtls + scripts/config.py set MBEDTLS_PSA_CRYPTO_DRIVERS + ``` + +2. Pass the driver description files through the Make variable `PSA_DRIVERS` when building the library. + + ``` + cd /path/to/mbedtls + make PSA_DRIVERS="/path/to/acme/driver.json /path/to/nadir/driver.json" lib + ``` + +3. Link your application with the implementation of the driver functions. + + ``` + cd /path/to/application + ld myapp.o -L/path/to/acme -lacmedriver -L/path/to/nadir -lnadirdriver -L/path/to/mbedtls -lmbedcrypto + ``` + + + + From 71db60bd11a0ccd09a2a5e1222788150846c5a86 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Jul 2020 13:18:28 +0200 Subject: [PATCH 04/55] Automatically define location/lifetime constants PSA_KEY_LOCATION_acme, PSA_KEY_LIFETIME_acme Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index 0e24ea3ec..b0cc149c5 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -5,7 +5,7 @@ This document describes an interface for cryptoprocessor drivers in the PSA cryp This specification is work in progress and should be considered to be in a beta stage. There is ongoing work to implement this interface in Mbed TLS, which is the reference implementation of the PSA Cryptography API. At this stage, Arm does not expect major changes, but minor changes are expected based on experience from the first implementation and on external feedback. -Time-stamp: "2020/07/13 08:07:14 GMT" +Time-stamp: "2020/07/13 10:03:05 GMT" ## Introduction @@ -452,7 +452,7 @@ For example, the following snippet creates an AES-GCM key which is only accessib ``` psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( - PSA_KEY_PERSISTENCE_DEFAULT, PSA_KEY_LOCATION_ACME_SECURE_ELEMENT)); + PSA_KEY_PERSISTENCE_DEFAULT, PSA_KEY_LOCATION_acme)); psa_set_key_identifer(&attributes, 42); psa_set_key_type(&attributes, PSA_KEY_TYPE_AES); psa_set_key_size(&attributes, 128); @@ -462,7 +462,27 @@ psa_key_handle_t handle = 0; psa_generate_key(&attributes, &handle); ``` -TODO: how does the application know which location value to use? + +## Using opaque drivers from an application + +The a compile-time constant for each opaque driver indicating its location called `PSA_KEY_LOCATION_`*prefix* where *prefix* is the value of the `"prefix"` property in the driver description. For convenience, Mbed TLS also declares a compile-time constant for the corresponding lifetime with the default persistence called `PSA_KEY_LIFETIME_`*prefix*. Therefore, to declare an opaque key in the location with the prefix `foo` with the default persistence, call `psa_set_key_lifetime` during the key creation as follows: +``` +psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_foo); +``` + +To declare a volatile key: +``` +psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( + PSA_KEY_LOCATION_foo, + PSA_KEY_PERSISTENCE_VOLATILE)); +``` + +Generally speaking, to declare a key with a specified persistence: +``` +psa_set_key_lifetime(&attributes, PSA_KEY_LIFETIME_FROM_PERSISTENCE_AND_LOCATION( + PSA_KEY_LOCATION_foo, + persistence)); +``` ## Open questions From bcce2eff2763536be99e5160e3511e0f3b0aacd1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Jul 2020 13:19:44 +0200 Subject: [PATCH 05/55] Transparent drivers may have init functions too Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index b0cc149c5..cabb81b07 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -5,7 +5,7 @@ This document describes an interface for cryptoprocessor drivers in the PSA cryp This specification is work in progress and should be considered to be in a beta stage. There is ongoing work to implement this interface in Mbed TLS, which is the reference implementation of the PSA Cryptography API. At this stage, Arm does not expect major changes, but minor changes are expected based on experience from the first implementation and on external feedback. -Time-stamp: "2020/07/13 10:03:05 GMT" +Time-stamp: "2020/07/13 11:19:35 GMT" ## Introduction @@ -275,7 +275,7 @@ The driver functions for key management differs significantly between [transpare #### Driver initialization -An opaque driver may declare an `"init"` function in a capability with no algorithm, key type or key size. If so, the driver calls this function once during the initialization of the PSA Crypto subsystem. If the init function of any driver fails, the initialization of the PSA Crypto subsystem fails. +A driver may declare an `"init"` function in a capability with no algorithm, key type or key size. If so, the driver calls this function once during the initialization of the PSA Crypto subsystem. If the init function of any driver fails, the initialization of the PSA Crypto subsystem fails. When multiple drivers have an init function, the order in which they are called is unspecified. It is also unspecified whether other drivers' init functions are called if one or more init function fails. From 2774fc45fffcd7fecfe9703e17bacbe20c117a5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 16 Jul 2020 10:40:13 +0200 Subject: [PATCH 06/55] Add -u option to check-generated-files.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/check-generated-files.sh | 32 +++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/tests/scripts/check-generated-files.sh b/tests/scripts/check-generated-files.sh index e39b66182..cc5db9730 100755 --- a/tests/scripts/check-generated-files.sh +++ b/tests/scripts/check-generated-files.sh @@ -23,11 +23,29 @@ set -eu +if [ $# -ne 0 ] && [ "$1" = "--help" ]; then + cat <&2 exit 1 fi +UPDATE= +if [ $# -ne 0 ] && [ "$1" = "-u" ]; then + shift + UPDATE='y' +fi + check() { SCRIPT=$1 @@ -53,9 +71,15 @@ check() for FILE in $FILES; do if ! diff $FILE $FILE.bak >/dev/null 2>&1; then echo "'$FILE' was either modified or deleted by '$SCRIPT'" - exit 1 + if [ -z "$UPDATE" ]; then + exit 1 + fi + fi + if [ -z "$UPDATE" ]; then + mv $FILE.bak $FILE + else + rm $FILE.bak fi - mv $FILE.bak $FILE if [ -d $TO_CHECK ]; then # Create a grep regular expression that we can check against the @@ -72,7 +96,9 @@ check() # Check if there are any new files if ls -1 $TO_CHECK | grep -v "$PATTERN" >/dev/null 2>&1; then echo "Files were created by '$SCRIPT'" - exit 1 + if [ -z "$UPDATE" ]; then + exit 1 + fi fi fi } From a80651c483cb74e0596be928bf6212eee852f7f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 16 Jul 2020 10:53:13 +0200 Subject: [PATCH 07/55] Add a pre-commit hook that checks generated files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/git-scripts/pre-commit.sh | 36 +++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100755 tests/git-scripts/pre-commit.sh diff --git a/tests/git-scripts/pre-commit.sh b/tests/git-scripts/pre-commit.sh new file mode 100755 index 000000000..43656869b --- /dev/null +++ b/tests/git-scripts/pre-commit.sh @@ -0,0 +1,36 @@ +#!/bin/sh + +# pre-commit.sh +# +# Copyright (c) 2017, ARM Limited, All Rights Reserved +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# This file is part of Mbed TLS (https://tls.mbed.org) + +# Purpose +# +# This script does quick sanity checks before commiting: +# - check that generated files are up-to-date. +# +# It is meant to be called as a git pre-commit hook, see README.md. +# +# From the git sample pre-commit hook: +# Called by "git commit" with no arguments. The hook should +# exit with non-zero status after issuing an appropriate message if +# it wants to stop the commit. + +set -eu + +tests/scripts/check-generated-files.sh From 799e57612abd3f587206f2034ba04b9dc7c57ba2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:34:00 +0200 Subject: [PATCH 08/55] ECDSA requires a short Weierstrass curve Document in config.h, and enforce in check_config.h, that MBEDTLS_ECDSA_C requires at least one short Weierstrass curve to be enabled. A Montgomery curve is not enough. Signed-off-by: Gilles Peskine --- include/mbedtls/check_config.h | 11 +++++++++++ include/mbedtls/config.h | 6 +++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index f2148a8b5..359659790 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -103,6 +103,17 @@ #if defined(MBEDTLS_ECDSA_C) && \ ( !defined(MBEDTLS_ECP_C) || \ + !( defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) ) || \ !defined(MBEDTLS_ASN1_PARSE_C) || \ !defined(MBEDTLS_ASN1_WRITE_C) ) #error "MBEDTLS_ECDSA_C defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index e00c546e5..0bab0c0cb 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -756,6 +756,7 @@ * * Comment macros to disable the curve and functions for it */ +/* Short Weierstrass curves (supporting ECP, ECDH, ECDSA) */ #define MBEDTLS_ECP_DP_SECP192R1_ENABLED #define MBEDTLS_ECP_DP_SECP224R1_ENABLED #define MBEDTLS_ECP_DP_SECP256R1_ENABLED @@ -767,6 +768,7 @@ #define MBEDTLS_ECP_DP_BP256R1_ENABLED #define MBEDTLS_ECP_DP_BP384R1_ENABLED #define MBEDTLS_ECP_DP_BP512R1_ENABLED +/* Montgomery curves (supporting ECP) */ #define MBEDTLS_ECP_DP_CURVE25519_ENABLED #define MBEDTLS_ECP_DP_CURVE448_ENABLED @@ -2571,7 +2573,9 @@ * This module is used by the following key exchanges: * ECDHE-ECDSA * - * Requires: MBEDTLS_ECP_C, MBEDTLS_ASN1_WRITE_C, MBEDTLS_ASN1_PARSE_C + * Requires: MBEDTLS_ECP_C, MBEDTLS_ASN1_WRITE_C, MBEDTLS_ASN1_PARSE_C, + * and at least one MBEDTLS_ECP_DP_XXX_ENABLED for a + * short Weierstrass curve. */ #define MBEDTLS_ECDSA_C From 9b99a8942f4afd63e5653482d20a2be1330c8c6a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 18:32:19 +0200 Subject: [PATCH 09/55] mbedtls_ecp_muladd is only for short Weierstrass curves Document that mbedtls_ecp_muladd and mbedtls_ecp_muladd_restartable are only implemented on short Weierstrass curves. Exclude these functions at build time if no short Weierstrass curve is included in the build. Before, these functions failed to compile in such a configuration. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 34 ++++++++++++++++++++++++++++++++++ library/ecp.c | 2 ++ 2 files changed, 36 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 450e35492..8185224a9 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -61,6 +61,26 @@ #define MBEDTLS_ERR_ECP_IN_PROGRESS -0x4B00 /**< Operation in progress, call again with the same parameters to continue. */ +/* Flags indicating whether to include code that is specific to certain + * types of curves. These flags are for internal library use only. */ +#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) +#define MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED +#endif +#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ + defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) +#define MBEDTLS_ECP_MONTGOMERY_ENABLED +#endif + #ifdef __cplusplus extern "C" { #endif @@ -906,6 +926,7 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng, mbedtls_ecp_restart_ctx *rs_ctx ); +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /** * \brief This function performs multiplication and addition of two * points by integers: \p R = \p m * \p P + \p n * \p Q @@ -915,6 +936,10 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * \note In contrast to mbedtls_ecp_mul(), this function does not * guarantee a constant execution flow and timing. * + * \note This function is only defined for short Weierstrass curves. + * It may not be included in builds without any short + * Weierstrass curve. + * * \param grp The ECP group to use. * This must be initialized and have group parameters * set, for example through mbedtls_ecp_group_load(). @@ -933,6 +958,8 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * valid private keys, or \p P or \p Q are not valid public * keys. * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED on memory-allocation failure. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if \p grp does not + * designate a short Weierstrass curve. * \return Another negative error code on other kinds of failure. */ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, @@ -950,6 +977,10 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * but it can return early and restart according to the limit * set with \c mbedtls_ecp_set_max_ops() to reduce blocking. * + * \note This function is only defined for short Weierstrass curves. + * It may not be included in builds without any short + * Weierstrass curve. + * * \param grp The ECP group to use. * This must be initialized and have group parameters * set, for example through mbedtls_ecp_group_load(). @@ -969,6 +1000,8 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * valid private keys, or \p P or \p Q are not valid public * keys. * \return #MBEDTLS_ERR_MPI_ALLOC_FAILED on memory-allocation failure. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if \p grp does not + * designate a short Weierstrass curve. * \return #MBEDTLS_ERR_ECP_IN_PROGRESS if maximum number of * operations was reached: see \c mbedtls_ecp_set_max_ops(). * \return Another negative error code on other kinds of failure. @@ -978,6 +1011,7 @@ int mbedtls_ecp_muladd_restartable( const mbedtls_mpi *m, const mbedtls_ecp_point *P, const mbedtls_mpi *n, const mbedtls_ecp_point *Q, mbedtls_ecp_restart_ctx *rs_ctx ); +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ /** * \brief This function checks that a point is a valid public key diff --git a/library/ecp.c b/library/ecp.c index 2f69d6869..1b289f67a 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2781,6 +2781,7 @@ cleanup: } #endif /* ECP_SHORTWEIERSTRASS */ +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * R = m * P with shortcuts for m == 1 and m == -1 * NOT constant-time - ONLY for short Weierstrass! @@ -2926,6 +2927,7 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, ECP_VALIDATE_RET( Q != NULL ); return( mbedtls_ecp_muladd_restartable( grp, R, m, P, n, Q, NULL ) ); } +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ #if defined(ECP_MONTGOMERY) /* From e8c04fed5129a2fdf89d0aad8746420b5bceb634 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:44:21 +0200 Subject: [PATCH 10/55] Replace ECP_xxx by MBEDTLS_ECP__xxx_ENABLED Replace the now-redundant internal curve type macros ECP_xxx by the macros MBEDTLS_ECP__xxx_ENABLED which are declared in ecp.h. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp_internal.h | 8 ++-- library/ecp.c | 75 +++++++++++++--------------------- 2 files changed, 32 insertions(+), 51 deletions(-) diff --git a/include/mbedtls/ecp_internal.h b/include/mbedtls/ecp_internal.h index 3b6fbf112..92fee42d4 100644 --- a/include/mbedtls/ecp_internal.h +++ b/include/mbedtls/ecp_internal.h @@ -105,7 +105,7 @@ int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp ); */ void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp ); -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) #if defined(MBEDTLS_ECP_RANDOMIZE_JAC_ALT) /** @@ -245,9 +245,9 @@ int mbedtls_internal_ecp_normalize_jac( const mbedtls_ecp_group *grp, mbedtls_ecp_point *pt ); #endif -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) #if defined(MBEDTLS_ECP_DOUBLE_ADD_MXZ_ALT) int mbedtls_internal_ecp_double_add_mxz( const mbedtls_ecp_group *grp, @@ -291,7 +291,7 @@ int mbedtls_internal_ecp_normalize_mxz( const mbedtls_ecp_group *grp, mbedtls_ecp_point *P ); #endif -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ #endif /* MBEDTLS_ECP_INTERNAL_ALT */ diff --git a/library/ecp.c b/library/ecp.c index 1b289f67a..ab5ab9585 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -501,25 +501,6 @@ int mbedtls_ecp_check_budget( const mbedtls_ecp_group *grp, #endif /* MBEDTLS_ECP_RESTARTABLE */ -#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) -#define ECP_SHORTWEIERSTRASS -#endif - -#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ - defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) -#define ECP_MONTGOMERY -#endif - /* * List of supported curves: * - internal ID @@ -897,7 +878,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { *olen = plen; @@ -907,7 +888,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary_le( &P->X, buf, plen ) ); } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* @@ -970,7 +951,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { if( plen != ilen ) @@ -986,7 +967,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &pt->Z, 1 ) ); } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { if( buf[0] == 0x00 ) @@ -1304,7 +1285,7 @@ cleanup: return( ret ); } -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * For curves in short Weierstrass form, we do all the internal operations in * Jacobian coordinates. @@ -2413,9 +2394,9 @@ cleanup: return( ret ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) /* * For Montgomery curves, we do all the internal arithmetic in projective * coordinates. Import/export of points uses only the x coordinates, which is @@ -2649,7 +2630,7 @@ cleanup: return( ret ); } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ /* * Restartable multiplication R = m * P @@ -2693,11 +2674,11 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, } ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) MBEDTLS_MPI_CHK( ecp_mul_mxz( grp, R, m, P, f_rng, p_rng ) ); #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) MBEDTLS_MPI_CHK( ecp_mul_comb( grp, R, m, P, f_rng, p_rng, rs_ctx ) ); #endif @@ -2731,7 +2712,7 @@ int mbedtls_ecp_mul( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, return( mbedtls_ecp_mul_restartable( grp, R, m, P, f_rng, p_rng, NULL ) ); } -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* * Check that an affine point is valid as a public key, * short weierstrass curves (SEC1 3.2.3.1) @@ -2779,7 +2760,7 @@ cleanup: return( ret ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ #if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* @@ -2929,7 +2910,7 @@ int mbedtls_ecp_muladd( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, } #endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) /* * Check validity of a public key for Montgomery curves with x-only schemes */ @@ -2943,7 +2924,7 @@ static int ecp_check_pubkey_mx( const mbedtls_ecp_group *grp, const mbedtls_ecp_ return( 0 ); } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ /* * Check that a point is valid as a public key @@ -2958,11 +2939,11 @@ int mbedtls_ecp_check_pubkey( const mbedtls_ecp_group *grp, if( mbedtls_mpi_cmp_int( &pt->Z, 1 ) != 0 ) return( MBEDTLS_ERR_ECP_INVALID_KEY ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) return( ecp_check_pubkey_mx( grp, pt ) ); #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) return( ecp_check_pubkey_sw( grp, pt ) ); #endif @@ -2978,7 +2959,7 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, ECP_VALIDATE_RET( grp != NULL ); ECP_VALIDATE_RET( d != NULL ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* see RFC 7748 sec. 5 para. 5 */ @@ -2993,8 +2974,8 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, return( 0 ); } -#endif /* ECP_MONTGOMERY */ -#if defined(ECP_SHORTWEIERSTRASS) +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* see SEC1 3.2 */ @@ -3004,7 +2985,7 @@ int mbedtls_ecp_check_privkey( const mbedtls_ecp_group *grp, else return( 0 ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); } @@ -3026,7 +3007,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, n_size = ( grp->nbits + 7 ) / 8; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* [M225] page 5 */ @@ -3052,9 +3033,9 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( d, 2, 0 ) ); } } -#endif /* ECP_MONTGOMERY */ +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ @@ -3096,7 +3077,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, } while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); } -#endif /* ECP_SHORTWEIERSTRASS */ +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ cleanup: return( ret ); @@ -3174,7 +3155,7 @@ int mbedtls_ecp_read_key( mbedtls_ecp_group_id grp_id, mbedtls_ecp_keypair *key, ret = MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE; -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { /* @@ -3209,7 +3190,7 @@ int mbedtls_ecp_read_key( mbedtls_ecp_group_id grp_id, mbedtls_ecp_keypair *key, } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &key->d, buf, buflen ) ); @@ -3237,7 +3218,7 @@ int mbedtls_ecp_write_key( mbedtls_ecp_keypair *key, ECP_VALIDATE_RET( key != NULL ); ECP_VALIDATE_RET( buf != NULL ); -#if defined(ECP_MONTGOMERY) +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { if( key->grp.id == MBEDTLS_ECP_DP_CURVE25519 ) @@ -3252,7 +3233,7 @@ int mbedtls_ecp_write_key( mbedtls_ecp_keypair *key, } #endif -#if defined(ECP_SHORTWEIERSTRASS) +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) if( mbedtls_ecp_get_type( &key->grp ) == MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS ) { MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( &key->d, buf, buflen ) ); From aa9493a4111e88ec1918dca5b1abb281faa405b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Sep 2018 14:44:03 +0200 Subject: [PATCH 11/55] Add guards around code that is specific to dynamically-loaded groups For some curves (semi-coincidentally, short Weierstrass curves), the ECP module calculates some group parameters dynamically. Build the code to calculate the parameters only if a relevant curve is enabled. This fixes an unused function warning when building with only Montgomery curves. Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 92bbb896a..137ef1ede 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -553,6 +553,22 @@ static const mbedtls_mpi_uint brainpoolP512r1_n[] = { }; #endif /* MBEDTLS_ECP_DP_BP512R1_ENABLED */ +#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) +/* For these curves, we build the group parameters dynamically. */ +#define ECP_LOAD_GROUP +#endif + +#if defined(ECP_LOAD_GROUP) /* * Create an MPI from embedded constants * (assumes len is an exact multiple of sizeof mbedtls_mpi_uint) @@ -603,6 +619,7 @@ static int ecp_group_load( mbedtls_ecp_group *grp, return( 0 ); } +#endif /* ECP_LOAD_GROUP */ #if defined(MBEDTLS_ECP_NIST_OPTIM) /* Forward declarations */ @@ -644,6 +661,7 @@ static int ecp_mod_p224k1( mbedtls_mpi * ); static int ecp_mod_p256k1( mbedtls_mpi * ); #endif +#if defined(ECP_LOAD_GROUP) #define LOAD_GROUP_A( G ) ecp_group_load( grp, \ G ## _p, sizeof( G ## _p ), \ G ## _a, sizeof( G ## _a ), \ @@ -659,6 +677,7 @@ static int ecp_mod_p256k1( mbedtls_mpi * ); G ## _gx, sizeof( G ## _gx ), \ G ## _gy, sizeof( G ## _gy ), \ G ## _n, sizeof( G ## _n ) ) +#endif /* ECP_LOAD_GROUP */ #if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) /* From 963a207678ff0b83ee39641af1b452d78049632a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 18:31:30 +0200 Subject: [PATCH 12/55] Document what needs to be done when adding a new curve Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 8185224a9..7fe747302 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -94,6 +94,20 @@ extern "C" { * parameters. Therefore, only standardized domain parameters from trusted * sources should be used. See mbedtls_ecp_group_load(). */ +/* Note: when adding a new curve: + * - Add it at the end of this enum, otherwise you'll break the ABI by + * changing the numerical value for existing curves. + * - Increment MBEDTLS_ECP_DP_MAX below. + * - Add the corresponding MBEDTLS_ECP_DP_xxx_ENABLED macro definition to + * config.h. + * - List the curve as a dependency of MBEDTLS_ECP_C and + * MBEDTLS_ECDSA_C if supported in check_config.h. + * - Add the curve to the appropriate curve type macro + * MBEDTLS_ECP_yyy_ENABLED above. + * - Add the necessary definitions to ecp_curves.c. + * - Add the curve to the ecp_supported_curves array in ecp.c. + * - Add the curve to applicable profiles in x509_crt.c if applicable. + */ typedef enum { MBEDTLS_ECP_DP_NONE = 0, /*!< Curve not defined. */ From 7ab66a6bf166891978ac1beec53843451f7dfc4c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 17:47:41 +0200 Subject: [PATCH 13/55] Add missing dependencies for ECDH_xxx key exchanges ECDH_ECDSA requires ECDSA and ECDH_RSA requires RSA. Signed-off-by: Gilles Peskine --- include/mbedtls/check_config.h | 6 ++++-- include/mbedtls/config.h | 4 ++-- tests/scripts/depends-pkalgs.pl | 4 +++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 359659790..4f6c6328d 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -257,12 +257,14 @@ #endif #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) && \ - ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_X509_CRT_PARSE_C) ) + ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDSA_C) || \ + !defined(MBEDTLS_X509_CRT_PARSE_C) ) #error "MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED defined, but not all prerequisites" #endif #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_X509_CRT_PARSE_C) ) + ( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_RSA_C) || \ + !defined(MBEDTLS_X509_CRT_PARSE_C) ) #error "MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED defined, but not all prerequisites" #endif diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 0bab0c0cb..24ba78992 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1085,7 +1085,7 @@ * * Enable the ECDH-ECDSA based ciphersuite modes in SSL / TLS. * - * Requires: MBEDTLS_ECDH_C, MBEDTLS_X509_CRT_PARSE_C + * Requires: MBEDTLS_ECDH_C, MBEDTLS_ECDSA_C, MBEDTLS_X509_CRT_PARSE_C * * This enables the following ciphersuites (if other requisites are * enabled as well): @@ -1109,7 +1109,7 @@ * * Enable the ECDH-RSA based ciphersuite modes in SSL / TLS. * - * Requires: MBEDTLS_ECDH_C, MBEDTLS_X509_CRT_PARSE_C + * Requires: MBEDTLS_ECDH_C, MBEDTLS_RSA_C, MBEDTLS_X509_CRT_PARSE_C * * This enables the following ciphersuites (if other requisites are * enabled as well): diff --git a/tests/scripts/depends-pkalgs.pl b/tests/scripts/depends-pkalgs.pl index 1577fee38..0d5d29741 100755 --- a/tests/scripts/depends-pkalgs.pl +++ b/tests/scripts/depends-pkalgs.pl @@ -50,7 +50,8 @@ my $config_h = 'include/mbedtls/config.h'; # Some algorithms can't be disabled on their own as others depend on them, so # we list those reverse-dependencies here to keep check_config.h happy. my %algs = ( - 'MBEDTLS_ECDSA_C' => ['MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED'], + 'MBEDTLS_ECDSA_C' => ['MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED', + 'MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED'], 'MBEDTLS_ECP_C' => ['MBEDTLS_ECDSA_C', 'MBEDTLS_ECDH_C', 'MBEDTLS_ECJPAKE_C', @@ -68,6 +69,7 @@ my %algs = ( 'MBEDTLS_RSA_C' => ['MBEDTLS_X509_RSASSA_PSS_SUPPORT', 'MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED', + 'MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED', 'MBEDTLS_KEY_EXCHANGE_RSA_ENABLED'], ); From d9767a579945e47b5667e336c6dec2f40dc95f72 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 14 Sep 2018 19:29:47 +0200 Subject: [PATCH 14/55] Tweak ECP self-test to work with secp192k1 The constants used in the test worked with every supported curve except secp192k1. For secp192k1, the "N-1" exponent was too large. Signed-off-by: Gilles Peskine --- library/ecp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index ab5ab9585..00917e842 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3303,11 +3303,13 @@ int mbedtls_ecp_self_test( int verbose ) mbedtls_ecp_point R, P; mbedtls_mpi m; unsigned long add_c_prev, dbl_c_prev, mul_c_prev; - /* exponents especially adapted for secp192r1 */ + /* Exponents especially adapted for secp192k1, which has the lowest + * order n of all supported curves (secp192r1 is in a slightly larger + * field but the order of its base point is slightly smaller). */ const char *exponents[] = { "000000000000000000000000000000000000000000000001", /* one */ - "FFFFFFFFFFFFFFFFFFFFFFFF99DEF836146BC9B1B4D22830", /* N - 1 */ + "FFFFFFFFFFFFFFFFFFFFFFFE26F2FC170F69466A74DEFD8C", /* n - 1 */ "5EA6F389A38B8BC81E767753B15AA5569E1782E30ABE7D25", /* random */ "400000000000000000000000000000000000000000000000", /* one and zeros */ "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", /* all ones */ From c95696fec46f8ee126acb398332e3d7c1d94b644 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 15:59:01 +0200 Subject: [PATCH 15/55] Factor common code in mbedtls_ecp_self_test No intended behavior change. Signed-off-by: Gilles Peskine --- library/ecp.c | 128 +++++++++++++++++++++++--------------------------- 1 file changed, 59 insertions(+), 69 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 00917e842..1f7943aa5 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3292,17 +3292,64 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +static int self_test_point( int verbose, + mbedtls_ecp_group *grp, + mbedtls_ecp_point *R, + mbedtls_mpi *m, + mbedtls_ecp_point *P, + const char *const *exponents, + size_t n_exponents ) +{ + int ret = 0; + size_t i; + unsigned long add_c_prev, dbl_c_prev, mul_c_prev; + add_count = 0; + dbl_count = 0; + mul_count = 0; + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[0] ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); + + for( i = 1; i < n_exponents; i++ ) + { + add_c_prev = add_count; + dbl_c_prev = dbl_count; + mul_c_prev = mul_count; + add_count = 0; + dbl_count = 0; + mul_count = 0; + + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[i] ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); + + if( add_count != add_c_prev || + dbl_count != dbl_c_prev || + mul_count != mul_c_prev ) + { + ret = 1; + break; + } + } + +cleanup: + if( verbose != 0 ) + { + if( ret != 0 ) + mbedtls_printf( "failed (%u)\n", (unsigned int) i ); + else + mbedtls_printf( "passed\n" ); + } + return( ret ); +} + /* * Checkup routine */ int mbedtls_ecp_self_test( int verbose ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - size_t i; mbedtls_ecp_group grp; mbedtls_ecp_point R, P; mbedtls_mpi m; - unsigned long add_c_prev, dbl_c_prev, mul_c_prev; /* Exponents especially adapted for secp192k1, which has the lowest * order n of all supported curves (secp192r1 is in a slightly larger * field but the order of its base point is slightly smaller). */ @@ -3330,80 +3377,23 @@ int mbedtls_ecp_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( " ECP test #1 (constant op_count, base point G): " ); - /* Do a dummy multiplication first to trigger precomputation */ MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &m, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &P, &m, &grp.G, NULL, NULL ) ); - - add_count = 0; - dbl_count = 0; - mul_count = 0; - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[0] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &grp.G, NULL, NULL ) ); - - for( i = 1; i < sizeof( exponents ) / sizeof( exponents[0] ); i++ ) - { - add_c_prev = add_count; - dbl_c_prev = dbl_count; - mul_c_prev = mul_count; - add_count = 0; - dbl_count = 0; - mul_count = 0; - - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[i] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &grp.G, NULL, NULL ) ); - - if( add_count != add_c_prev || - dbl_count != dbl_c_prev || - mul_count != mul_c_prev ) - { - if( verbose != 0 ) - mbedtls_printf( "failed (%u)\n", (unsigned int) i ); - - ret = 1; - goto cleanup; - } - } - - if( verbose != 0 ) - mbedtls_printf( "passed\n" ); + ret = self_test_point( verbose, + &grp, &R, &m, &grp.G, + exponents, + sizeof( exponents ) / sizeof( exponents[0] )); + if( ret != 0 ) + goto cleanup; if( verbose != 0 ) mbedtls_printf( " ECP test #2 (constant op_count, other point): " ); /* We computed P = 2G last time, use it */ - - add_count = 0; - dbl_count = 0; - mul_count = 0; - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[0] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &P, NULL, NULL ) ); - - for( i = 1; i < sizeof( exponents ) / sizeof( exponents[0] ); i++ ) - { - add_c_prev = add_count; - dbl_c_prev = dbl_count; - mul_c_prev = mul_count; - add_count = 0; - dbl_count = 0; - mul_count = 0; - - MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &m, 16, exponents[i] ) ); - MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &R, &m, &P, NULL, NULL ) ); - - if( add_count != add_c_prev || - dbl_count != dbl_c_prev || - mul_count != mul_c_prev ) - { - if( verbose != 0 ) - mbedtls_printf( "failed (%u)\n", (unsigned int) i ); - - ret = 1; - goto cleanup; - } - } - - if( verbose != 0 ) - mbedtls_printf( "passed\n" ); + ret = self_test_point( verbose, + &grp, &R, &m, &P, + exponents, + sizeof( exponents ) / sizeof( exponents[0] )); cleanup: From 24666795e4a079001961881d260438d7e8c65926 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:29:49 +0200 Subject: [PATCH 16/55] ECP self test: add self-test step for Montgomery curves Run some self-test both for a short Weierstrass curve and for a Montgomery curve, if the build-time configuration includes a curve of both types. Run both because there are significant differences in the implementation. The test data is suitable for Curve25519. Signed-off-by: Gilles Peskine --- library/ecp.c | 55 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 1f7943aa5..519c50adb 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3301,7 +3301,7 @@ static int self_test_point( int verbose, size_t n_exponents ) { int ret = 0; - size_t i; + size_t i = 0; unsigned long add_c_prev, dbl_c_prev, mul_c_prev; add_count = 0; dbl_count = 0; @@ -3350,10 +3350,12 @@ int mbedtls_ecp_self_test( int verbose ) mbedtls_ecp_group grp; mbedtls_ecp_point R, P; mbedtls_mpi m; + +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* Exponents especially adapted for secp192k1, which has the lowest * order n of all supported curves (secp192r1 is in a slightly larger * field but the order of its base point is slightly smaller). */ - const char *exponents[] = + const char *sw_exponents[] = { "000000000000000000000000000000000000000000000001", /* one */ "FFFFFFFFFFFFFFFFFFFFFFFE26F2FC170F69466A74DEFD8C", /* n - 1 */ @@ -3362,12 +3364,25 @@ int mbedtls_ecp_self_test( int verbose ) "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF", /* all ones */ "555555555555555555555555555555555555555555555555", /* 101010... */ }; +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + const char *m_exponents[] = + { + "4000000000000000000000000000000000000000000000000000000000000000", + "5C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C30", + "5715ECCE24583F7A7023C24164390586842E816D7280A49EF6DF4EAE6B280BF8", + "41A2B017516F6D254E1F002BCCBADD54BE30F8CEC737A0E912B4963B6BA74460", + "5555555555555555555555555555555555555555555555555555555555555550", + "7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF8", + }; +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ mbedtls_ecp_group_init( &grp ); mbedtls_ecp_point_init( &R ); mbedtls_ecp_point_init( &P ); mbedtls_mpi_init( &m ); +#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) /* Use secp192r1 if available, or any available curve */ #if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_SECP192R1 ) ); @@ -3376,24 +3391,48 @@ int mbedtls_ecp_self_test( int verbose ) #endif if( verbose != 0 ) - mbedtls_printf( " ECP test #1 (constant op_count, base point G): " ); + mbedtls_printf( " ECP SW test #1 (constant op_count, base point G): " ); /* Do a dummy multiplication first to trigger precomputation */ MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &m, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( &grp, &P, &m, &grp.G, NULL, NULL ) ); ret = self_test_point( verbose, &grp, &R, &m, &grp.G, - exponents, - sizeof( exponents ) / sizeof( exponents[0] )); + sw_exponents, + sizeof( sw_exponents ) / sizeof( sw_exponents[0] )); if( ret != 0 ) goto cleanup; if( verbose != 0 ) - mbedtls_printf( " ECP test #2 (constant op_count, other point): " ); + mbedtls_printf( " ECP SW test #2 (constant op_count, other point): " ); /* We computed P = 2G last time, use it */ ret = self_test_point( verbose, &grp, &R, &m, &P, - exponents, - sizeof( exponents ) / sizeof( exponents[0] )); + sw_exponents, + sizeof( sw_exponents ) / sizeof( sw_exponents[0] )); + if( ret != 0 ) + goto cleanup; + + mbedtls_ecp_group_free( &grp ); + mbedtls_ecp_point_free( &R ); +#endif /* MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ + +#if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + if( verbose != 0 ) + mbedtls_printf( " ECP Montgomery test (constant op_count): " ); +#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) + MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_CURVE25519 ) ); +#elif defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) + MBEDTLS_MPI_CHK( mbedtls_ecp_group_load( &grp, MBEDTLS_ECP_DP_CURVE448 ) ); +#else +#error "MBEDTLS_ECP_MONTGOMERY_ENABLED is defined, but no curve is supported for self-test" +#endif + ret = self_test_point( verbose, + &grp, &R, &m, &grp.G, + m_exponents, + sizeof( m_exponents ) / sizeof( m_exponents[0] )); + if( ret != 0 ) + goto cleanup; +#endif /* MBEDTLS_ECP_MONTGOMERY_ENABLED */ cleanup: From a088c81fcba5357d0cf44b5b107c09582e20dc81 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:31:15 +0200 Subject: [PATCH 17/55] Adjust ECP self-test to support Curve448 Adjust the Montgomery self-test to use Curve448 in builds without Curve25519. Signed-off-by: Gilles Peskine --- library/ecp.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index 519c50adb..f44298ab8 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3292,6 +3292,39 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +static int self_test_adjust_exponent( const mbedtls_ecp_group *grp, + mbedtls_mpi *m ) +{ + int ret = 0; + switch( grp->id ) + { + /* If Curve25519 is available, then that's what we use for the + * Montgomery test, so we don't need the adjustment code. */ +#if ! defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) +#if defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) + case MBEDTLS_ECP_DP_CURVE448: + /* Move highest bit from 254 to N-1. Setting bit N-1 is + * necessary to enforce the highest-bit-set constraint. */ + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( m, 254, 0 ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_set_bit( m, grp->nbits, 1 ) ); + /* Copy second-highest bit from 253 to N-2. This is not + * necessary but improves the test variety a bit. */ + MBEDTLS_MPI_CHK( + mbedtls_mpi_set_bit( m, grp->nbits - 1, + mbedtls_mpi_get_bit( m, 253 ) ) ); + break; +#endif +#endif /* ! defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) */ + default: + /* Non-Montgomery curves and Curve25519 need no adjustment. */ + (void) grp; + (void) m; + goto cleanup; + } +cleanup: + return( ret ); +} + static int self_test_point( int verbose, mbedtls_ecp_group *grp, mbedtls_ecp_point *R, @@ -3306,7 +3339,9 @@ static int self_test_point( int verbose, add_count = 0; dbl_count = 0; mul_count = 0; + MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[0] ) ); + MBEDTLS_MPI_CHK( self_test_adjust_exponent( grp, m ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); for( i = 1; i < n_exponents; i++ ) @@ -3319,6 +3354,7 @@ static int self_test_point( int verbose, mul_count = 0; MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( m, 16, exponents[i] ) ); + MBEDTLS_MPI_CHK( self_test_adjust_exponent( grp, m ) ); MBEDTLS_MPI_CHK( mbedtls_ecp_mul( grp, R, m, P, NULL, NULL ) ); if( add_count != add_c_prev || From a2611604d4dda9629e2659b77b6310de9be53b79 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 18:40:33 +0200 Subject: [PATCH 18/55] curves.pl: test with each elliptic curve enabled Previously curves.pl tested with all elliptic curves enabled except one, for each curve. This catches tests that are missing dependencies on one of the curve that they use, but does not catch misplaced conditional directives around parts of the library. Now, we additionally test with a single curve, for each curve. This catches missing or extraneous guards around code that is specific to one particular curve or to a class of curves. Signed-off-by: Gilles Peskine --- tests/scripts/curves.pl | 91 ++++++++++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 14 deletions(-) diff --git a/tests/scripts/curves.pl b/tests/scripts/curves.pl index cd6ea0d9a..8db44303c 100755 --- a/tests/scripts/curves.pl +++ b/tests/scripts/curves.pl @@ -2,7 +2,7 @@ # curves.pl # -# Copyright (c) 2014-2016, ARM Limited, All Rights Reserved +# Copyright (c) 2014-2020, ARM Limited, All Rights Reserved # SPDX-License-Identifier: Apache-2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -21,21 +21,25 @@ # # Purpose # -# To test the code dependencies on individual curves in each test suite. This -# is a verification step to ensure we don't ship test suites that do not work -# for some build options. +# The purpose of this test script is to validate that the library works +# with any combination of elliptic curves. To this effect, build the library +# and run the test suite with each tested combination of elliptic curves. # -# The process is: -# for each possible curve -# build the library and test suites with the curve disabled -# execute the test suites -# -# And any test suite with the wrong dependencies will fail. +# Testing all 2^n combinations would be too much, so we only test 2*n: # +# 1. Test with a single curve, for each curve. This validates that the +# library works with any curve, and in particular that curve-specific +# code is guarded by the proper preprocessor conditionals. +# 2. Test with all curves except one, for each curve. This validates that +# the test cases have correct dependencies. Testing with a single curve +# doesn't validate this for tests that require more than one curve. + # Usage: tests/scripts/curves.pl # # This script should be executed from the root of the project directory. # +# Only curves that are enabled in config.h will be tested. +# # For best effect, run either with cmake disabled, or cmake enabled in a mode # that includes -Werror. @@ -48,6 +52,25 @@ my $sed_cmd = 's/^#define \(MBEDTLS_ECP_DP.*_ENABLED\)/\1/p'; my $config_h = 'include/mbedtls/config.h'; my @curves = split( /\s+/, `sed -n -e '$sed_cmd' $config_h` ); +# Determine which curves support ECDSA by checking the dependencies of +# ECDSA in check_config.h. +my %curve_supports_ecdsa = (); +{ + local $/ = ""; + local *CHECK_CONFIG; + open(CHECK_CONFIG, '<', 'include/mbedtls/check_config.h') + or die "open include/mbedtls/check_config.h: $!"; + while (my $stanza = ) { + if ($stanza =~ /\A#if defined\(MBEDTLS_ECDSA_C\)/) { + for my $curve ($stanza =~ /(?<=\()MBEDTLS_ECP_DP_\w+_ENABLED(?=\))/g) { + $curve_supports_ecdsa{$curve} = 1; + } + last; + } + } + close(CHECK_CONFIG); +} + system( "cp $config_h $config_h.bak" ) and die; sub abort { system( "mv $config_h.bak $config_h" ) and warn "$config_h not restored\n"; @@ -56,6 +79,46 @@ sub abort { exit 1; } +# Disable all the curves. We'll then re-enable them one by one. +for my $curve (@curves) { + system( "scripts/config.pl unset $curve" ) + and abort "Failed to disable $curve\n"; +} +# Depends on a specific curve. Also, ignore error if it wasn't enabled. +system( "scripts/config.pl unset MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED" ); + +# Test with only $curve enabled, for each $curve. +for my $curve (@curves) { + system( "make clean" ) and die; + + print "\n******************************************\n"; + print "* Testing with only curve: $curve\n"; + print "******************************************\n"; + $ENV{MBEDTLS_TEST_CONFIGURATION} = "$curve"; + + system( "scripts/config.pl set $curve" ) + and abort "Failed to enable $curve\n"; + + my $ecdsa = $curve_supports_ecdsa{$curve} ? "set" : "unset"; + for my $dep (qw(MBEDTLS_ECDSA_C + MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED + MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED)) { + system( "scripts/config.pl $ecdsa $dep" ) + and abort "Failed to $ecdsa $dep\n"; + } + + system( "CFLAGS='-Werror -Wall -Wextra' make" ) + and abort "Failed to build: only $curve\n"; + system( "make test" ) + and abort "Failed test suite: only $curve\n"; + + system( "scripts/config.pl unset $curve" ) + and abort "Failed to disable $curve\n"; +} + +system( "cp $config_h.bak $config_h" ) and die "$config_h not restored\n"; + +# Test with $curve disabled but the others enabled, for each $curve. for my $curve (@curves) { system( "cp $config_h.bak $config_h" ) and die "$config_h not restored\n"; system( "make clean" ) and die; @@ -71,10 +134,10 @@ for my $curve (@curves) { system( "scripts/config.py unset $curve" ) and abort "Failed to disable $curve\n"; - system( "CFLAGS='-Werror -Wall -Wextra' make lib" ) - and abort "Failed to build lib: $curve\n"; - system( "make" ) and abort "Failed to build tests: $curve\n"; - system( "make test" ) and abort "Failed test suite: $curve\n"; + system( "CFLAGS='-Werror -Wall -Wextra' make" ) + and abort "Failed to build: all but $curve\n"; + system( "make test" ) + and abort "Failed test suite: all but $curve\n"; } From 599700561191916265d0c826ad6c047511dbe3d6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Feb 2019 13:12:06 +0100 Subject: [PATCH 19/55] Fix unused variables in Montgomery-only configuration Signed-off-by: Gilles Peskine --- library/ecp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index f44298ab8..0c901b0ac 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -879,6 +879,7 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp, plen = mbedtls_mpi_size( &grp->P ); #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) + (void) format; /* Montgomery curves always use the same point format */ if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_MONTGOMERY ) { *olen = plen; @@ -2653,6 +2654,8 @@ int mbedtls_ecp_mul_restartable( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, /* reset ops count for this call if top-level */ if( rs_ctx != NULL && rs_ctx->depth++ == 0 ) rs_ctx->ops_done = 0; +#else + (void) rs_ctx; #endif #if defined(MBEDTLS_ECP_INTERNAL_ALT) From 0478c2f77e6606d01b0547446fade696da1f9a36 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 17 Sep 2018 21:21:28 +0200 Subject: [PATCH 20/55] Add ChangeLog entry for single-curve build fixes Fix #941, #1412, #1147, #2017 Signed-off-by: Gilles Peskine --- ChangeLog.d/build_with_only_montgomery_curves.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/build_with_only_montgomery_curves.txt diff --git a/ChangeLog.d/build_with_only_montgomery_curves.txt b/ChangeLog.d/build_with_only_montgomery_curves.txt new file mode 100644 index 000000000..d4ec7c56c --- /dev/null +++ b/ChangeLog.d/build_with_only_montgomery_curves.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix build errors when the only enabled elliptic curves are Montgomery + curves. Raised by signpainter in #941 and by Taiki-San in #1412. This + also fixes missing declarations reported by Steven Cooreman in #1147. + * Fix self-test failure when the only enabled short Weierstrass elliptic + curve is secp192k1. Fixes #2017. From d3beca9e38637a6b3f395c476789eaa60997fd4d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 3 Jul 2020 00:15:37 +0200 Subject: [PATCH 21/55] Test Everest with only Curve25519 enabled tests/scripts/curves.pl tests the library with a single curve enabled. This uses the legacy ECDH context and the default ECDH implementation. For Curve25519, there is an alternative implementation, which is Everest. Test this. This also tests the new ECDH context, which Everest requires. Signed-off-by: Gilles Peskine --- tests/scripts/all.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index ec61d1962..558016d04 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1001,6 +1001,25 @@ component_test_everest () { if_build_succeeded tests/compat.sh -f ECDH -V NO -e 'ARCFOUR\|ARIA\|CAMELLIA\|CHACHA\|DES\|RC4' } +component_test_everest_curve25519_only () { + msg "build: Everest ECDH context, only Curve25519" # ~ 6 min + scripts/config.py unset MBEDTLS_ECDH_LEGACY_CONTEXT + scripts/config.py set MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED + scripts/config.py unset MBEDTLS_ECDSA_C + scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED + scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED + # Disable all curves + for c in $(sed -n 's/#define \(MBEDTLS_ECP_DP_[0-9A-Z_a-z]*_ENABLED\).*/\1/p' <"$CONFIG_H"); do + scripts/config.py unset "$c" + done + scripts/config.py set MBEDTLS_ECP_DP_CURVE25519_ENABLED + + make CFLAGS="$ASAN_CFLAGS -O2" LDFLAGS="$ASAN_CFLAGS" + + msg "test: Everest ECDH context, only Curve25519" # ~ 50s + make test +} + component_test_small_ssl_out_content_len () { msg "build: small SSL_OUT_CONTENT_LEN (ASan build)" scripts/config.py set MBEDTLS_SSL_IN_CONTENT_LEN 16384 From a3de08d0b53dbaee2aff957c630ceab7db83e650 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jul 2020 01:23:37 +0200 Subject: [PATCH 22/55] Reorder curve enumeration like mbedtls_ecp_group_id No semantic change. Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 7fe747302..b317d7021 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -68,12 +68,12 @@ defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) || \ defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ - defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) #define MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED #endif #if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ From 6d9c8d7b2db393b9b51d92577c077eeaff13fb73 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jul 2020 01:26:25 +0200 Subject: [PATCH 23/55] Minor documentation improvements Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 2 +- library/ecp.c | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index b317d7021..980ec5e66 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -97,7 +97,7 @@ extern "C" { /* Note: when adding a new curve: * - Add it at the end of this enum, otherwise you'll break the ABI by * changing the numerical value for existing curves. - * - Increment MBEDTLS_ECP_DP_MAX below. + * - Increment MBEDTLS_ECP_DP_MAX below if needed. * - Add the corresponding MBEDTLS_ECP_DP_xxx_ENABLED macro definition to * config.h. * - List the curve as a dependency of MBEDTLS_ECP_C and diff --git a/library/ecp.c b/library/ecp.c index 0c901b0ac..d6ef5edc4 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -3295,6 +3295,9 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +/* Adjust the exponent to be a valid private point for the specified curve. + * This is sometimes necessary because we use a single set of exponents + * for all curves but the validity of values depends on the curve. */ static int self_test_adjust_exponent( const mbedtls_ecp_group *grp, mbedtls_mpi *m ) { @@ -3328,11 +3331,13 @@ cleanup: return( ret ); } +/* Calculate R = m.P for each m in exponents. Check that the number of + * basic operations doesn't depend on the value of m. */ static int self_test_point( int verbose, mbedtls_ecp_group *grp, mbedtls_ecp_point *R, mbedtls_mpi *m, - mbedtls_ecp_point *P, + const mbedtls_ecp_point *P, const char *const *exponents, size_t n_exponents ) { @@ -3407,6 +3412,9 @@ int mbedtls_ecp_self_test( int verbose ) #if defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) const char *m_exponents[] = { + /* Valid private values for Curve25519. In a build with Curve448 + * but not Curve25519, they will be adjusted in + * self_test_adjust_exponent(). */ "4000000000000000000000000000000000000000000000000000000000000000", "5C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C3C30", "5715ECCE24583F7A7023C24164390586842E816D7280A49EF6DF4EAE6B280BF8", From 71fd80d279ab46934534c8e8f4a76e4f1b2e9ea3 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 7 Jul 2020 21:12:27 +0200 Subject: [PATCH 24/55] Re-define members of psa_key_slot_t In preparation for the implementation of the accelerator APIs. This is ramping up to the goal of only storing the export representation in the key slot, and not keeping the crypto implementation-specific representations around. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 60 +++++++++++++++++++-------------------- library/psa_crypto_core.h | 7 +++-- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 79bc9c9db..54980730c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -443,7 +443,7 @@ mbedtls_ecp_group_id mbedtls_ecc_group_of_psa( psa_ecc_family_t curve, static psa_status_t prepare_raw_data_slot( psa_key_type_t type, size_t bits, - struct raw_data *raw ) + struct key_data *key ) { /* Check that the bit size is acceptable for the key type */ switch( type ) @@ -491,11 +491,11 @@ static psa_status_t prepare_raw_data_slot( psa_key_type_t type, return( PSA_ERROR_INVALID_ARGUMENT ); /* Allocate memory for the key */ - raw->bytes = PSA_BITS_TO_BYTES( bits ); - raw->data = mbedtls_calloc( 1, raw->bytes ); - if( raw->data == NULL ) + key->bytes = PSA_BITS_TO_BYTES( bits ); + key->data = mbedtls_calloc( 1, key->bytes ); + if( key->data == NULL ) { - raw->bytes = 0; + key->bytes = 0; return( PSA_ERROR_INSUFFICIENT_MEMORY ); } return( PSA_SUCCESS ); @@ -716,7 +716,7 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) size_t bits = 0; /* return 0 on an empty slot */ if( key_type_is_raw_bytes( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( slot->data.raw.bytes ); + bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); #if defined(MBEDTLS_RSA_C) else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( slot->data.rsa ) ); @@ -751,11 +751,11 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( bit_size > PSA_MAX_KEY_BITS ) return( PSA_ERROR_NOT_SUPPORTED ); status = prepare_raw_data_slot( slot->attr.type, bit_size, - &slot->data.raw ); + &slot->data.key ); if( status != PSA_SUCCESS ) return( status ); if( data_length != 0 ) - memcpy( slot->data.raw.data, data, data_length ); + memcpy( slot->data.key.data, data, data_length ); } else #if defined(MBEDTLS_ECP_C) @@ -963,7 +963,7 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) } else if( key_type_is_raw_bytes( slot->attr.type ) ) { - mbedtls_free( slot->data.raw.data ); + mbedtls_free( slot->data.key.data ); } else #if defined(MBEDTLS_RSA_C) @@ -1306,12 +1306,12 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( key_type_is_raw_bytes( slot->attr.type ) ) { - if( slot->data.raw.bytes > data_size ) + if( slot->data.key.bytes > data_size ) return( PSA_ERROR_BUFFER_TOO_SMALL ); - memcpy( data, slot->data.raw.data, slot->data.raw.bytes ); - memset( data + slot->data.raw.bytes, 0, - data_size - slot->data.raw.bytes ); - *data_length = slot->data.raw.bytes; + memcpy( data, slot->data.key.data, slot->data.key.bytes ); + memset( data + slot->data.key.bytes, 0, + data_size - slot->data.key.bytes ); + *data_length = slot->data.key.bytes; return( PSA_SUCCESS ); } #if defined(MBEDTLS_ECP_C) @@ -2718,7 +2718,7 @@ static int psa_cmac_setup( psa_mac_operation_t *operation, return( ret ); ret = mbedtls_cipher_cmac_starts( &operation->ctx.cmac, - slot->data.raw.data, + slot->data.key.data, key_bits ); return( ret ); } @@ -2862,8 +2862,8 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, } status = psa_hmac_setup_internal( &operation->ctx.hmac, - slot->data.raw.data, - slot->data.raw.bytes, + slot->data.key.data, + slot->data.key.bytes, hash_alg ); } else @@ -3795,8 +3795,8 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, { /* Two-key Triple-DES is 3-key Triple-DES with K1=K3 */ uint8_t keys[24]; - memcpy( keys, slot->data.raw.data, 16 ); - memcpy( keys + 16, slot->data.raw.data, 8 ); + memcpy( keys, slot->data.key.data, 16 ); + memcpy( keys + 16, slot->data.key.data, 8 ); ret = mbedtls_cipher_setkey( &operation->ctx.cipher, keys, 192, cipher_operation ); @@ -3805,7 +3805,7 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, #endif { ret = mbedtls_cipher_setkey( &operation->ctx.cipher, - slot->data.raw.data, + slot->data.key.data, (int) key_bits, cipher_operation ); } if( ret != 0 ) @@ -4137,7 +4137,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_ccm_init( &operation->ctx.ccm ); status = mbedtls_to_psa_error( mbedtls_ccm_setkey( &operation->ctx.ccm, cipher_id, - operation->slot->data.raw.data, + operation->slot->data.key.data, (unsigned int) key_bits ) ); if( status != 0 ) goto cleanup; @@ -4156,7 +4156,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_gcm_init( &operation->ctx.gcm ); status = mbedtls_to_psa_error( mbedtls_gcm_setkey( &operation->ctx.gcm, cipher_id, - operation->slot->data.raw.data, + operation->slot->data.key.data, (unsigned int) key_bits ) ); if( status != 0 ) goto cleanup; @@ -4173,7 +4173,7 @@ static psa_status_t psa_aead_setup( aead_operation_t *operation, mbedtls_chachapoly_init( &operation->ctx.chachapoly ); status = mbedtls_to_psa_error( mbedtls_chachapoly_setkey( &operation->ctx.chachapoly, - operation->slot->data.raw.data ) ); + operation->slot->data.key.data ) ); if( status != 0 ) goto cleanup; break; @@ -5246,8 +5246,8 @@ psa_status_t psa_key_derivation_input_key( return( psa_key_derivation_input_internal( operation, step, slot->attr.type, - slot->data.raw.data, - slot->data.raw.bytes ) ); + slot->data.key.data, + slot->data.key.bytes ) ); } @@ -5525,17 +5525,17 @@ static psa_status_t psa_generate_key_internal( if( key_type_is_raw_bytes( type ) ) { psa_status_t status; - status = prepare_raw_data_slot( type, bits, &slot->data.raw ); + status = prepare_raw_data_slot( type, bits, &slot->data.key ); if( status != PSA_SUCCESS ) return( status ); - status = psa_generate_random( slot->data.raw.data, - slot->data.raw.bytes ); + status = psa_generate_random( slot->data.key.data, + slot->data.key.bytes ); if( status != PSA_SUCCESS ) return( status ); #if defined(MBEDTLS_DES_C) if( type == PSA_KEY_TYPE_DES ) - psa_des_set_key_parity( slot->data.raw.data, - slot->data.raw.bytes ); + psa_des_set_key_parity( slot->data.key.data, + slot->data.key.bytes ); #endif /* MBEDTLS_DES_C */ } else diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index ef40f7994..8af45a17d 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -43,12 +43,13 @@ typedef struct psa_core_key_attributes_t attr; union { - /* Raw-data key (key_type_is_raw_bytes() in psa_crypto.c) */ - struct raw_data + /* Dynamically allocated key data buffer. + * Format as specified in psa_export_key(). */ + struct key_data { uint8_t *data; size_t bytes; - } raw; + } key; #if defined(MBEDTLS_RSA_C) /* RSA public key or key pair */ mbedtls_rsa_context *rsa; From 81be2fa0b24a9aaa3b5b5344fdba5857cdb13b7e Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 22:04:59 +0200 Subject: [PATCH 25/55] Pull apart slot memory allocation from key validation. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 64 ++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 54980730c..e2e99d7d1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -441,9 +441,8 @@ mbedtls_ecp_group_id mbedtls_ecc_group_of_psa( psa_ecc_family_t curve, } #endif /* defined(MBEDTLS_ECP_C) */ -static psa_status_t prepare_raw_data_slot( psa_key_type_t type, - size_t bits, - struct key_data *key ) +static psa_status_t validate_unstructured_key_bit_size( psa_key_type_t type, + size_t bits ) { /* Check that the bit size is acceptable for the key type */ switch( type ) @@ -490,14 +489,6 @@ static psa_status_t prepare_raw_data_slot( psa_key_type_t type, if( bits % 8 != 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - /* Allocate memory for the key */ - key->bytes = PSA_BITS_TO_BYTES( bits ); - key->data = mbedtls_calloc( 1, key->bytes ); - if( key->data == NULL ) - { - key->bytes = 0; - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } return( PSA_SUCCESS ); } @@ -740,22 +731,42 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, { psa_status_t status = PSA_SUCCESS; + /* zero-length keys are never supported. */ + if( data_length == 0 ) + return( PSA_ERROR_NOT_SUPPORTED ); + + /* Ensure that the bytes-to-bit conversion never overflows. */ + if( data_length > SIZE_MAX / 8 ) + return( PSA_ERROR_NOT_SUPPORTED ); + if( key_type_is_raw_bytes( slot->attr.type ) ) { size_t bit_size = PSA_BYTES_TO_BITS( data_length ); - /* Ensure that the bytes-to-bit conversion didn't overflow. */ - if( data_length > SIZE_MAX / 8 ) - return( PSA_ERROR_NOT_SUPPORTED ); + /* Enforce a size limit, and in particular ensure that the bit * size fits in its representation type. */ if( bit_size > PSA_MAX_KEY_BITS ) return( PSA_ERROR_NOT_SUPPORTED ); - status = prepare_raw_data_slot( slot->attr.type, bit_size, - &slot->data.key ); + + status = validate_unstructured_key_bit_size( slot->attr.type, bit_size ); if( status != PSA_SUCCESS ) - return( status ); - if( data_length != 0 ) - memcpy( slot->data.key.data, data, data_length ); + return status; + + /* Allocate memory for the key */ + slot->data.key.data = mbedtls_calloc( 1, data_length ); + if( slot->data.key.data == NULL ) + { + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + slot->data.key.bytes = data_length; + + /* copy key into allocated buffer */ + memcpy(slot->data.key.data, data, data_length); + + /* Write the actual key size to the slot. + * psa_start_key_creation() wrote the size declared by the + * caller, which may be 0 (meaning unspecified) or wrong. */ + slot->attr.bits = (psa_key_bits_t) bit_size; } else #if defined(MBEDTLS_ECP_C) @@ -5525,13 +5536,26 @@ static psa_status_t psa_generate_key_internal( if( key_type_is_raw_bytes( type ) ) { psa_status_t status; - status = prepare_raw_data_slot( type, bits, &slot->data.key ); + + status = validate_unstructured_key_bit_size( slot->attr.type, bits ); if( status != PSA_SUCCESS ) return( status ); + + /* Allocate memory for the key */ + slot->data.key.bytes = PSA_BITS_TO_BYTES( bits ); + slot->data.key.data = mbedtls_calloc( 1, slot->data.key.bytes ); + if( slot->data.key.data == NULL ) + { + slot->data.key.bytes = 0; + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + status = psa_generate_random( slot->data.key.data, slot->data.key.bytes ); if( status != PSA_SUCCESS ) return( status ); + + slot->attr.bits = (psa_key_bits_t) bits; #if defined(MBEDTLS_DES_C) if( type == PSA_KEY_TYPE_DES ) psa_des_set_key_parity( slot->data.key.data, From a01795d6090361e53afbafd731ec0357a95d0cba Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 22:48:15 +0200 Subject: [PATCH 26/55] Remove RSA internal representation from key slot Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 380 +++++++++++++++++++++++++++++++------- library/psa_crypto_core.h | 5 - 2 files changed, 315 insertions(+), 70 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e2e99d7d1..6f374b12b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -492,7 +492,9 @@ static psa_status_t validate_unstructured_key_bit_size( psa_key_type_t type, return( PSA_SUCCESS ); } -#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) +#if defined(MBEDTLS_RSA_C) + +#if defined(MBEDTLS_PK_PARSE_C) /* Mbed TLS doesn't support non-byte-aligned key sizes (i.e. key sizes * that are not a multiple of 8) well. For example, there is only * mbedtls_rsa_get_len(), which returns a number of bytes, and no @@ -514,63 +516,201 @@ static psa_status_t psa_check_rsa_key_byte_aligned( mbedtls_mpi_free( &n ); return( status ); } +#endif /* MBEDTLS_PK_PARSE_C */ -static psa_status_t psa_import_rsa_key( psa_key_type_t type, - const uint8_t *data, - size_t data_length, - mbedtls_rsa_context **p_rsa ) +/** Load the contents of a key slot into an internal RSA representation + * + * \param[in] slot The slot from which to load the representation + * \param[out] rsa The internal RSA representation to hold the key. Must be + * allocated and initialized. If it already holds a + * different key, it will be overwritten and cause a memory + * leak. + */ +static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, + mbedtls_rsa_context *rsa ) { +#if defined(MBEDTLS_PK_PARSE_C) psa_status_t status; - mbedtls_pk_context pk; - mbedtls_rsa_context *rsa; + mbedtls_pk_context ctx; size_t bits; - - mbedtls_pk_init( &pk ); + mbedtls_pk_init( &ctx ); /* Parse the data. */ - if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) + if( PSA_KEY_TYPE_IS_KEY_PAIR( slot->attr.type ) ) status = mbedtls_to_psa_error( - mbedtls_pk_parse_key( &pk, data, data_length, NULL, 0 ) ); + mbedtls_pk_parse_key( &ctx, slot->data.key.data, slot->data.key.bytes, NULL, 0 ) ); else status = mbedtls_to_psa_error( - mbedtls_pk_parse_public_key( &pk, data, data_length ) ); + mbedtls_pk_parse_public_key( &ctx, slot->data.key.data, slot->data.key.bytes ) ); if( status != PSA_SUCCESS ) goto exit; /* We have something that the pkparse module recognizes. If it is a * valid RSA key, store it. */ - if( mbedtls_pk_get_type( &pk ) != MBEDTLS_PK_RSA ) + if( mbedtls_pk_get_type( &ctx ) != MBEDTLS_PK_RSA ) { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; } - rsa = mbedtls_pk_rsa( pk ); /* The size of an RSA key doesn't have to be a multiple of 8. Mbed TLS * supports non-byte-aligned key sizes, but not well. For example, * mbedtls_rsa_get_len() returns the key size in bytes, not in bits. */ - bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( rsa ) ); + bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( mbedtls_pk_rsa( ctx ) ) ); if( bits > PSA_VENDOR_RSA_MAX_KEY_BITS ) { status = PSA_ERROR_NOT_SUPPORTED; goto exit; } - status = psa_check_rsa_key_byte_aligned( rsa ); + status = psa_check_rsa_key_byte_aligned( mbedtls_pk_rsa( ctx ) ); + + if( status != PSA_SUCCESS ) + goto exit; + + /* Copy the PK-contained RSA context to the one provided as function input */ + status = mbedtls_to_psa_error( + mbedtls_rsa_copy( rsa, mbedtls_pk_rsa( ctx ) ) ); exit: - /* Free the content of the pk object only on error. */ + mbedtls_pk_free( &ctx ); + return( status ); +#else + (void) slot; + (void) rsa; + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* MBEDTLS_PK_PARSE_C */ +} + +/** Export an RSA key to export representation + * + * \param[in] type The type of key (public/private) to export + * \param[in] rsa The internal RSA representation from which to export + * \param[out] data The buffer to export to + * \param[in] data_size The length of the buffer to export to + * \param[out] data_length The amount of bytes written to \p data + */ +static psa_status_t psa_export_rsa_key( psa_key_type_t type, + mbedtls_rsa_context *rsa, + uint8_t *data, + size_t data_size, + size_t *data_length ) +{ +#if defined(MBEDTLS_PK_WRITE_C) + int ret; + mbedtls_pk_context pk; + uint8_t *pos = data + data_size; + + mbedtls_pk_init( &pk ); + pk.pk_info = &mbedtls_rsa_info; + pk.pk_ctx = rsa; + + /* PSA Crypto API defines the format of an RSA key as a DER-encoded + * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey + * or the RFC3279 RSAPublicKey for a private key or a public key. */ + if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) + ret = mbedtls_pk_write_key_der( &pk, data, data_size ); + else + ret = mbedtls_pk_write_pubkey( &pos, data, &pk ); + + if( ret < 0 ) + return mbedtls_to_psa_error( ret ); + + /* The mbedtls_pk_xxx functions write to the end of the buffer. + * Move the data to the beginning and erase remaining data + * at the original location. */ + if( 2 * (size_t) ret <= data_size ) + { + memcpy( data, data + data_size - ret, ret ); + memset( data + data_size - ret, 0, ret ); + } + else if( (size_t) ret < data_size ) + { + memmove( data, data + data_size - ret, ret ); + memset( data + ret, 0, data_size - ret ); + } + + *data_length = ret; + return( PSA_SUCCESS ); +#else + (void) type; + (void) rsa; + (void) data; + (void) data_size; + (void) data_length; + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* MBEDTLS_PK_WRITE_C */ +} + +/** Import an RSA key from import representation to a slot + * + * \param[in,out] slot The slot where to store the export representation to + * \param[in] data The buffer containing the import representation + * \param[in] data_length The amount of bytes in \p data + */ +static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, + const uint8_t *data, + size_t data_length ) +{ + psa_status_t status; + uint8_t* output = NULL; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + /* Temporarily load input into slot. The cast here is safe since it'll + * only be used for load_rsa_representation, which doesn't modify the + * buffer. */ + slot->data.key.data = (uint8_t *)data; + slot->data.key.bytes = data_length; + + /* Parse input */ + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + goto exit; + + slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( + mbedtls_rsa_get_len( &rsa ) ); + + /* Re-export the data to PSA export format, which in case of RSA is the + * smallest representation we can parse. */ + output = mbedtls_calloc( 1, data_length ); + + if( output == NULL ) + { + status = PSA_ERROR_INSUFFICIENT_MEMORY; + goto exit; + } + + /* PSA Crypto API defines the format of an RSA key as a DER-encoded + * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey + * or the RFC3279 RSAPublicKey for a private key or a public key. That + * means we have no other choice then to run an import to verify the key + * size. */ + status = psa_export_rsa_key( slot->attr.type, + &rsa, + output, + data_length, + &data_length); + +exit: + /* Always free the RSA object */ + mbedtls_rsa_free( &rsa ); + + /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) { - mbedtls_pk_free( &pk ); + mbedtls_free( output ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; return( status ); } - /* On success, store the content of the object in the RSA context. */ - *p_rsa = rsa; + /* On success, store the allocated export-formatted key. */ + slot->data.key.data = output; + slot->data.key.bytes = data_length; return( PSA_SUCCESS ); } -#endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ +#endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, @@ -708,10 +848,6 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) if( key_type_is_raw_bytes( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); -#if defined(MBEDTLS_RSA_C) - else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( slot->data.rsa ) ); -#endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) bits = slot->data.ecp->grp.pbits; @@ -788,9 +924,7 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, #if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - status = psa_import_rsa_key( slot->attr.type, - data, data_length, - &slot->data.rsa ); + status = psa_import_rsa_key( slot, data, data_length ); } else #endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ @@ -800,10 +934,13 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( status == PSA_SUCCESS ) { - /* Write the actual key size to the slot. - * psa_start_key_creation() wrote the size declared by the - * caller, which may be 0 (meaning unspecified) or wrong. */ - slot->attr.bits = psa_calculate_key_bits( slot ); + if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + { + /* Write the actual key size to the slot. + * psa_start_key_creation() wrote the size declared by the + * caller, which may be 0 (meaning unspecified) or wrong. */ + slot->attr.bits = psa_calculate_key_bits( slot ); + } } return( status ); } @@ -980,8 +1117,9 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_free( slot->data.rsa ); - mbedtls_free( slot->data.rsa ); + mbedtls_free( slot->data.key.data ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -1232,7 +1370,18 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, if( psa_key_slot_is_external( slot ) ) break; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ - status = psa_get_rsa_public_exponent( slot->data.rsa, attributes ); + { + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + break; + + status = psa_get_rsa_public_exponent( &rsa, + attributes ); + mbedtls_rsa_free( &rsa ); + } break; #endif /* MBEDTLS_RSA_C */ default: @@ -1276,6 +1425,20 @@ static int pk_write_pubkey_simple( mbedtls_pk_context *key, } #endif /* defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) */ +static psa_status_t psa_internal_export_key_buffer( const psa_key_slot_t *slot, + uint8_t *data, + size_t data_size, + size_t *data_length ) +{ + if( slot->data.key.bytes > data_size ) + return( PSA_ERROR_BUFFER_TOO_SMALL ); + memcpy( data, slot->data.key.data, slot->data.key.bytes ); + memset( data + slot->data.key.bytes, 0, + data_size - slot->data.key.bytes ); + *data_length = slot->data.key.bytes; + return( PSA_SUCCESS ); +} + static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, uint8_t *data, size_t data_size, @@ -1354,10 +1517,36 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) - mbedtls_pk_init( &pk ); - pk.pk_info = &mbedtls_rsa_info; - pk.pk_ctx = slot->data.rsa; + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Exporting public -> public */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + else if( !export_public_key ) + { + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + + /* Exporting private -> public */ + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, + &rsa, + data, + data_size, + data_length ); + + mbedtls_rsa_free( &rsa ); + + return( status ); #else + /* We don't know how to convert a private RSA key to public. */ return( PSA_ERROR_NOT_SUPPORTED ); #endif } @@ -1805,12 +1994,19 @@ static psa_status_t psa_validate_optional_attributes( #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; mbedtls_mpi actual, required; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); - ret = mbedtls_rsa_export( slot->data.rsa, + ret = mbedtls_rsa_export( &rsa, NULL, NULL, NULL, NULL, &actual ); + mbedtls_rsa_free( &rsa ); if( ret != 0 ) goto rsa_exit; ret = mbedtls_mpi_read_binary( &required, @@ -3447,11 +3643,21 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - status = psa_rsa_sign( slot->data.rsa, + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, + &rsa ); + if( status != PSA_SUCCESS ) + goto exit; + + status = psa_rsa_sign( &rsa, alg, hash, hash_length, signature, signature_size, signature_length ); + + mbedtls_rsa_free( &rsa ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3533,10 +3739,19 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - return( psa_rsa_verify( slot->data.rsa, - alg, - hash, hash_length, - signature, signature_length ) ); + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_rsa_verify( &rsa, + alg, + hash, hash_length, + signature, signature_length ); + mbedtls_rsa_free( &rsa ); + return( status ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3606,14 +3821,22 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context *rsa = slot->data.rsa; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - if( output_size < mbedtls_rsa_get_len( rsa ) ) + if( output_size < mbedtls_rsa_get_len( &rsa ) ) + { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_BUFFER_TOO_SMALL ); + } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_encrypt( rsa, + ret = mbedtls_rsa_pkcs1_encrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3626,8 +3849,8 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_encrypt( rsa, + psa_rsa_oaep_set_padding_mode( alg, &rsa ); + ret = mbedtls_rsa_rsaes_oaep_encrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3639,10 +3862,13 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } if( ret == 0 ) - *output_length = mbedtls_rsa_get_len( rsa ); + *output_length = mbedtls_rsa_get_len( &rsa ); + + mbedtls_rsa_free( &rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -3685,16 +3911,24 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context *rsa = slot->data.rsa; + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + + status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - if( input_length != mbedtls_rsa_get_len( rsa ) ) + if( input_length != mbedtls_rsa_get_len( &rsa ) ) + { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); + } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_decrypt( rsa, + ret = mbedtls_rsa_pkcs1_decrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3708,8 +3942,8 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, rsa ); - ret = mbedtls_rsa_rsaes_oaep_decrypt( rsa, + psa_rsa_oaep_set_padding_mode( alg, &rsa ); + ret = mbedtls_rsa_rsaes_oaep_decrypt( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3722,9 +3956,11 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { + mbedtls_rsa_free( &rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } + mbedtls_rsa_free( &rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -5567,7 +5803,7 @@ static psa_status_t psa_generate_key_internal( #if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_GENPRIME) if ( type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context *rsa; + mbedtls_rsa_context rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; int exponent; psa_status_t status; @@ -5582,22 +5818,36 @@ static psa_status_t psa_generate_key_internal( &exponent ); if( status != PSA_SUCCESS ) return( status ); - rsa = mbedtls_calloc( 1, sizeof( *rsa ) ); - if( rsa == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_rsa_init( rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - ret = mbedtls_rsa_gen_key( rsa, + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + ret = mbedtls_rsa_gen_key( &rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, (unsigned int) bits, exponent ); if( ret != 0 ) - { - mbedtls_rsa_free( rsa ); - mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); + + /* Make sure to always have an export representation available */ + size_t bytes = PSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZE( bits ); + + slot->data.key.data = mbedtls_calloc( 1, bytes ); + if( slot->data.key.data == NULL ) + { + mbedtls_rsa_free( &rsa ); + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + + status = psa_export_rsa_key( type, + &rsa, + slot->data.key.data, + bytes, + &slot->data.key.bytes ); + mbedtls_rsa_free( &rsa ); + if( status != PSA_SUCCESS ) + { + psa_remove_key_data_from_memory( slot ); + return( status ); } - slot->data.rsa = rsa; } else #endif /* MBEDTLS_RSA_C && MBEDTLS_GENPRIME */ diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index 8af45a17d..c90d7375a 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -33,7 +33,6 @@ #include "psa/crypto_se_driver.h" #include "mbedtls/ecp.h" -#include "mbedtls/rsa.h" /** The data structure representing a key slot, containing key material * and metadata for one key. @@ -50,10 +49,6 @@ typedef struct uint8_t *data; size_t bytes; } key; -#if defined(MBEDTLS_RSA_C) - /* RSA public key or key pair */ - mbedtls_rsa_context *rsa; -#endif /* MBEDTLS_RSA_C */ #if defined(MBEDTLS_ECP_C) /* EC public key or key pair */ mbedtls_ecp_keypair *ecp; From acda8346bf3ab9dbffbf8a56b09f45e0ed8f795d Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:09:52 +0200 Subject: [PATCH 27/55] Remove ECP internal representation from key slot Change to on-demand loading of the internal representation when required in order to call an mbed TLS cryptography API. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 423 ++++++++++++++++++++++---------------- library/psa_crypto_core.h | 6 - 2 files changed, 242 insertions(+), 187 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6f374b12b..ee616956a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -713,18 +713,17 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, - size_t data_length, - int is_public, - mbedtls_ecp_keypair **p_ecp ) +/* Load the key slot contents into an mbedTLS internal representation object. + * Note: caller is responsible for freeing the object properly */ +static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, + mbedtls_ecp_keypair *ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; - *p_ecp = mbedtls_calloc( 1, sizeof( mbedtls_ecp_keypair ) ); - if( *p_ecp == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_ecp_keypair_init( *p_ecp ); + size_t data_length = slot->data.key.bytes; + psa_status_t status; + mbedtls_ecp_keypair_init( ecp ); - if( is_public ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { /* A public key is represented as: * - The byte 0x04; @@ -732,99 +731,169 @@ static psa_status_t psa_prepare_import_ec_key( psa_ecc_family_t curve, * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. * So its data length is 2m+1 where n is the key size in bits. */ - if( ( data_length & 1 ) == 0 ) + if( ( slot->data.key.bytes & 1 ) == 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - data_length = data_length / 2; + data_length = slot->data.key.bytes / 2; } /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( curve, data_length ); + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), + data_length ); if( grp_id == MBEDTLS_ECP_DP_NONE ) return( PSA_ERROR_INVALID_ARGUMENT ); - return( mbedtls_to_psa_error( - mbedtls_ecp_group_load( &( *p_ecp )->grp, grp_id ) ) ); + status = mbedtls_to_psa_error( + mbedtls_ecp_group_load( &ecp->grp, grp_id ) ); + if( status != PSA_SUCCESS ) + return( status ); + + /* Load the key material */ + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Load the public value. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, + slot->data.key.data, + slot->data.key.bytes ) ); + if( status != PSA_SUCCESS ) + goto exit; + + /* Check that the point is on the curve. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_check_pubkey( &ecp->grp, &ecp->Q ) ); + if( status != PSA_SUCCESS ) + goto exit; + } + else + { + /* Load the secret value. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_read_key( ecp->grp.id, + ecp, + slot->data.key.data, + slot->data.key.bytes ) ); + + if( status != PSA_SUCCESS ) + goto exit; + /* Validate the private key. */ + status = mbedtls_to_psa_error( + mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) ); + if( status != PSA_SUCCESS ) + goto exit; + } +exit: + if( status != PSA_SUCCESS ) + mbedtls_ecp_keypair_free( ecp ); + return status; } -/* Import a public key given as the uncompressed representation defined by SEC1 - * 2.3.3 as the content of an ECPoint. */ -static psa_status_t psa_import_ec_public_key( psa_ecc_family_t curve, - const uint8_t *data, - size_t data_length, - mbedtls_ecp_keypair **p_ecp ) +static psa_status_t psa_export_ecp_key( psa_key_type_t type, + mbedtls_ecp_keypair *ecp, + uint8_t *data, + size_t data_size, + size_t *data_length ) { - psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - mbedtls_ecp_keypair *ecp = NULL; + psa_status_t status; - status = psa_prepare_import_ec_key( curve, data_length, 1, &ecp ); - if( status != PSA_SUCCESS ) - goto exit; - - /* Load the public value. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, - data, data_length ) ); - if( status != PSA_SUCCESS ) - goto exit; - - /* Check that the point is on the curve. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_check_pubkey( &ecp->grp, &ecp->Q ) ); - if( status != PSA_SUCCESS ) - goto exit; - - *p_ecp = ecp; - return( PSA_SUCCESS ); - -exit: - if( ecp != NULL ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + /* Check whether the public part is loaded */ + if( mbedtls_ecp_is_zero( &ecp->Q ) ) + { + /* Calculate the public key */ + status = mbedtls_to_psa_error( + mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, + mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); + if( status != PSA_SUCCESS ) + return status; + } + + return( mbedtls_to_psa_error( + mbedtls_ecp_point_write_binary( &ecp->grp, &ecp->Q, + MBEDTLS_ECP_PF_UNCOMPRESSED, + data_length, + data, + data_size ) ) ); + } + else + { + if( data_size < PSA_BITS_TO_BYTES(ecp->grp.nbits) ) + return( PSA_ERROR_BUFFER_TOO_SMALL ); + + status = mbedtls_to_psa_error( + mbedtls_ecp_write_key( ecp, + data, + PSA_BITS_TO_BYTES(ecp->grp.nbits) ) ); + if( status == PSA_SUCCESS ) + { + *data_length = PSA_BITS_TO_BYTES(ecp->grp.nbits); + } + + return( status ); } - return( status ); } -/* Import a private key given as a byte string which is the private value - * in big-endian order. */ -static psa_status_t psa_import_ec_private_key( psa_ecc_family_t curve, - const uint8_t *data, - size_t data_length, - mbedtls_ecp_keypair **p_ecp ) +static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, + const uint8_t *data, + size_t data_length ) { - psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; - mbedtls_ecp_keypair *ecp = NULL; + psa_status_t status; + uint8_t* output = NULL; + mbedtls_ecp_keypair ecp; - status = psa_prepare_import_ec_key( curve, data_length, 0, &ecp ); + /* Temporarily load input into slot. The cast here is safe since it'll + * only be used for load_ecp_representation, which doesn't modify the + * buffer. */ + slot->data.key.data = (uint8_t *)data; + slot->data.key.bytes = data_length; + + /* Parse input */ + status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) goto exit; - /* Load and validate the secret key */ - status = mbedtls_to_psa_error( - mbedtls_ecp_read_key( ecp->grp.id, ecp, data, data_length ) ); - if( status != PSA_SUCCESS ) - goto exit; + if( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) == PSA_ECC_FAMILY_MONTGOMERY) + slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits + 1; + else + slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits; - /* Calculate the public key from the private key. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, - mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); - if( status != PSA_SUCCESS ) - goto exit; + /* Re-export the data to PSA export format. There is currently no support + * for other input formats then the export format, so this is a 1-1 + * copy operation. */ + output = mbedtls_calloc( 1, data_length ); - *p_ecp = ecp; - return( PSA_SUCCESS ); + if( output == NULL ) + { + status = PSA_ERROR_INSUFFICIENT_MEMORY; + goto exit; + } + + status = psa_export_ecp_key( slot->attr.type, + &ecp, + output, + data_length, + &data_length); exit: - if( ecp != NULL ) + /* Always free the PK object (will also free contained RSA context) */ + mbedtls_ecp_keypair_free( &ecp ); + + /* Free the allocated buffer only on error. */ + if( status != PSA_SUCCESS ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + mbedtls_free( output ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; + return( status ); } - return( status ); + + /* On success, store the allocated export-formatted key. */ + slot->data.key.data = output; + slot->data.key.bytes = data_length; + + return( PSA_SUCCESS ); } #endif /* defined(MBEDTLS_ECP_C) */ - /** Return the size of the key in the given slot, in bits. * * \param[in] slot A key slot. @@ -848,10 +917,6 @@ static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) if( key_type_is_raw_bytes( slot->attr.type ) ) bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); -#if defined(MBEDTLS_ECP_C) - else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - bits = slot->data.ecp->grp.pbits; -#endif /* defined(MBEDTLS_ECP_C) */ /* We know that the size fits in psa_key_bits_t thanks to checks * when the key was created. */ @@ -906,18 +971,9 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, } else #if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - status = psa_import_ec_private_key( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data, data_length, - &slot->data.ecp ); - } - else if( PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY( slot->attr.type ) ) - { - status = psa_import_ec_public_key( - PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data, data_length, - &slot->data.ecp ); + status = psa_import_ecp_key( slot, data, data_length ); } else #endif /* MBEDTLS_ECP_C */ @@ -934,7 +990,8 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( status == PSA_SUCCESS ) { - if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) && + !PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { /* Write the actual key size to the slot. * psa_start_key_creation() wrote the size declared by the @@ -1126,8 +1183,9 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - mbedtls_ecp_keypair_free( slot->data.ecp ); - mbedtls_free( slot->data.ecp ); + mbedtls_free( slot->data.key.data ); + slot->data.key.data = NULL; + slot->data.key.bytes = 0; } else #endif /* defined(MBEDTLS_ECP_C) */ @@ -1409,22 +1467,6 @@ psa_status_t psa_get_key_slot_number( } #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ -#if defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) -static int pk_write_pubkey_simple( mbedtls_pk_context *key, - unsigned char *buf, size_t size ) -{ - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char *c; - size_t len = 0; - - c = buf + size; - - MBEDTLS_ASN1_CHK_ADD( len, mbedtls_pk_write_pubkey( &c, buf, key ) ); - - return( (int) len ); -} -#endif /* defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C) */ - static psa_status_t psa_internal_export_key_buffer( const psa_key_slot_t *slot, uint8_t *data, size_t data_size, @@ -1491,29 +1533,15 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) && !export_public_key ) { - psa_status_t status; - - size_t bytes = PSA_BITS_TO_BYTES( slot->attr.bits ); - if( bytes > data_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - status = mbedtls_to_psa_error( - mbedtls_ecp_write_key( slot->data.ecp, - data, bytes ) ); - if( status != PSA_SUCCESS ) - return( status ); - memset( data + bytes, 0, data_size - bytes ); - *data_length = bytes; - return( PSA_SUCCESS ); + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); } #endif else { -#if defined(MBEDTLS_PK_WRITE_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - mbedtls_pk_context pk; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) @@ -1553,44 +1581,28 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, else { #if defined(MBEDTLS_ECP_C) - mbedtls_pk_init( &pk ); - pk.pk_info = &mbedtls_eckey_info; - pk.pk_ctx = slot->data.ecp; + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( + PSA_KEY_TYPE_ECC_GET_FAMILY( + slot->attr.type ) ), + &ecp, + data, + data_size, + data_length ); + + mbedtls_ecp_keypair_free( &ecp ); + return( status ); #else + /* We don't know how to convert a private ECC key to public. */ return( PSA_ERROR_NOT_SUPPORTED ); -#endif +#endif /* defined(MBEDTLS_ECP_C) */ } - if( export_public_key || PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) - { - ret = pk_write_pubkey_simple( &pk, data, data_size ); - } - else - { - ret = mbedtls_pk_write_key_der( &pk, data, data_size ); - } - if( ret < 0 ) - { - memset( data, 0, data_size ); - return( mbedtls_to_psa_error( ret ) ); - } - /* The mbedtls_pk_xxx functions write to the end of the buffer. - * Move the data to the beginning and erase remaining data - * at the original location. */ - if( 2 * (size_t) ret <= data_size ) - { - memcpy( data, data + data_size - ret, ret ); - memset( data + data_size - ret, 0, ret ); - } - else if( (size_t) ret < data_size ) - { - memmove( data, data + data_size - ret, ret ); - memset( data + ret, 0, data_size - ret ); - } - *data_length = ret; - return( PSA_SUCCESS ); } else -#endif /* defined(MBEDTLS_PK_WRITE_C) */ { /* This shouldn't happen in the reference implementation, but it is valid for a special-purpose implementation to omit @@ -3580,6 +3592,14 @@ static psa_status_t psa_ecdsa_verify( mbedtls_ecp_keypair *ecp, signature + curve_bytes, curve_bytes ) ); + /* Check whether the public part is loaded. If not, load it. */ + if( mbedtls_ecp_is_zero( &ecp->Q ) ) + { + MBEDTLS_MPI_CHK( + mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d, &ecp->grp.G, + mbedtls_ctr_drbg_random, &global_data.ctr_drbg ) ); + } + ret = mbedtls_ecdsa_verify( &ecp->grp, hash, hash_length, &ecp->Q, &r, &s ); @@ -3672,11 +3692,18 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, PSA_ALG_IS_RANDOMIZED_ECDSA( alg ) #endif ) - status = psa_ecdsa_sign( slot->data.ecp, + { + mbedtls_ecp_keypair ecp; + status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + goto exit; + status = psa_ecdsa_sign( &ecp, alg, hash, hash_length, signature, signature_size, signature_length ); + mbedtls_ecp_keypair_free( &ecp ); + } else #endif /* defined(MBEDTLS_ECDSA_C) */ { @@ -3760,9 +3787,17 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, { #if defined(MBEDTLS_ECDSA_C) if( PSA_ALG_IS_ECDSA( alg ) ) - return( psa_ecdsa_verify( slot->data.ecp, - hash, hash_length, - signature, signature_length ) ); + { + mbedtls_ecp_keypair ecp; + status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + status = psa_ecdsa_verify( &ecp, + hash, hash_length, + signature, signature_length ); + mbedtls_ecp_keypair_free( &ecp ); + return status; + } else #endif /* defined(MBEDTLS_ECDSA_C) */ { @@ -5511,21 +5546,26 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t shared_secret_size, size_t *shared_secret_length ) { - mbedtls_ecp_keypair *their_key = NULL; + mbedtls_ecp_keypair their_key; + psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; size_t bits = 0; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( our_key->grp.id, &bits ); mbedtls_ecdh_init( &ecdh ); + memset(&their_key_slot, 0, sizeof(their_key_slot)); - status = psa_import_ec_public_key( curve, - peer_key, peer_key_length, - &their_key ); + /* Creating ephemeral key slot for import purposes */ + their_key_slot.attr.type = PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve); + their_key_slot.data.key.data = (uint8_t*) peer_key; + their_key_slot.data.key.bytes = peer_key_length; + + status = psa_load_ecp_representation( &their_key_slot, &their_key ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( - mbedtls_ecdh_get_params( &ecdh, their_key, MBEDTLS_ECDH_THEIRS ) ); + mbedtls_ecdh_get_params( &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( @@ -5548,8 +5588,7 @@ exit: if( status != PSA_SUCCESS ) mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); - mbedtls_ecp_keypair_free( their_key ); - mbedtls_free( their_key ); + mbedtls_ecp_keypair_free( &their_key ); return( status ); } #endif /* MBEDTLS_ECDH_C */ @@ -5570,10 +5609,16 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, case PSA_ALG_ECDH: if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); - return( psa_key_agreement_ecdh( peer_key, peer_key_length, - private_key->data.ecp, - shared_secret, shared_secret_size, - shared_secret_length ) ); + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); + if( status != PSA_SUCCESS ) + return status; + status = psa_key_agreement_ecdh( peer_key, peer_key_length, + &ecp, + shared_secret, shared_secret_size, + shared_secret_length ); + mbedtls_ecp_keypair_free( &ecp ); + return status; #endif /* MBEDTLS_ECDH_C */ default: (void) private_key; @@ -5860,7 +5905,7 @@ static psa_status_t psa_generate_key_internal( mbedtls_ecc_group_of_psa( curve, PSA_BITS_TO_BYTES( bits ) ); const mbedtls_ecp_curve_info *curve_info = mbedtls_ecp_curve_info_from_grp_id( grp_id ); - mbedtls_ecp_keypair *ecp; + mbedtls_ecp_keypair ecp; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( domain_parameters_size != 0 ) return( PSA_ERROR_NOT_SUPPORTED ); @@ -5868,20 +5913,36 @@ static psa_status_t psa_generate_key_internal( return( PSA_ERROR_NOT_SUPPORTED ); if( curve_info->bit_size != bits ) return( PSA_ERROR_INVALID_ARGUMENT ); - ecp = mbedtls_calloc( 1, sizeof( *ecp ) ); - if( ecp == NULL ) - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - mbedtls_ecp_keypair_init( ecp ); - ret = mbedtls_ecp_gen_key( grp_id, ecp, + mbedtls_ecp_keypair_init( &ecp ); + ret = mbedtls_ecp_gen_key( grp_id, &ecp, mbedtls_ctr_drbg_random, &global_data.ctr_drbg ); if( ret != 0 ) { - mbedtls_ecp_keypair_free( ecp ); - mbedtls_free( ecp ); + mbedtls_ecp_keypair_free( &ecp ); return( mbedtls_to_psa_error( ret ) ); } - slot->data.ecp = ecp; + + + /* Make sure to always have an export representation available */ + size_t bytes = PSA_BITS_TO_BYTES( bits ); + slot->data.key.data = mbedtls_calloc( 1, bytes ); + if( slot->data.key.data == NULL ) + { + mbedtls_ecp_keypair_free( &ecp ); + return( PSA_ERROR_INSUFFICIENT_MEMORY ); + } + slot->data.key.bytes = bytes; + psa_status_t status = mbedtls_to_psa_error( + mbedtls_ecp_write_key( &ecp, slot->data.key.data, bytes ) ); + + mbedtls_ecp_keypair_free( &ecp ); + if( status != PSA_SUCCESS ) + { + psa_remove_key_data_from_memory( slot ); + return( status ); + } + return( PSA_SUCCESS ); } else #endif /* MBEDTLS_ECP_C */ diff --git a/library/psa_crypto_core.h b/library/psa_crypto_core.h index c90d7375a..53fb61a71 100644 --- a/library/psa_crypto_core.h +++ b/library/psa_crypto_core.h @@ -32,8 +32,6 @@ #include "psa/crypto.h" #include "psa/crypto_se_driver.h" -#include "mbedtls/ecp.h" - /** The data structure representing a key slot, containing key material * and metadata for one key. */ @@ -49,10 +47,6 @@ typedef struct uint8_t *data; size_t bytes; } key; -#if defined(MBEDTLS_ECP_C) - /* EC public key or key pair */ - mbedtls_ecp_keypair *ecp; -#endif /* MBEDTLS_ECP_C */ #if defined(MBEDTLS_PSA_CRYPTO_SE_C) /* Any key type in a secure element */ struct se From 560c28a1acfd8bc83cc90cd69cc2f709575cfa11 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:20:24 +0200 Subject: [PATCH 28/55] Unify key handling logic Now that both ECP and RSA keys are represented in export representation, they can be treated more uniformly. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 184 ++++++++++++++----------------------------- 1 file changed, 60 insertions(+), 124 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ee616956a..de5e8588d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -905,24 +905,6 @@ static inline size_t psa_get_key_slot_bits( const psa_key_slot_t *slot ) return( slot->attr.bits ); } -/** Calculate the size of the key in the given slot, in bits. - * - * \param[in] slot A key slot containing a transparent key. - * - * \return The key size in bits, calculated from the key data. - */ -static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot ) -{ - size_t bits = 0; /* return 0 on an empty slot */ - - if( key_type_is_raw_bytes( slot->attr.type ) ) - bits = PSA_BYTES_TO_BITS( slot->data.key.bytes ); - - /* We know that the size fits in psa_key_bits_t thanks to checks - * when the key was created. */ - return( (psa_key_bits_t) bits ); -} - /** Import key data into a slot. `slot->attr.type` must have been set * previously. This function assumes that the slot does not contain * any key material yet. On failure, the slot content is unchanged. */ @@ -988,17 +970,6 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, return( PSA_ERROR_NOT_SUPPORTED ); } - if( status == PSA_SUCCESS ) - { - if( !PSA_KEY_TYPE_IS_RSA( slot->attr.type ) && - !PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - { - /* Write the actual key size to the slot. - * psa_start_key_creation() wrote the size declared by the - * caller, which may be 0 (meaning unspecified) or wrong. */ - slot->attr.bits = psa_calculate_key_bits( slot ); - } - } return( status ); } @@ -1166,29 +1137,15 @@ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) { /* No key material to clean. */ } - else if( key_type_is_raw_bytes( slot->attr.type ) ) - { - mbedtls_free( slot->data.key.data ); - } - else -#if defined(MBEDTLS_RSA_C) - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) + else if( key_type_is_raw_bytes( slot->attr.type ) || + PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || + PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { mbedtls_free( slot->data.key.data ); slot->data.key.data = NULL; slot->data.key.bytes = 0; } else -#endif /* defined(MBEDTLS_RSA_C) */ -#if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) - { - mbedtls_free( slot->data.key.data ); - slot->data.key.data = NULL; - slot->data.key.bytes = 0; - } - else -#endif /* defined(MBEDTLS_ECP_C) */ { /* Shouldn't happen: the key type is not any type that we * put in. */ @@ -1522,94 +1479,78 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( key_type_is_raw_bytes( slot->attr.type ) ) { - if( slot->data.key.bytes > data_size ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - memcpy( data, slot->data.key.data, slot->data.key.bytes ); - memset( data + slot->data.key.bytes, 0, - data_size - slot->data.key.bytes ); - *data_length = slot->data.key.bytes; - return( PSA_SUCCESS ); - } -#if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC_KEY_PAIR( slot->attr.type ) && !export_public_key ) - { - /* Exporting private -> private */ return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); } -#endif - else + else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || + PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) { - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) || - PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + { + /* Exporting public -> public */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + else if( !export_public_key ) + { + /* Exporting private -> private */ + return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); + } + /* Need to export the public part of a private key, + * so conversion is needed */ + if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - { #if defined(MBEDTLS_RSA_C) - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) - { - /* Exporting public -> public */ - return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); - } - else if( !export_public_key ) - { - /* Exporting private -> private */ - return( psa_internal_export_key_buffer( slot, data, data_size, data_length ) ); - } + mbedtls_rsa_context rsa; + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - /* Exporting private -> public */ - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + if( status != PSA_SUCCESS ) + return status; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); - if( status != PSA_SUCCESS ) - return status; + status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, + &rsa, + data, + data_size, + data_length ); - status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, - &rsa, - data, - data_size, - data_length ); + mbedtls_rsa_free( &rsa ); - mbedtls_rsa_free( &rsa ); - - return( status ); + return( status ); #else - /* We don't know how to convert a private RSA key to public. */ - return( PSA_ERROR_NOT_SUPPORTED ); + /* We don't know how to convert a private RSA key to public. */ + return( PSA_ERROR_NOT_SUPPORTED ); #endif - } - else - { -#if defined(MBEDTLS_ECP_C) - mbedtls_ecp_keypair ecp; - psa_status_t status = psa_load_ecp_representation( slot, &ecp ); - if( status != PSA_SUCCESS ) - return status; - - status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( - PSA_KEY_TYPE_ECC_GET_FAMILY( - slot->attr.type ) ), - &ecp, - data, - data_size, - data_length ); - - mbedtls_ecp_keypair_free( &ecp ); - return( status ); -#else - /* We don't know how to convert a private ECC key to public. */ - return( PSA_ERROR_NOT_SUPPORTED ); -#endif /* defined(MBEDTLS_ECP_C) */ - } } else { - /* This shouldn't happen in the reference implementation, but - it is valid for a special-purpose implementation to omit - support for exporting certain key types. */ +#if defined(MBEDTLS_ECP_C) + mbedtls_ecp_keypair ecp; + psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + if( status != PSA_SUCCESS ) + return status; + + status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( + PSA_KEY_TYPE_ECC_GET_FAMILY( + slot->attr.type ) ), + &ecp, + data, + data_size, + data_length ); + + mbedtls_ecp_keypair_free( &ecp ); + return( status ); +#else + /* We don't know how to convert a private ECC key to public */ return( PSA_ERROR_NOT_SUPPORTED ); +#endif } } + else + { + /* This shouldn't happen in the reference implementation, but + it is valid for a special-purpose implementation to omit + support for exporting certain key types. */ + return( PSA_ERROR_NOT_SUPPORTED ); + } } psa_status_t psa_export_key( psa_key_handle_t handle, @@ -5889,10 +5830,8 @@ static psa_status_t psa_generate_key_internal( &slot->data.key.bytes ); mbedtls_rsa_free( &rsa ); if( status != PSA_SUCCESS ) - { psa_remove_key_data_from_memory( slot ); - return( status ); - } + return( status ); } else #endif /* MBEDTLS_RSA_C && MBEDTLS_GENPRIME */ @@ -5938,11 +5877,8 @@ static psa_status_t psa_generate_key_internal( mbedtls_ecp_keypair_free( &ecp ); if( status != PSA_SUCCESS ) - { psa_remove_key_data_from_memory( slot ); - return( status ); - } - return( PSA_SUCCESS ); + return( status ); } else #endif /* MBEDTLS_ECP_C */ From 19fd574b3ac40819d8db6b0201e69f0f41661251 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Fri, 24 Jul 2020 23:31:01 +0200 Subject: [PATCH 29/55] Disconnect knowing about a PSA key type from knowing the mbedTLS API Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index de5e8588d..4a3877cc1 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -951,22 +951,31 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, * caller, which may be 0 (meaning unspecified) or wrong. */ slot->attr.bits = (psa_key_bits_t) bit_size; } - else + else if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + { #if defined(MBEDTLS_ECP_C) - if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) ) + status = psa_import_ecp_key( slot, + data, data_length ); +#else + /* No drivers have been implemented yet, so without mbed TLS backing + * there's no way to do ECP with the current library. */ + return( PSA_ERROR_NOT_SUPPORTED ); +#endif /* defined(MBEDTLS_ECP_C) */ + } + else if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - status = psa_import_ecp_key( slot, data, data_length ); +#if defined(MBEDTLS_RSA_C) + status = psa_import_rsa_key( slot, + data, data_length ); +#else + /* No drivers have been implemented yet, so without mbed TLS backing + * there's no way to do RSA with the current library. */ + status = PSA_ERROR_NOT_SUPPORTED; +#endif /* defined(MBEDTLS_RSA_C) */ } else -#endif /* MBEDTLS_ECP_C */ -#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) - if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) - { - status = psa_import_rsa_key( slot, data, data_length ); - } - else -#endif /* defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PK_PARSE_C) */ { + /* Unknown key type */ return( PSA_ERROR_NOT_SUPPORTED ); } From 75b743666eb13739720b5e36dcffeed6316ed22c Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 28 Jul 2020 14:30:13 +0200 Subject: [PATCH 30/55] Update after feedback on #3492 * Updated wording * Split out buffer allocation to a convenience function * Moved variable declarations to beginning of their code block Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 95 +++++++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 37 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4a3877cc1..34d48950f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -605,8 +605,8 @@ static psa_status_t psa_export_rsa_key( psa_key_type_t type, pk.pk_ctx = rsa; /* PSA Crypto API defines the format of an RSA key as a DER-encoded - * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey - * or the RFC3279 RSAPublicKey for a private key or a public key. */ + * representation of the non-encrypted PKCS#1 RSAPrivateKey for a + * private key and of the RFC3279 RSAPublicKey for a public key. */ if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) ret = mbedtls_pk_write_key_der( &pk, data, data_size ); else @@ -670,8 +670,10 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( mbedtls_rsa_get_len( &rsa ) ); - /* Re-export the data to PSA export format, which in case of RSA is the - * smallest representation we can parse. */ + /* Re-export the data to PSA export format, such that we can store export + * representation in the key slot. Export representation in case of RSA is + * the smallest representation that's allowed as input, so a straight-up + * allocation of the same size as the input buffer will be large enough. */ output = mbedtls_calloc( 1, data_length ); if( output == NULL ) @@ -680,11 +682,6 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, goto exit; } - /* PSA Crypto API defines the format of an RSA key as a DER-encoded - * representation of respectively the non-encrypted PKCS#1 RSAPrivateKey - * or the RFC3279 RSAPublicKey for a private key or a public key. That - * means we have no other choice then to run an import to verify the key - * size. */ status = psa_export_rsa_key( slot->attr.type, &rsa, output, @@ -905,6 +902,32 @@ static inline size_t psa_get_key_slot_bits( const psa_key_slot_t *slot ) return( slot->attr.bits ); } +/** Try to allocate a buffer to an empty key slot. + * + * \param[in,out] slot Key slot to attach buffer to. + * \param[in] buffer_length Requested size of the buffer. + * + * \retval #PSA_SUCCESS + * The buffer has been successfully allocated. + * \retval #PSA_ERROR_INSUFFICIENT_MEMORY + * Not enough memory was available for allocation. + * \retval #PSA_ERROR_ALREADY_EXISTS + * Trying to allocate a buffer to a non-empty key slot. + */ +static psa_status_t psa_allocate_buffer_to_slot( psa_key_slot_t *slot, + size_t buffer_length ) +{ + if( slot->data.key.data != NULL ) + return PSA_ERROR_ALREADY_EXISTS; + + slot->data.key.data = mbedtls_calloc( 1, buffer_length ); + if( slot->data.key.data == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; + + slot->data.key.bytes = buffer_length; + return PSA_SUCCESS; +} + /** Import key data into a slot. `slot->attr.type` must have been set * previously. This function assumes that the slot does not contain * any key material yet. On failure, the slot content is unchanged. */ @@ -918,14 +941,14 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, if( data_length == 0 ) return( PSA_ERROR_NOT_SUPPORTED ); - /* Ensure that the bytes-to-bit conversion never overflows. */ - if( data_length > SIZE_MAX / 8 ) - return( PSA_ERROR_NOT_SUPPORTED ); - if( key_type_is_raw_bytes( slot->attr.type ) ) { size_t bit_size = PSA_BYTES_TO_BITS( data_length ); + /* Ensure that the bytes-to-bits conversion hasn't overflown. */ + if( data_length > SIZE_MAX / 8 ) + return( PSA_ERROR_NOT_SUPPORTED ); + /* Enforce a size limit, and in particular ensure that the bit * size fits in its representation type. */ if( bit_size > PSA_MAX_KEY_BITS ) @@ -936,12 +959,9 @@ psa_status_t psa_import_key_into_slot( psa_key_slot_t *slot, return status; /* Allocate memory for the key */ - slot->data.key.data = mbedtls_calloc( 1, data_length ); - if( slot->data.key.data == NULL ) - { - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } - slot->data.key.bytes = data_length; + status = psa_allocate_buffer_to_slot( slot, data_length ); + if( status != PSA_SUCCESS ) + return status; /* copy key into allocated buffer */ memcpy(slot->data.key.data, data, data_length); @@ -1135,6 +1155,10 @@ static psa_status_t psa_get_transparent_key( psa_key_handle_t handle, /** Wipe key data from a slot. Preserve metadata such as the policy. */ static psa_status_t psa_remove_key_data_from_memory( psa_key_slot_t *slot ) { + /* Check whether key is already clean */ + if( slot->data.key.data == NULL ) + return PSA_SUCCESS; + #if defined(MBEDTLS_PSA_CRYPTO_SE_C) if( psa_key_slot_is_external( slot ) ) { @@ -1958,11 +1982,12 @@ static psa_status_t psa_validate_optional_attributes( { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_mpi actual, required; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - mbedtls_mpi actual, required; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); @@ -3808,11 +3833,11 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( output_size < mbedtls_rsa_get_len( &rsa ) ) { mbedtls_rsa_free( &rsa ); @@ -3898,11 +3923,11 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, { mbedtls_rsa_context rsa; mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; if( input_length != mbedtls_rsa_get_len( &rsa ) ) { @@ -5773,13 +5798,9 @@ static psa_status_t psa_generate_key_internal( return( status ); /* Allocate memory for the key */ - slot->data.key.bytes = PSA_BITS_TO_BYTES( bits ); - slot->data.key.data = mbedtls_calloc( 1, slot->data.key.bytes ); - if( slot->data.key.data == NULL ) - { - slot->data.key.bytes = 0; - return( PSA_ERROR_INSUFFICIENT_MEMORY ); - } + status = psa_allocate_buffer_to_slot( slot, PSA_BITS_TO_BYTES( bits ) ); + if( status != PSA_SUCCESS ) + return status; status = psa_generate_random( slot->data.key.data, slot->data.key.bytes ); @@ -5825,11 +5846,11 @@ static psa_status_t psa_generate_key_internal( /* Make sure to always have an export representation available */ size_t bytes = PSA_KEY_EXPORT_RSA_KEY_PAIR_MAX_SIZE( bits ); - slot->data.key.data = mbedtls_calloc( 1, bytes ); - if( slot->data.key.data == NULL ) + status = psa_allocate_buffer_to_slot( slot, bytes ); + if( status != PSA_SUCCESS ) { mbedtls_rsa_free( &rsa ); - return( PSA_ERROR_INSUFFICIENT_MEMORY ); + return status; } status = psa_export_rsa_key( type, @@ -5874,14 +5895,14 @@ static psa_status_t psa_generate_key_internal( /* Make sure to always have an export representation available */ size_t bytes = PSA_BITS_TO_BYTES( bits ); - slot->data.key.data = mbedtls_calloc( 1, bytes ); - if( slot->data.key.data == NULL ) + psa_status_t status = psa_allocate_buffer_to_slot( slot, bytes ); + if( status != PSA_SUCCESS ) { mbedtls_ecp_keypair_free( &ecp ); - return( PSA_ERROR_INSUFFICIENT_MEMORY ); + return status; } - slot->data.key.bytes = bytes; - psa_status_t status = mbedtls_to_psa_error( + + status = mbedtls_to_psa_error( mbedtls_ecp_write_key( &ecp, slot->data.key.data, bytes ) ); mbedtls_ecp_keypair_free( &ecp ); From a2371e53e4146828de28855929b144fe89d47ed1 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Tue, 28 Jul 2020 14:30:39 +0200 Subject: [PATCH 31/55] Update after feedback from #3492 * Allocate internal representation contexts on the heap (i.e. don't change where they're being allocated) * Unify load_xxx_representation in terms of allocation and init behaviour Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 190 +++++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 79 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 34d48950f..15a612b68 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -520,14 +520,14 @@ static psa_status_t psa_check_rsa_key_byte_aligned( /** Load the contents of a key slot into an internal RSA representation * - * \param[in] slot The slot from which to load the representation - * \param[out] rsa The internal RSA representation to hold the key. Must be - * allocated and initialized. If it already holds a - * different key, it will be overwritten and cause a memory - * leak. + * \param[in] slot The slot from which to load the representation + * \param[out] p_rsa Returns a pointer to an RSA context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. */ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, - mbedtls_rsa_context *rsa ) + mbedtls_rsa_context **p_rsa ) { #if defined(MBEDTLS_PK_PARSE_C) psa_status_t status; @@ -567,9 +567,10 @@ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; - /* Copy the PK-contained RSA context to the one provided as function input */ - status = mbedtls_to_psa_error( - mbedtls_rsa_copy( rsa, mbedtls_pk_rsa( ctx ) ) ); + /* Copy out the pointer to the RSA context, and reset the PK context + * such that pk_free doesn't free the RSA context we just grabbed. */ + *p_rsa = mbedtls_pk_rsa( ctx ); + ctx.pk_info = NULL; exit: mbedtls_pk_free( &ctx ); @@ -653,8 +654,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, { psa_status_t status; uint8_t* output = NULL; - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; /* Temporarily load input into slot. The cast here is safe since it'll * only be used for load_rsa_representation, which doesn't modify the @@ -668,7 +668,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, goto exit; slot->attr.bits = (psa_key_bits_t) PSA_BYTES_TO_BITS( - mbedtls_rsa_get_len( &rsa ) ); + mbedtls_rsa_get_len( rsa ) ); /* Re-export the data to PSA export format, such that we can store export * representation in the key slot. Export representation in case of RSA is @@ -683,14 +683,16 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, } status = psa_export_rsa_key( slot->attr.type, - &rsa, + rsa, output, data_length, &data_length); exit: /* Always free the RSA object */ - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + if( rsa != NULL ) + mbedtls_free( rsa ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -710,15 +712,24 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -/* Load the key slot contents into an mbedTLS internal representation object. - * Note: caller is responsible for freeing the object properly */ +/** Load the contents of a key slot into an internal ECP representation + * + * \param[in] slot The slot from which to load the representation + * \param[out] p_ecp Returns a pointer to an ECP context on success. + * The caller is responsible for freeing both the + * contents of the context and the context itself + * when done. + */ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, - mbedtls_ecp_keypair *ecp ) + mbedtls_ecp_keypair **p_ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; size_t data_length = slot->data.key.bytes; psa_status_t status; - mbedtls_ecp_keypair_init( ecp ); + mbedtls_ecp_keypair *ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); + + if( ecp == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { @@ -733,6 +744,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, data_length = slot->data.key.bytes / 2; } + mbedtls_ecp_keypair_init( ecp ); + /* Load the group. */ grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), data_length ); @@ -777,9 +790,15 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; } + + *p_ecp = ecp; exit: if( status != PSA_SUCCESS ) + { mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); + } + return status; } @@ -835,7 +854,7 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, { psa_status_t status; uint8_t* output = NULL; - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; /* Temporarily load input into slot. The cast here is safe since it'll * only be used for load_ecp_representation, which doesn't modify the @@ -849,9 +868,9 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, goto exit; if( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) == PSA_ECC_FAMILY_MONTGOMERY) - slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits + 1; + slot->attr.bits = (psa_key_bits_t) ecp->grp.nbits + 1; else - slot->attr.bits = (psa_key_bits_t) ecp.grp.nbits; + slot->attr.bits = (psa_key_bits_t) ecp->grp.nbits; /* Re-export the data to PSA export format. There is currently no support * for other input formats then the export format, so this is a 1-1 @@ -865,14 +884,16 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, } status = psa_export_ecp_key( slot->attr.type, - &ecp, + ecp, output, data_length, &data_length); exit: - /* Always free the PK object (will also free contained RSA context) */ - mbedtls_ecp_keypair_free( &ecp ); + /* Always free the PK object (will also free contained ECP context) */ + mbedtls_ecp_keypair_free( ecp ); + if( ecp != NULL ) + mbedtls_free( ecp ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -1419,16 +1440,16 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, break; #endif /* MBEDTLS_PSA_CRYPTO_SE_C */ { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) break; - status = psa_get_rsa_public_exponent( &rsa, + status = psa_get_rsa_public_exponent( rsa, attributes ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); } break; #endif /* MBEDTLS_RSA_C */ @@ -1532,20 +1553,19 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { #if defined(MBEDTLS_RSA_C) - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); - + mbedtls_rsa_context *rsa = NULL; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; status = psa_export_rsa_key( PSA_KEY_TYPE_RSA_PUBLIC_KEY, - &rsa, + rsa, data, data_size, data_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( status ); #else @@ -1556,7 +1576,7 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, else { #if defined(MBEDTLS_ECP_C) - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) return status; @@ -1564,12 +1584,13 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, status = psa_export_ecp_key( PSA_KEY_TYPE_ECC_PUBLIC_KEY( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ) ), - &ecp, + ecp, data, data_size, data_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return( status ); #else /* We don't know how to convert a private ECC key to public */ @@ -1980,8 +2001,7 @@ static psa_status_t psa_validate_optional_attributes( #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; mbedtls_mpi actual, required; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); @@ -1991,9 +2011,10 @@ static psa_status_t psa_validate_optional_attributes( int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); - ret = mbedtls_rsa_export( &rsa, + ret = mbedtls_rsa_export( rsa, NULL, NULL, NULL, NULL, &actual ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); if( ret != 0 ) goto rsa_exit; ret = mbedtls_mpi_read_binary( &required, @@ -3638,21 +3659,21 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) goto exit; - status = psa_rsa_sign( &rsa, + status = psa_rsa_sign( rsa, alg, hash, hash_length, signature, signature_size, signature_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); } else #endif /* defined(MBEDTLS_RSA_C) */ @@ -3668,16 +3689,17 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, #endif ) { - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) goto exit; - status = psa_ecdsa_sign( &ecp, + status = psa_ecdsa_sign( ecp, alg, hash, hash_length, signature, signature_size, signature_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); } else #endif /* defined(MBEDTLS_ECDSA_C) */ @@ -3741,18 +3763,18 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - status = psa_rsa_verify( &rsa, + status = psa_rsa_verify( rsa, alg, hash, hash_length, signature, signature_length ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( status ); } else @@ -3763,14 +3785,15 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, #if defined(MBEDTLS_ECDSA_C) if( PSA_ALG_IS_ECDSA( alg ) ) { - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; status = psa_load_ecp_representation( slot, &ecp ); if( status != PSA_SUCCESS ) return status; - status = psa_ecdsa_verify( &ecp, + status = psa_ecdsa_verify( ecp, hash, hash_length, signature, signature_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return status; } else @@ -3831,22 +3854,23 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( PSA_KEY_TYPE_IS_RSA( slot->attr.type ) ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa = NULL; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - if( output_size < mbedtls_rsa_get_len( &rsa ) ) + + if( output_size < mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_BUFFER_TOO_SMALL ); } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_encrypt( &rsa, + ret = mbedtls_rsa_pkcs1_encrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3859,8 +3883,8 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, &rsa ); - ret = mbedtls_rsa_rsaes_oaep_encrypt( &rsa, + psa_rsa_oaep_set_padding_mode( alg, rsa ); + ret = mbedtls_rsa_rsaes_oaep_encrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PUBLIC, @@ -3872,13 +3896,15 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } if( ret == 0 ) - *output_length = mbedtls_rsa_get_len( &rsa ); + *output_length = mbedtls_rsa_get_len( rsa ); - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -3921,24 +3947,24 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_RSA_C) if( slot->attr.type == PSA_KEY_TYPE_RSA_KEY_PAIR ) { - mbedtls_rsa_context rsa; - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, MBEDTLS_MD_NONE ); + mbedtls_rsa_context *rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - if( input_length != mbedtls_rsa_get_len( &rsa ) ) + if( input_length != mbedtls_rsa_get_len( rsa ) ) { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } #if defined(MBEDTLS_PKCS1_V15) if( alg == PSA_ALG_RSA_PKCS1V15_CRYPT ) { - ret = mbedtls_rsa_pkcs1_decrypt( &rsa, + ret = mbedtls_rsa_pkcs1_decrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3952,8 +3978,8 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, #if defined(MBEDTLS_PKCS1_V21) if( PSA_ALG_IS_RSA_OAEP( alg ) ) { - psa_rsa_oaep_set_padding_mode( alg, &rsa ); - ret = mbedtls_rsa_rsaes_oaep_decrypt( &rsa, + psa_rsa_oaep_set_padding_mode( alg, rsa ); + ret = mbedtls_rsa_rsaes_oaep_decrypt( rsa, mbedtls_ctr_drbg_random, &global_data.ctr_drbg, MBEDTLS_RSA_PRIVATE, @@ -3966,11 +3992,13 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, else #endif /* MBEDTLS_PKCS1_V21 */ { - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( PSA_ERROR_INVALID_ARGUMENT ); } - mbedtls_rsa_free( &rsa ); + mbedtls_rsa_free( rsa ); + mbedtls_free( rsa ); return( mbedtls_to_psa_error( ret ) ); } else @@ -5521,7 +5549,7 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t shared_secret_size, size_t *shared_secret_length ) { - mbedtls_ecp_keypair their_key; + mbedtls_ecp_keypair *their_key; psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; @@ -5540,7 +5568,7 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, goto exit; status = mbedtls_to_psa_error( - mbedtls_ecdh_get_params( &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) ); + mbedtls_ecdh_get_params( &ecdh, their_key, MBEDTLS_ECDH_THEIRS ) ); if( status != PSA_SUCCESS ) goto exit; status = mbedtls_to_psa_error( @@ -5563,7 +5591,10 @@ exit: if( status != PSA_SUCCESS ) mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); - mbedtls_ecp_keypair_free( &their_key ); + mbedtls_ecp_keypair_free( their_key ); + if( their_key != NULL) + mbedtls_free( their_key ); + return( status ); } #endif /* MBEDTLS_ECDH_C */ @@ -5584,15 +5615,16 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, case PSA_ALG_ECDH: if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); - mbedtls_ecp_keypair ecp; + mbedtls_ecp_keypair *ecp = NULL; psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_key_agreement_ecdh( peer_key, peer_key_length, - &ecp, + ecp, shared_secret, shared_secret_size, shared_secret_length ); - mbedtls_ecp_keypair_free( &ecp ); + mbedtls_ecp_keypair_free( ecp ); + mbedtls_free( ecp ); return status; #endif /* MBEDTLS_ECDH_C */ default: From 6d839f05bf1404c8d424bdf9a858e7d521f262f3 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 11:36:45 +0200 Subject: [PATCH 32/55] Cleanup * No null-check before calling free * Close memory leak * No need for double check of privkey validity Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 15a612b68..b6a34dfd3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -691,8 +691,7 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, exit: /* Always free the RSA object */ mbedtls_rsa_free( rsa ); - if( rsa != NULL ) - mbedtls_free( rsa ); + mbedtls_free( rsa ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -726,10 +725,7 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; size_t data_length = slot->data.key.bytes; psa_status_t status; - mbedtls_ecp_keypair *ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); - - if( ecp == NULL ) - return PSA_ERROR_INSUFFICIENT_MEMORY; + mbedtls_ecp_keypair *ecp = NULL; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { @@ -744,19 +740,27 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, data_length = slot->data.key.bytes / 2; } + /* Allocate and initialize a key representation. */ + ecp = mbedtls_calloc(1, sizeof(mbedtls_ecp_keypair)); + if( ecp == NULL ) + return PSA_ERROR_INSUFFICIENT_MEMORY; mbedtls_ecp_keypair_init( ecp ); /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type), + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), data_length ); if( grp_id == MBEDTLS_ECP_DP_NONE ) - return( PSA_ERROR_INVALID_ARGUMENT ); + { + status = PSA_ERROR_INVALID_ARGUMENT; + goto exit; + } + status = mbedtls_to_psa_error( mbedtls_ecp_group_load( &ecp->grp, grp_id ) ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; - /* Load the key material */ + /* Load the key material. */ if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) { /* Load the public value. */ @@ -775,7 +779,7 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, } else { - /* Load the secret value. */ + /* Load and validate the secret value. */ status = mbedtls_to_psa_error( mbedtls_ecp_read_key( ecp->grp.id, ecp, @@ -784,11 +788,6 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, if( status != PSA_SUCCESS ) goto exit; - /* Validate the private key. */ - status = mbedtls_to_psa_error( - mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) ); - if( status != PSA_SUCCESS ) - goto exit; } *p_ecp = ecp; @@ -892,8 +891,7 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, exit: /* Always free the PK object (will also free contained ECP context) */ mbedtls_ecp_keypair_free( ecp ); - if( ecp != NULL ) - mbedtls_free( ecp ); + mbedtls_free( ecp ); /* Free the allocated buffer only on error. */ if( status != PSA_SUCCESS ) @@ -2003,12 +2001,12 @@ static psa_status_t psa_validate_optional_attributes( { mbedtls_rsa_context *rsa = NULL; mbedtls_mpi actual, required; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; psa_status_t status = psa_load_rsa_representation( slot, &rsa ); if( status != PSA_SUCCESS ) return status; - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi_init( &actual ); mbedtls_mpi_init( &required ); ret = mbedtls_rsa_export( rsa, @@ -5592,8 +5590,7 @@ exit: mbedtls_platform_zeroize( shared_secret, shared_secret_size ); mbedtls_ecdh_free( &ecdh ); mbedtls_ecp_keypair_free( their_key ); - if( their_key != NULL) - mbedtls_free( their_key ); + mbedtls_free( their_key ); return( status ); } From 7f39187d6b6fcca0775e303d3509fff36a451c7e Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 14:57:44 +0200 Subject: [PATCH 33/55] Convert load_xxx_representation to take buffers instead of a whole slot Avoids stack-allocating a key slot during ECDH, and mock-attaching a key to a key slot during key import. Signed-off-by: Steven Cooreman --- library/psa_crypto.c | 144 +++++++++++++++++++++++++++---------------- 1 file changed, 90 insertions(+), 54 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b6a34dfd3..6e9d25961 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -518,15 +518,19 @@ static psa_status_t psa_check_rsa_key_byte_aligned( } #endif /* MBEDTLS_PK_PARSE_C */ -/** Load the contents of a key slot into an internal RSA representation +/** Load the contents of a key buffer into an internal RSA representation * - * \param[in] slot The slot from which to load the representation + * \param[in] buffer The buffer from which to load the representation. + * \param[in] size The size in bytes of \p buffer. + * \param[in] type The type of key contained in the \p buffer. * \param[out] p_rsa Returns a pointer to an RSA context on success. * The caller is responsible for freeing both the * contents of the context and the context itself * when done. */ -static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, +static psa_status_t psa_load_rsa_representation( const uint8_t *buffer, + size_t size, + psa_key_type_t type, mbedtls_rsa_context **p_rsa ) { #if defined(MBEDTLS_PK_PARSE_C) @@ -536,12 +540,12 @@ static psa_status_t psa_load_rsa_representation( const psa_key_slot_t *slot, mbedtls_pk_init( &ctx ); /* Parse the data. */ - if( PSA_KEY_TYPE_IS_KEY_PAIR( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_KEY_PAIR( type ) ) status = mbedtls_to_psa_error( - mbedtls_pk_parse_key( &ctx, slot->data.key.data, slot->data.key.bytes, NULL, 0 ) ); + mbedtls_pk_parse_key( &ctx, buffer, size, NULL, 0 ) ); else status = mbedtls_to_psa_error( - mbedtls_pk_parse_public_key( &ctx, slot->data.key.data, slot->data.key.bytes ) ); + mbedtls_pk_parse_public_key( &ctx, buffer, size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -576,7 +580,9 @@ exit: mbedtls_pk_free( &ctx ); return( status ); #else - (void) slot; + (void) buffer; + (void) size; + (void) type; (void) rsa; return( PSA_ERROR_NOT_SUPPORTED ); #endif /* MBEDTLS_PK_PARSE_C */ @@ -656,14 +662,11 @@ static psa_status_t psa_import_rsa_key( psa_key_slot_t *slot, uint8_t* output = NULL; mbedtls_rsa_context *rsa = NULL; - /* Temporarily load input into slot. The cast here is safe since it'll - * only be used for load_rsa_representation, which doesn't modify the - * buffer. */ - slot->data.key.data = (uint8_t *)data; - slot->data.key.bytes = data_length; - /* Parse input */ - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( data, + data_length, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -711,23 +714,27 @@ exit: #endif /* defined(MBEDTLS_RSA_C) */ #if defined(MBEDTLS_ECP_C) -/** Load the contents of a key slot into an internal ECP representation +/** Load the contents of a key buffer into an internal ECP representation * - * \param[in] slot The slot from which to load the representation + * \param[in] buffer The buffer from which to load the representation. + * \param[in] size The size in bytes of \p buffer. + * \param[in] type The type of key contained in the \p buffer. * \param[out] p_ecp Returns a pointer to an ECP context on success. * The caller is responsible for freeing both the * contents of the context and the context itself * when done. */ -static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, +static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, + size_t size, + psa_key_type_t type, mbedtls_ecp_keypair **p_ecp ) { mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; - size_t data_length = slot->data.key.bytes; psa_status_t status; mbedtls_ecp_keypair *ecp = NULL; + size_t curve_size = size; - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { /* A public key is represented as: * - The byte 0x04; @@ -735,9 +742,9 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. * So its data length is 2m+1 where n is the key size in bits. */ - if( ( slot->data.key.bytes & 1 ) == 0 ) + if( ( size & 1 ) == 0 ) return( PSA_ERROR_INVALID_ARGUMENT ); - data_length = slot->data.key.bytes / 2; + curve_size = size / 2; } /* Allocate and initialize a key representation. */ @@ -747,8 +754,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, mbedtls_ecp_keypair_init( ecp ); /* Load the group. */ - grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( slot->attr.type ), - data_length ); + grp_id = mbedtls_ecc_group_of_psa( PSA_KEY_TYPE_ECC_GET_FAMILY( type ), + curve_size ); if( grp_id == MBEDTLS_ECP_DP_NONE ) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -761,13 +768,13 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, goto exit; /* Load the key material. */ - if( PSA_KEY_TYPE_IS_PUBLIC_KEY( slot->attr.type ) ) + if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { /* Load the public value. */ status = mbedtls_to_psa_error( mbedtls_ecp_point_read_binary( &ecp->grp, &ecp->Q, - slot->data.key.data, - slot->data.key.bytes ) ); + buffer, + size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -783,8 +790,8 @@ static psa_status_t psa_load_ecp_representation( const psa_key_slot_t *slot, status = mbedtls_to_psa_error( mbedtls_ecp_read_key( ecp->grp.id, ecp, - slot->data.key.data, - slot->data.key.bytes ) ); + buffer, + size ) ); if( status != PSA_SUCCESS ) goto exit; @@ -855,14 +862,11 @@ static psa_status_t psa_import_ecp_key( psa_key_slot_t *slot, uint8_t* output = NULL; mbedtls_ecp_keypair *ecp = NULL; - /* Temporarily load input into slot. The cast here is safe since it'll - * only be used for load_ecp_representation, which doesn't modify the - * buffer. */ - slot->data.key.data = (uint8_t *)data; - slot->data.key.bytes = data_length; - /* Parse input */ - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( data, + data_length, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) goto exit; @@ -1440,7 +1444,10 @@ psa_status_t psa_get_key_attributes( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) break; @@ -1552,7 +1559,11 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, { #if defined(MBEDTLS_RSA_C) mbedtls_rsa_context *rsa = NULL; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + psa_status_t status = psa_load_rsa_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -1575,7 +1586,11 @@ static psa_status_t psa_internal_export_key( const psa_key_slot_t *slot, { #if defined(MBEDTLS_ECP_C) mbedtls_ecp_keypair *ecp = NULL; - psa_status_t status = psa_load_ecp_representation( slot, &ecp ); + psa_status_t status = psa_load_ecp_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; @@ -2003,7 +2018,11 @@ static psa_status_t psa_validate_optional_attributes( mbedtls_mpi actual, required; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - psa_status_t status = psa_load_rsa_representation( slot, &rsa ); + psa_status_t status = psa_load_rsa_representation( + slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3659,7 +3678,9 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, &rsa ); if( status != PSA_SUCCESS ) goto exit; @@ -3688,7 +3709,10 @@ psa_status_t psa_sign_hash( psa_key_handle_t handle, ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) goto exit; status = psa_ecdsa_sign( ecp, @@ -3763,7 +3787,10 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, { mbedtls_rsa_context *rsa = NULL; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3784,7 +3811,10 @@ psa_status_t psa_verify_hash( psa_key_handle_t handle, if( PSA_ALG_IS_ECDSA( alg ) ) { mbedtls_ecp_keypair *ecp = NULL; - status = psa_load_ecp_representation( slot, &ecp ); + status = psa_load_ecp_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_ecdsa_verify( ecp, @@ -3855,7 +3885,10 @@ psa_status_t psa_asymmetric_encrypt( psa_key_handle_t handle, mbedtls_rsa_context *rsa = NULL; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -3948,7 +3981,10 @@ psa_status_t psa_asymmetric_decrypt( psa_key_handle_t handle, mbedtls_rsa_context *rsa; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - status = psa_load_rsa_representation( slot, &rsa ); + status = psa_load_rsa_representation( slot->data.key.data, + slot->data.key.bytes, + slot->attr.type, + &rsa ); if( status != PSA_SUCCESS ) return status; @@ -5548,20 +5584,16 @@ static psa_status_t psa_key_agreement_ecdh( const uint8_t *peer_key, size_t *shared_secret_length ) { mbedtls_ecp_keypair *their_key; - psa_key_slot_t their_key_slot; mbedtls_ecdh_context ecdh; psa_status_t status; size_t bits = 0; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( our_key->grp.id, &bits ); mbedtls_ecdh_init( &ecdh ); - memset(&their_key_slot, 0, sizeof(their_key_slot)); - /* Creating ephemeral key slot for import purposes */ - their_key_slot.attr.type = PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve); - their_key_slot.data.key.data = (uint8_t*) peer_key; - their_key_slot.data.key.bytes = peer_key_length; - - status = psa_load_ecp_representation( &their_key_slot, &their_key ); + status = psa_load_ecp_representation( peer_key, + peer_key_length, + PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve), + &their_key ); if( status != PSA_SUCCESS ) goto exit; @@ -5613,7 +5645,11 @@ static psa_status_t psa_key_agreement_raw_internal( psa_algorithm_t alg, if( ! PSA_KEY_TYPE_IS_ECC_KEY_PAIR( private_key->attr.type ) ) return( PSA_ERROR_INVALID_ARGUMENT ); mbedtls_ecp_keypair *ecp = NULL; - psa_status_t status = psa_load_ecp_representation( private_key, &ecp ); + psa_status_t status = psa_load_ecp_representation( + private_key->data.key.data, + private_key->data.key.bytes, + private_key->attr.type, + &ecp ); if( status != PSA_SUCCESS ) return status; status = psa_key_agreement_ecdh( peer_key, peer_key_length, From 3fa684ed918e29bc4083553d3369218c87b9a636 Mon Sep 17 00:00:00 2001 From: Steven Cooreman Date: Thu, 30 Jul 2020 15:04:07 +0200 Subject: [PATCH 34/55] Allow importing Montgomery public keys in PSA Crypto PSA Crypto was checking the byte length of a to-be-imported public ECP key against the expected length for Weierstrass keys, forgetting that Curve25519/Curve448 exists. Signed-off-by: Steven Cooreman --- .../psa_curve25519_public_key_import.txt | 3 ++ library/psa_crypto.c | 35 +++++++++++++------ tests/suites/test_suite_psa_crypto.data | 4 +++ tests/suites/test_suite_psa_crypto.function | 25 ++++++++----- 4 files changed, 49 insertions(+), 18 deletions(-) create mode 100644 ChangeLog.d/psa_curve25519_public_key_import.txt diff --git a/ChangeLog.d/psa_curve25519_public_key_import.txt b/ChangeLog.d/psa_curve25519_public_key_import.txt new file mode 100644 index 000000000..2ea11e2c8 --- /dev/null +++ b/ChangeLog.d/psa_curve25519_public_key_import.txt @@ -0,0 +1,3 @@ +Bugfix + * PSA key import will now correctly import a Curve25519/Curve448 public key + instead of erroring out. Contributed by Steven Cooreman in #3492. diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 6e9d25961..056e67796 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -732,19 +732,34 @@ static psa_status_t psa_load_ecp_representation( const uint8_t *buffer, mbedtls_ecp_group_id grp_id = MBEDTLS_ECP_DP_NONE; psa_status_t status; mbedtls_ecp_keypair *ecp = NULL; - size_t curve_size = size; + size_t curve_size; if( PSA_KEY_TYPE_IS_PUBLIC_KEY( type ) ) { - /* A public key is represented as: - * - The byte 0x04; - * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; - * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. - * So its data length is 2m+1 where n is the key size in bits. - */ - if( ( size & 1 ) == 0 ) - return( PSA_ERROR_INVALID_ARGUMENT ); - curve_size = size / 2; + if( PSA_KEY_TYPE_ECC_GET_FAMILY( type ) == PSA_ECC_FAMILY_MONTGOMERY ) + { + /* A Montgomery public key is represented as its raw + * compressed public point. + */ + curve_size = size; + } + else + { + /* A Weierstrass public key is represented as: + * - The byte 0x04; + * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; + * - `y_P` as a `ceiling(m/8)`-byte string, big-endian. + * So its data length is 2m+1 where n is the key size in bits. + */ + if( ( size & 1 ) == 0 ) + return( PSA_ERROR_INVALID_ARGUMENT ); + curve_size = size / 2; + } + } + else + { + /* Private keys are represented as the raw private value */ + curve_size = size; } /* Allocate and initialize a key representation. */ diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 6a2859124..d1855fdb5 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -252,6 +252,10 @@ PSA import/export EC brainpoolP256r1 public key: good depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_BP256R1_ENABLED import_export:"04768c8cae4abca6306db0ed81b0c4a6215c378066ec6d616c146e13f1c7df809b96ab6911c27d8a02339f0926840e55236d3d1efbe2669d090e4c4c660fada91d":PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_BRAINPOOL_P_R1):PSA_KEY_USAGE_EXPORT:PSA_ALG_ECDSA_ANY:256:0:PSA_SUCCESS:1 +PSA import/export curve25519 public key: good +depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_CURVE25519_ENABLED +import_export:"8520f0098930a754748b7ddcb43ef75a0dbf3a0d26381af4eba4a98eaa9b4e6a":PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_MONTGOMERY):PSA_KEY_USAGE_EXPORT:PSA_ALG_ECDH:255:0:PSA_SUCCESS:1 + PSA import/export AES key: policy forbids export depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CTR import_export:"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa":PSA_KEY_TYPE_AES:PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT:PSA_ALG_CTR:128:0:PSA_ERROR_NOT_PERMITTED:1 diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 4576b8bb4..f4b9a8f67 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -961,14 +961,23 @@ static int exported_key_sanity_check( psa_key_type_t type, size_t bits, #if defined(MBEDTLS_ECP_C) if( PSA_KEY_TYPE_IS_ECC_PUBLIC_KEY( type ) ) { - /* The representation of an ECC public key is: - * - The byte 0x04; - * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; - * - `y_P` as a `ceiling(m/8)`-byte string, big-endian; - * - where m is the bit size associated with the curve. - */ - TEST_EQUAL( p + 1 + 2 * PSA_BITS_TO_BYTES( bits ), end ); - TEST_EQUAL( p[0], 4 ); + if( PSA_KEY_TYPE_ECC_GET_FAMILY( type ) == PSA_ECC_FAMILY_MONTGOMERY ) + { + /* The representation of an ECC Montgomery public key is + * the raw compressed point */ + TEST_EQUAL( p + PSA_BITS_TO_BYTES( bits ), end ); + } + else + { + /* The representation of an ECC Weierstrass public key is: + * - The byte 0x04; + * - `x_P` as a `ceiling(m/8)`-byte string, big-endian; + * - `y_P` as a `ceiling(m/8)`-byte string, big-endian; + * - where m is the bit size associated with the curve. + */ + TEST_EQUAL( p + 1 + 2 * PSA_BITS_TO_BYTES( bits ), end ); + TEST_EQUAL( p[0], 4 ); + } } else #endif /* MBEDTLS_ECP_C */ From b6c43f61a42eca15a2f7ae6de2486ec94b2ded40 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Aug 2020 10:55:16 +0200 Subject: [PATCH 35/55] Call driver entry point functions "entry point" Call the functions listed in the driver description "entry points". It's more precise than "functions", which could also mean any C function defined in the driver code. Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 86 +++++++++++++-------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index cabb81b07..79cc4d0d1 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -129,30 +129,30 @@ PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_CURVE_SECP_R1) PSA_KEY_TYPE_ECC_KEY_PAIR(_) ``` -### Driver functions +### Driver entry points -#### Overview of driver functions +#### Overview of driver entry points -Drivers define functions, each of which implements an aspect of a capability of a driver, such as a cryptographic operation, a part of a cryptographic operation, or a key management action. Most driver functions correspond to a particular function in the PSA Cryptography API. For example, if a call to `psa_sign_hash()` is dispatched to a driver, it invokes the driver's `sign_hash` function. +Drivers define functions, each of which implements an aspect of a capability of a driver, such as a cryptographic operation, a part of a cryptographic operation, or a key management action. These functions are called the **entry points** of the driver. Most driver entry points correspond to a particular function in the PSA Cryptography API. For example, if a call to `psa_sign_hash()` is dispatched to a driver, it invokes the driver's `sign_hash` function. -All driver functions return a status of type `psa_status_t` which should use the status codes documented for PSA services in general and for PSA Crypto in particular: `PSA_SUCCESS` indicates that the function succeeded, and `PSA_ERROR_xxx` values indicate that an error occurred. +All driver entry points return a status of type `psa_status_t` which should use the status codes documented for PSA services in general and for PSA Crypto in particular: `PSA_SUCCESS` indicates that the function succeeded, and `PSA_ERROR_xxx` values indicate that an error occurred. -The signature of a driver function generally looks like the signature of the PSA Crypto API that it implements, with some modifications. This section gives an overview of modifications that apply to whole classes of functions. Refer to the reference section for each function or function family for details. +The signature of a driver entry point generally looks like the signature of the PSA Crypto API that it implements, with some modifications. This section gives an overview of modifications that apply to whole classes of entry points. Refer to the reference section for each entry point or entry point family for details. -* For functions that operate on an existing key, the `psa_key_id_t` parameter is replaced by a sequence of three parameters that describe the key: +* For entry points that operate on an existing key, the `psa_key_id_t` parameter is replaced by a sequence of three parameters that describe the key: 1. `const psa_key_attributes_t *attributes`: the key attributes. 2. `const uint8_t *key_buffer`: a key material or key context buffer. 3. `size_t key_buffer_size`: the size of the key buffer in bytes. For transparent drivers, the key buffer contains the key material, in the same format as defined for `psa_export_key()` and `psa_export_public_key()` in the PSA Cryptography API. For opaque drivers, the content of the key buffer is entirely up to the driver. -* For functions that involve a multi-part operation, the operation state type (`psa_XXX_operation_t`) is replaced by a driver-specific operation state type (*prefix*`_XXX_operation_t`). +* For entry points that involve a multi-part operation, the operation state type (`psa_XXX_operation_t`) is replaced by a driver-specific operation state type (*prefix*`_XXX_operation_t`). -Some functions are grouped in families that must be implemented as a whole. If a driver supports a function family, it must provide all the functions in the family. +Some entry points are grouped in families that must be implemented as a whole. If a driver supports a entry point family, it must provide all the entry points in the family. -#### General considerations on driver function parameters +#### General considerations on driver entry point parameters -Buffer parameters for driver functions obey the following conventions: +Buffer parameters for driver entry points obey the following conventions: * An input buffer has the type `const uint8_t *` and is immediately followed by a parameter of type `size_t` that indicates the buffer size. * An output buffer has the type `uint8_t *` and is immediately followed by a parameter of type `size_t` that indicates the buffer size. A third parameter of type `size_t *` is provided to report the actual buffer size if the function succeeds. @@ -162,40 +162,40 @@ Buffers of size 0 may be represented with either a null pointer or a non-null po Input buffers and other input-only parameters (`const` pointers) may be in read-only memory. Overlap is possible between input buffers, and between an input buffer and an output buffer, but not between two output buffers or between a non-buffer parameter and another parameter. -#### Driver functions for single-part cryptographic operations +#### Driver entry points for single-part cryptographic operations -The following driver functions perform a cryptographic operation in one shot (single-part operation): +The following driver entry points perform a cryptographic operation in one shot (single-part operation): -* `"hash_compute"` (transparent drivers only): calculation of a hash. Called by `psa_hash_compute()` and `psa_hash_compare()`. To verify a hash with `psa_hash_compare()`, the core calls the driver's `"hash_compute"` function and compares the result with the reference hash value. -* `"mac_compute"`: calculation of a MAC. Called by `psa_mac_compute()` and possibly `psa_mac_verify()`. To verify a mac with `psa_mac_verify()`, the core calls an applicable driver's `"mac_verify"` function if there is one, otherwise the core calls an applicable driver's `"mac_compute"` function and compares the result with the reference MAC value. -* `"mac_verify"`: verification of a MAC. Called by `psa_mac_verify()`. This function is mainly useful for drivers of secure elements that verify a MAC without revealing the correct MAC. Although transparent drivers may implement this function in addition to `"mac_compute"`, it is generally not useful because the core can call the `"mac_compute"` function and compare with the expected MAC value. +* `"hash_compute"` (transparent drivers only): calculation of a hash. Called by `psa_hash_compute()` and `psa_hash_compare()`. To verify a hash with `psa_hash_compare()`, the core calls the driver's `"hash_compute"` entry point and compares the result with the reference hash value. +* `"mac_compute"`: calculation of a MAC. Called by `psa_mac_compute()` and possibly `psa_mac_verify()`. To verify a mac with `psa_mac_verify()`, the core calls an applicable driver's `"mac_verify"` entry point if there is one, otherwise the core calls an applicable driver's `"mac_compute"` entry point and compares the result with the reference MAC value. +* `"mac_verify"`: verification of a MAC. Called by `psa_mac_verify()`. This entry point is mainly useful for drivers of secure elements that verify a MAC without revealing the correct MAC. Although transparent drivers may implement this entry point in addition to `"mac_compute"`, it is generally not useful because the core can call the `"mac_compute"` entry point and compare with the expected MAC value. * `"cipher_encrypt"`: unauthenticated symmetric cipher encryption. Called by `psa_cipher_encrypt()`. * `"cipher_decrypt"`: unauthenticated symmetric cipher decryption. Called by `psa_cipher_decrypt()`. * `"aead_encrypt"`: authenticated encryption with associated data. Called by `psa_aead_encrypt()`. * `"aead_decrypt"`: authenticated decryption with associated data. Called by `psa_aead_decrypt()`. * `"asymmetric_encrypt"`: asymmetric encryption. Called by `psa_asymmetric_encrypt()`. * `"asymmetric_decrypt"`: asymmetric decryption. Called by `psa_asymmetric_decrypt()`. -* `"sign_hash"`: signature of an already calculated hash. Called by `psa_sign_hash()` and possibly `psa_sign_message()`. To sign a message with `psa_sign_message()`, the core calls an applicable driver's `"sign_message"` function if there is one, otherwise the core calls an applicable driver's `"hash_compute"` function followed by an applicable driver's `"sign_hash"` function. -* `"verify_hash"`: verification of an already calculated hash. Called by `psa_verify_hash()` and possibly `psa_verify_message()`. To verify a message with `psa_verify_message()`, the core calls an applicable driver's `"verify_message"` function if there is one, otherwise the core calls an applicable driver's `"hash_compute"` function followed by an applicable driver's `"verify_hash"` function. +* `"sign_hash"`: signature of an already calculated hash. Called by `psa_sign_hash()` and possibly `psa_sign_message()`. To sign a message with `psa_sign_message()`, the core calls an applicable driver's `"sign_message"` entry point if there is one, otherwise the core calls an applicable driver's `"hash_compute"` entry point followed by an applicable driver's `"sign_hash"` entry point. +* `"verify_hash"`: verification of an already calculated hash. Called by `psa_verify_hash()` and possibly `psa_verify_message()`. To verify a message with `psa_verify_message()`, the core calls an applicable driver's `"verify_message"` entry point if there is one, otherwise the core calls an applicable driver's `"hash_compute"` entry point followed by an applicable driver's `"verify_hash"` entry point. * `"sign_message"`: signature of a message. Called by `psa_sign_message()`. * `"verify_message"`: verification of a message. Called by `psa_verify_message()`. * `"key_agreement"`: key agreement without a subsequent key derivation. Called by `psa_raw_key_agreement()` and possibly `psa_key_derivation_key_agreement()`. -### Driver functions for multi-part operations +### Driver entry points for multi-part operations #### General considerations on multi-part operations -The functions that implement each step of a multi-part operation are grouped into a family. A driver that implements a multi-part operation must define all of the functions in this family as well as a type that represents the operation context. The lifecycle of a driver operation context is similar to the lifecycle of an API operation context: +The entry points that implement each step of a multi-part operation are grouped into a family. A driver that implements a multi-part operation must define all of the entry points in this family as well as a type that represents the operation context. The lifecycle of a driver operation context is similar to the lifecycle of an API operation context: 1. The core initializes operation context objects to either all-bits-zero or to logical zero (`{0}`), at its discretion. -1. The core calls the `xxx_setup` function for this operation family. If this fails, the core destroys the operation context object without calling any other driver function on it. -1. The core calls other functions that manipulate the operation context object, respecting the constraints. -1. If any function fails, the core calls the driver's `xxx_abort` function for this operation family, then destroys the operation context object without calling any other driver function on it. -1. If a “finish” function fails, the core destroys the operation context object without calling any other driver function on it. The finish functions are: *prefix*`_mac_sign_finish`, *prefix*`_mac_verify_finish`, *prefix*`_cipher_fnish`, *prefix*`_aead_finish`, *prefix*`_aead_verify`. +1. The core calls the `xxx_setup` entry point for this operation family. If this fails, the core destroys the operation context object without calling any other driver entry point on it. +1. The core calls other entry points that manipulate the operation context object, respecting the constraints. +1. If any entry point fails, the core calls the driver's `xxx_abort` entry point for this operation family, then destroys the operation context object without calling any other driver entry point on it. +1. If a “finish” entry point fails, the core destroys the operation context object without calling any other driver entry point on it. The finish entry points are: *prefix*`_mac_sign_finish`, *prefix*`_mac_verify_finish`, *prefix*`_cipher_fnish`, *prefix*`_aead_finish`, *prefix*`_aead_verify`. -If a driver implements a multi-part operation but not the corresponding single-part operation, the core calls the driver's multipart operation functions to perform the single-part operation. +If a driver implements a multi-part operation but not the corresponding single-part operation, the core calls the driver's multipart operation entry points to perform the single-part operation. -#### Multi-part operation function family `"hash_multipart"` +#### Multi-part operation entry point family `"hash_multipart"` This family corresponds to the calculation of a hash in one multiple parts. @@ -209,9 +209,9 @@ This family requires the following type and functions: * `"hash_finish"`: called by `psa_hash_finish()` and `psa_hash_verify()`. * `"hash_abort"`: called by all multi-part hash functions. -To verify a hash with `psa_hash_verify()`, the core calls the driver's *prefix`_hash_finish` function and compares the result with the reference hsah value. +To verify a hash with `psa_hash_verify()`, the core calls the driver's *prefix`_hash_finish` entry point and compares the result with the reference hsah value. -For example, a driver with the prefix `"acme"` that implements the `"hash_multipart"` function family must define the following type and functions (assuming that the capability does not use the `"names"` property to declare different type and function names): +For example, a driver with the prefix `"acme"` that implements the `"hash_multipart"` entry point family must define the following type and entry points (assuming that the capability does not use the `"names"` property to declare different type and entry point names): ``` typedef ... acme_hash_operation_t; @@ -253,12 +253,12 @@ TODO #### Operation family `"key_derivation"` -This family requires the following type and functions: +This family requires the following type and entry points: * Type `"key_derivation_operation_t"`: the type of a key derivation operation context. * `"key_derivation_setup"`: called by `psa_key_derivation_setup()`. * `"key_derivation_set_capacity"`: called by `psa_key_derivation_set_capacity()`. The core will always enforce the capacity, therefore this function does not need to do anything for algorithms where the output stream only depends on the effective generated length and not on the capacity. -* `"key_derivation_input_bytes"`: called by `psa_key_derivation_input_bytes()` and `psa_key_derivation_input_key()`. For transparent drivers, when processing a call to `psa_key_derivation_input_key()`, the core always calls the applicable driver's `"key_derivation_input_bytes"` function. +* `"key_derivation_input_bytes"`: called by `psa_key_derivation_input_bytes()` and `psa_key_derivation_input_key()`. For transparent drivers, when processing a call to `psa_key_derivation_input_key()`, the core always calls the applicable driver's `"key_derivation_input_bytes"` entry point. * `"key_derivation_input_key"` (opaque drivers only) * `"key_derivation_output_bytes"`: called by `psa_key_derivation_output_bytes()`; also by `psa_key_derivation_output_key()` for transparent drivers. * `"key_derivation_abort"`: called by all key derivation functions. @@ -267,17 +267,17 @@ TODO: key input and output for opaque drivers; deterministic key generation for TODO -### Driver functions for key management +### Driver entry points for key management -The driver functions for key management differs significantly between [transparent drivers](#key-management-with-transparent-drivers) and [opaque drivers](#key-management-with-transparent-drivers). Refer to the applicable section for each driver type. +The driver entry points for key management differs significantly between [transparent drivers](#key-management-with-transparent-drivers) and [opaque drivers](#key-management-with-transparent-drivers). Refer to the applicable section for each driver type. -### Miscellaneous driver functions +### Miscellaneous driver entry points #### Driver initialization -A driver may declare an `"init"` function in a capability with no algorithm, key type or key size. If so, the driver calls this function once during the initialization of the PSA Crypto subsystem. If the init function of any driver fails, the initialization of the PSA Crypto subsystem fails. +A driver may declare an `"init"` entry point in a capability with no algorithm, key type or key size. If so, the driver calls this entry point once during the initialization of the PSA Crypto subsystem. If the init entry point of any driver fails, the initialization of the PSA Crypto subsystem fails. -When multiple drivers have an init function, the order in which they are called is unspecified. It is also unspecified whether other drivers' init functions are called if one or more init function fails. +When multiple drivers have an init entry point, the order in which they are called is unspecified. It is also unspecified whether other drivers' init functions are called if one or more init function fails. On platforms where the PSA Crypto implementation is a subsystem of a single application, the initialization of the PSA Crypto subsystem takes place during the call to `psa_crypto_init()`. On platforms where the PSA Crypto implementation is separate from the application or applications, the initialization the initialization of the PSA Crypto subsystem takes place before or during the first time an application calls `psa_crypto_init()`. @@ -292,7 +292,7 @@ For an opaque driver, if the init function succeeds, the core saves the updated ### Combining multiple drivers -To declare a cryproprocessor can handle both cleartext and plaintext keys, you need to provide two driver descriptions, one for a transparent driver and one for an opaque driver. You can use the mapping in capabilities' `"names"` property to arrange for driver functions to map to the same C function. +To declare a cryproprocessor can handle both cleartext and plaintext keys, you need to provide two driver descriptions, one for a transparent driver and one for an opaque driver. You can use the mapping in capabilities' `"names"` property to arrange for driver entry points to map to the same C function. ## Transparent drivers @@ -302,7 +302,7 @@ The format of a key for transparent drivers is the same as in applications. Refe ### Key management with transparent drivers -Transparent drivers may provide the following key management functions: +Transparent drivers may provide the following key management entry points: * `"generate_key"`: called by `psa_generate_key()`, only when generating a key pair (key such that `PSA_KEY_TYPE_IS_ASYMMETRIC` is true). * `"derive_key"`: called by `psa_key_derivation_output_key()`, only when deriving a key pair (key such that `PSA_KEY_TYPE_IS_ASYMMETRIC` is true). @@ -312,11 +312,11 @@ Transparent drivers are not involved when importing, exporting, copying or destr ### Fallback -If a transparent driver function is part of a capability which has a true `"fallback"` property and returns `PSA_ERROR_NOT_SUPPORTED`, the built-in software implementation will be called instead. Any other value (`PSA_SUCCESS` or a different error code) is returned to the application. +If a transparent driver entry point is part of a capability which has a true `"fallback"` property and returns `PSA_ERROR_NOT_SUPPORTED`, the built-in software implementation will be called instead. Any other value (`PSA_SUCCESS` or a different error code) is returned to the application. If there are multiple available transparent drivers, the core tries them in turn until one is declared without a true `"fallback"` property or returns a status other than `PSA_ERROR_NOT_SUPPORTED`. -If a transparent driver function is part of a capability where the `"fallback"` property is false or omitted, the core should not include any other code for this capability, whether built in or in another transparent driver. +If a transparent driver entry point is part of a capability where the `"fallback"` property is false or omitted, the core should not include any other code for this capability, whether built in or in another transparent driver. ## Opaque drivers @@ -378,7 +378,7 @@ If the core does not support dynamic allocation for the key context or chooses n If the key is stored in the secure element and the driver only needs to store a label for the key, use `"base_size"` as the size of the label plus any other metadata that the driver needs to store, and omit the other properties. -If the key is stored in the secure element, but the secure element does not store the public part of a key pair and cannot recompute it on demand, additionally use the `"store_public_key"` property with the value `true`. Note that this only influences the size of the key context: the driver code must copy the public key to the key context and retrieve it on demand in its `export_public_key` function. +If the key is stored in the secure element, but the secure element does not store the public part of a key pair and cannot recompute it on demand, additionally use the `"store_public_key"` property with the value `true`. Note that this only influences the size of the key context: the driver code must copy the public key to the key context and retrieve it on demand in its `export_public_key` entry point. #### Key context size for a secure element without storage @@ -386,12 +386,12 @@ If the key is stored in wrapped form outside the secure element, and the wrapped ### Key management with opaque drivers -Transparent drivers may provide the following key management functions: +Transparent drivers may provide the following key management entry points: * `"allocate_key"`: called by `psa_import_key()`, `psa_generate_key()`, `psa_key_derivation_output_key()` or `psa_copy_key()` before creating a key in the location of this driver. * `"import_key"`: called by `psa_import_key()`, or by `psa_copy_key()` when copying a key from another location. * `"export_key"`: called by `psa_export_key()`, or by `psa_copy_key()` when copying a key from to location. -* `"export_public_key"`: called by the core to obtain the public key of a key pair. The core may call this function at any time to obtain the public key, which can be for `psa_export_public_key()` but also at other times, including during a cryptographic operation that requires the public key such as a call to `psa_verify_message()` on a key pair object. +* `"export_public_key"`: called by the core to obtain the public key of a key pair. The core may call this entry point at any time to obtain the public key, which can be for `psa_export_public_key()` but also at other times, including during a cryptographic operation that requires the public key such as a call to `psa_verify_message()` on a key pair object. * `"copy_key"`: called by `psa_copy_key()` when copying a key within the same location. * `"destroy_key"`: called by `psa_destroy_key()`. * `"generate_key"`: called by `psa_generate_key()`. @@ -399,9 +399,9 @@ Transparent drivers may provide the following key management functions: #### Key creation in a secure element without storage -This section describes the key creation process for secure elements that do not store the key material. The driver must obtain a wrapped form of the key material which the core will store. A driver for such a secure element has no `"allocate_key"` function. +This section describes the key creation process for secure elements that do not store the key material. The driver must obtain a wrapped form of the key material which the core will store. A driver for such a secure element has no `"allocate_key"` entry point. -When creating a key with an opaque driver which does not have an `"allocate_key"` function: +When creating a key with an opaque driver which does not have an `"allocate_key"` entry point: 1. The core allocates memory for the key context. 2. The core calls the driver's import, generate, derive or copy function. From 921492625cb32b669fd2b646fc5d24d065d87d9e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Aug 2020 11:35:49 +0200 Subject: [PATCH 36/55] Fix typos and copypasta Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index 79cc4d0d1..07e83a826 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -75,7 +75,7 @@ A driver description is a JSON object containing the following properties: A list of **capabilities**. Each capability describes a family of functions that the driver implements for a certain class of cryptographic mechanisms. * `"key_context"` (not permitted for transparent drivers, mandatory for opaque drivers): information about the [representation of keys](#key-format-for-opaque-drivers). * `"persistent_state_size"` (not permitted for transparent drivers, optional for opaque drivers, integer or string). The size in bytes of the [persistent state of the driver](#opaque-driver-persistent-state). This may be either a non-negative integer or a C constant expression of type `size_t`. -* `"location"` (not permitted for transparent drivers, optional for opaque drivers, integer or string). The location value for which this driver is invoked. This may be either a non-negative integer or a C constant expression of type `psa_key_location_t`. +* `"location"` (not permitted for transparent drivers, optional for opaque drivers, integer or string). The location value for which this driver is invoked. In other words, this determines the lifetimes for which the driver is invoked. This may be either a non-negative integer or a C constant expression of type `psa_key_location_t`. #### Driver description capability @@ -197,7 +197,7 @@ If a driver implements a multi-part operation but not the corresponding single-p #### Multi-part operation entry point family `"hash_multipart"` -This family corresponds to the calculation of a hash in one multiple parts. +This family corresponds to the calculation of a hash in multiple steps. This family applies to transparent drivers only. @@ -292,7 +292,7 @@ For an opaque driver, if the init function succeeds, the core saves the updated ### Combining multiple drivers -To declare a cryproprocessor can handle both cleartext and plaintext keys, you need to provide two driver descriptions, one for a transparent driver and one for an opaque driver. You can use the mapping in capabilities' `"names"` property to arrange for driver entry points to map to the same C function. +To declare a cryptoprocessor can handle both cleartext and plaintext keys, you need to provide two driver descriptions, one for a transparent driver and one for an opaque driver. You can use the mapping in capabilities' `"names"` property to arrange for multiple driver entry points to map to the same C function. ## Transparent drivers @@ -411,7 +411,7 @@ When creating a key with an opaque driver which does not have an `"allocate_key" This section describes the key creation process for secure elements that have persistent storage for the key material. A driver for such a secure element has an `"allocate_key"` function whose intended purpose is to obtain an identifier for the key. This may be, for example, a unique label or a slot number. -When creating a persistent key with an opaque driver which does not have an `"allocate_key"` function: +When creating a persistent key with an opaque driver which has an `"allocate_key"` entry point: 1. The core calls the driver's `"allocate_key"` function. This function typically allocates an identifier for the key without modifying the state of the secure element and stores the identifier in the key context. This function should not modify the state of the secure element. @@ -462,7 +462,6 @@ psa_key_handle_t handle = 0; psa_generate_key(&attributes, &handle); ``` - ## Using opaque drivers from an application The a compile-time constant for each opaque driver indicating its location called `PSA_KEY_LOCATION_`*prefix* where *prefix* is the value of the `"prefix"` property in the driver description. For convenience, Mbed TLS also declares a compile-time constant for the corresponding lifetime with the default persistence called `PSA_KEY_LIFETIME_`*prefix*. Therefore, to declare an opaque key in the location with the prefix `foo` with the default persistence, call `psa_set_key_lifetime` during the key creation as follows: From 8d06ad0177bef060990e82b9e659383e280f6917 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Aug 2020 11:37:02 +0200 Subject: [PATCH 37/55] Rework and expand key management in opaque drivers Opaque drivers only have a destroy function if the key is stored in the secure element. Expand on how key creation works. Provide more explanations of allocate_key in drivers for secure elements with storage. Discuss key destruction as well. Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 99 +++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 15 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index 07e83a826..ba31cd5c5 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -388,45 +388,114 @@ If the key is stored in wrapped form outside the secure element, and the wrapped Transparent drivers may provide the following key management entry points: -* `"allocate_key"`: called by `psa_import_key()`, `psa_generate_key()`, `psa_key_derivation_output_key()` or `psa_copy_key()` before creating a key in the location of this driver. -* `"import_key"`: called by `psa_import_key()`, or by `psa_copy_key()` when copying a key from another location. * `"export_key"`: called by `psa_export_key()`, or by `psa_copy_key()` when copying a key from to location. * `"export_public_key"`: called by the core to obtain the public key of a key pair. The core may call this entry point at any time to obtain the public key, which can be for `psa_export_public_key()` but also at other times, including during a cryptographic operation that requires the public key such as a call to `psa_verify_message()` on a key pair object. -* `"copy_key"`: called by `psa_copy_key()` when copying a key within the same location. -* `"destroy_key"`: called by `psa_destroy_key()`. +* `"import_key"`: called by `psa_import_key()`, or by `psa_copy_key()` when copying a key from another location. * `"generate_key"`: called by `psa_generate_key()`. * `"derive_key"`: called by `psa_key_derivation_output_key()`. +* `"copy_key"`: called by `psa_copy_key()` when copying a key within the same location. + +In addition, secure elements that store the key material internally must provide the following two entry points: + +* `"allocate_key"`: called by `psa_import_key()`, `psa_generate_key()`, `psa_key_derivation_output_key()` or `psa_copy_key()` before creating a key in the location of this driver. +* `"destroy_key"`: called by `psa_destroy_key()`. #### Key creation in a secure element without storage -This section describes the key creation process for secure elements that do not store the key material. The driver must obtain a wrapped form of the key material which the core will store. A driver for such a secure element has no `"allocate_key"` entry point. +This section describes the key creation process for secure elements that do not store the key material. The driver must obtain a wrapped form of the key material which the core will store. A driver for such a secure element has no `"allocate_key"` or `"destroy_key"` entry point. -When creating a key with an opaque driver which does not have an `"allocate_key"` entry point: +When creating a key with an opaque driver which does not have an `"allocate_key"` or `"destroy_key"` entry point: 1. The core allocates memory for the key context. 2. The core calls the driver's import, generate, derive or copy function. 3. The core saves the resulting wrapped key material and any other data that the key context may contain. -#### Key creation in a secure element with storage +To destroy a key, the core simply destroys the wrapped key material, without invoking driver code. -This section describes the key creation process for secure elements that have persistent storage for the key material. A driver for such a secure element has an `"allocate_key"` function whose intended purpose is to obtain an identifier for the key. This may be, for example, a unique label or a slot number. +#### Key management in a secure element with storage + +This section describes the key creation and key destruction processes for secure elements that have persistent storage for the key material. A driver for such a secure element has two mandatory entry points: + +* `"allocate_key"`: this function obtains an internal identifier for the key. This may be, for example, a unique label or a slot number. +* `"destroy_key"`: this function invalidates the internal identifier and destroys the associated key material. + +These functions have the following prototypes: +``` +psa_status_t acme_allocate_key(const psa_key_attributes_t *attributes, + uint8_t *key_buffer, + size_t key_buffer_size); +psa_status_t acme_destroy_key(const psa_key_attributes_t *attributes, + const uint8_t *key_buffer, + size_t key_buffer_size); +``` When creating a persistent key with an opaque driver which has an `"allocate_key"` entry point: -1. The core calls the driver's `"allocate_key"` function. This function typically allocates an identifier for the key without modifying the state of the secure element and stores the identifier in the key context. This function should not modify the state of the secure element. +1. The core calls the driver's `"allocate_key"` entry point. This function typically allocates an internal identifier for the key without modifying the state of the secure element and stores the identifier in the key context. This function should not modify the state of the secure element. It may modify the copy of the persistent state of the driver in memory. 1. The core saves the key context to persistent storage. -1. The core saves the driver's persistent state. +1. The core calls the driver's key creation entry point. -1. The core calls the driver's key creation function. +1. The core saves the updated key context to persistent storage. -If a failure occurs after the `"allocate_key"` step but before the call to the second driver function, the core will do one of the following: +If a failure occurs after the `"allocate_key"` step but before the call to the second driver entry point, the core will do one of the following: -* Fail the creation of the key without indicating this to the driver. This can happen, in particular, if the device loses power immediately after the key allocation function returns. -* Call the driver's `"destroy_key"` function. +* Fail the creation of the key without indicating this to the driver. This can happen, in particular, if the device loses power immediately after the key allocation entry point returns. +* Call the driver's `"destroy_key"` entry point. -TODO: explain the individual key management functions +To destroy a key, the core calls the driver's `"destroy_key"` entry point. + +Note that the key allocation and destruction entry point must not rely solely on the key identifier in the key attributes to identify a key. Some implementations of the PSA Crypto API store keys on behalf of multiple clients, and different clients may use the same key identifier to designate different keys. The manner in which the core distinguishes keys that have the same identifier but are part of the key namespace for different clients is implementation-dependent and is not accessible to drivers. Some typical strategies to allocate an internal key identifier are: + +* Maintain a set of free slot numbers which is stored either in the secure element or in the driver's persistent storage. To allocate a key slot, find a free slot number, mark it as occupied and store the number in the key context. When the key is destroyed, mark the slot number as free. +* Maintain a monotonic counter with a practically unbounded range in the secure element or in the driver's persistent storage. To allocate a key slot, increment the counter and store the current value in the key context. Destroying a key does not change the counter. + +TODO: explain constraints on how the driver updates its persistent state for resilience + +TODO: some of the above doesn't apply to volatile keys + +#### Key creation entry points in opaque drivers + +The key creation entry points have the following prototypes: + +``` +psa_status_t acme_import_key(const psa_key_attributes_t *attributes, + const uint8_t *data, + size_t data_length, + uint8_t *key_buffer, + size_t key_buffer_size); +psa_status_t acme_generate_key(const psa_key_attributes_t *attributes, + uint8_t *key_buffer, + size_t key_buffer_size); +``` + +If the driver has an [`"allocate_key"` entry point](#key-management-in-a-secure-element-with-storage), the core calls the `"allocate_key"` entry point with the same attributes on the same key buffer before calling the key creation function. + +TODO: derivation, copy + +#### Key export entry points in opaque drivers + +The key export entry points have the following prototypes: + +``` +psa_status_t acme_export_key(const psa_key_attributes_t *attributes, + const uint8_t *key_buffer, + size_t key_buffer_size); + uint8_t *data, + size_t data_size, + size_t *data_length); +psa_status_t acme_export_public_key(const psa_key_attributes_t *attributes, + const uint8_t *key_buffer, + size_t key_buffer_size); + uint8_t *data, + size_t data_size, + size_t *data_length); +``` + +The core will only call `acme_export_public_key` on a private key. Drivers implementers may choose to store the public key in the key context buffer or to recalculate it on demand. If the key context includes the public key, it needs to have an adequate size; see [“Key format for opaque drivers”](#key-format-for-opaque-drivers). + +The core guarantees that the size of the output buffer (`data_size`) is sufficient to export any key with the given attributes. The driver must set `*data_length` to the exact size of the exported key. ### Opaque driver persistent state From c1d388ae549c39cf6f2701647e6a2a8f07ed641e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Aug 2020 12:02:30 +0200 Subject: [PATCH 38/55] Change driver persistent data to a callback interface Rather than have some functions take the in-memory copy of the persistent data as argument, allow all of them to access the persistent data, including modifying it. Signed-off-by: Gilles Peskine --- docs/proposed/psa-driver-interface.md | 35 ++++++++++++++++++--------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/docs/proposed/psa-driver-interface.md b/docs/proposed/psa-driver-interface.md index ba31cd5c5..4402540d2 100644 --- a/docs/proposed/psa-driver-interface.md +++ b/docs/proposed/psa-driver-interface.md @@ -5,7 +5,7 @@ This document describes an interface for cryptoprocessor drivers in the PSA cryp This specification is work in progress and should be considered to be in a beta stage. There is ongoing work to implement this interface in Mbed TLS, which is the reference implementation of the PSA Cryptography API. At this stage, Arm does not expect major changes, but minor changes are expected based on experience from the first implementation and on external feedback. -Time-stamp: "2020/07/13 11:19:35 GMT" +Time-stamp: "2020/08/03 09:34:23 GMT" ## Introduction @@ -281,14 +281,7 @@ When multiple drivers have an init entry point, the order in which they are call On platforms where the PSA Crypto implementation is a subsystem of a single application, the initialization of the PSA Crypto subsystem takes place during the call to `psa_crypto_init()`. On platforms where the PSA Crypto implementation is separate from the application or applications, the initialization the initialization of the PSA Crypto subsystem takes place before or during the first time an application calls `psa_crypto_init()`. -For transparent drivers, the init function does not take any parameter. - -For opaque drivers, the init function takes the following parameters: - -* `uint8_t *persistent_state`: the driver's persistent state. On the first boot of the device, this contains all-bits-zero. On subsequent boots, the core loads the last saved state. -* `size_t persistent_state_size`: the size of the persistent state in bytes. - -For an opaque driver, if the init function succeeds, the core saves the updated persistent state. If the init function fails, the persistent state is unchanged. +The init function does not take any parameter. ### Combining multiple drivers @@ -501,7 +494,23 @@ The core guarantees that the size of the output buffer (`data_size`) is sufficie The core maintains persistent state on behalf of an opaque driver. This persistent state consists of a single byte array whose size is given by the `"persistent_state_size"` property in the [driver description](#driver-description-top-level-element). -TODO: how the state is passed to the driver; which driver functions can modify the state and how; when the core saves the updated state +The core loads the persistent state in memory before it calls the driver's [init entry point](#driver-initialization). It is adjusted to match the size declared by the driver, in case a driver upgrade changes the size: + +* The first time the driver is loaded on a system, the persistent state is all-bits-zero. +* If the stored persistent state is smaller than the declared size, the core pads the persistent state with all-bits-zero at the end. +* If the stored persistent state is larger than the declared size, the core truncates the persistent state to the declared size. + +The core provides the following callback functions, which an opaque driver may call while it is processing a call from the driver: +``` +psa_status_t psa_crypto_driver_get_persistent_state(uint_8_t **persistent_state_ptr); +psa_status_t psa_crypto_driver_update_persistent_state(size_t from, size_t length); +``` + +`psa_crypto_driver_get_persistent_state` sets `*persistent_state_ptr` to a pointer to the first byte of the persistent state. This pointer remains valid during a call to a driver entry point. Once the entry point returns, the pointer is no longer valid. The core guarantees that calls to `psa_crypto_driver_get_persistent_state` within the same entry point return the same address for the persistent state, but this address may change between calls to an entry point. + +`psa_crypto_driver_update_persistent_state` updates the persistent state in persistent storage. Only the portion at byte offsets `from` inclusive to `from + length` exclusive is guaranteed to be updated; it is unspecified whether changes made to other parts of the state are taken into account. The driver must call this function after updating the persistent state in memory and before returning from the entry point, otherwise it is unspecified whether the persistent state is updated. + +In a multithreaded environment, the driver may only call these two functions from the thread that is executing the entry point. ## How to use drivers from an application @@ -607,7 +616,11 @@ Note that a solution also has to work for transparent keys, and when importing a #### Opaque driver persistent state -Should the driver be able to update it at any time? +The driver is allowed to update the state at any time. Is this ok? + +An example use case for updating the persistent state at arbitrary times is to renew a key that is used to encrypt communications between the application processor and the secure element. + +`psa_crypto_driver_get_persistent_state` does not identify the calling driver, so the driver needs to remember which driver it's calling. This may require a thread-local variable in a multithreaded core. Is this ok?