Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom methods for key generation and key derivation #167

Open
gilles-peskine-arm opened this issue Feb 12, 2024 · 8 comments · May be fixed by #194
Open

Custom methods for key generation and key derivation #167

gilles-peskine-arm opened this issue Feb 12, 2024 · 8 comments · May be fixed by #194
Labels
API design Related the design of the API Crypto API Issue or PR related to the Cryptography API enhancement New feature or request

Comments

@gilles-peskine-arm
Copy link
Contributor

The PSA API should provide ways to customize how key material is created from random or pseudorandom inputs, i.e. key generation and key derivation. The scope here is for customizing the construction of keys (especially cooked keys, i.e. keys that aren't just chosen uniformly among bit-strings of a given length). Here are some possible use cases:

  • Generate an RSA key with a chosen public exponent. The current API imposes e=65537. There is demand for e=3, and occasionally for other values. This is also relevant for derivation, although the PSA API does not specify how to do derivation for RSA.
  • Choose among several plausible methods for deriving an RSA key. This would likely remain implementation-specific, since the PSA API does not specify how to do derivation for RSA.
  • When deriving an ECC key (which involves randomly selecting an integer in a range that isn't a power of 2), use the fixed-input-size-divided-by-N method rather than the method imposed by the API, which is to read input repeatedly until it's in range.
  • Choose a custom random generator when generating a key randomly.

Urgency: Mbed TLS is interested in having a way to generate an RSA key with a chosen public exponent soon. Ideally we would like to include a beta version of this in our next release (code freeze: mid-March), but we don't know whether we'll have time to do the coding even.

My general idea is to define new functions psa_generate_key_ext() and psa_key_derivation_output_key_ext() (tentative names), which are similar to psa_generate_key() and psa_key_derivation_output_key() but take an extra parameter indicating the custom generation/derivation method. The type of the method parameter has a default value which means the same thing that the non-ext functions do.

@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request API design Related the design of the API Crypto API Issue or PR related to the Cryptography API labels Feb 12, 2024
@gilles-peskine-arm
Copy link
Contributor Author

See Mbed-TLS/mbedtls#8815 for an API proposal (with a prototype implementation).

@gilles-peskine-arm
Copy link
Contributor Author

On Tuesday 13 Feb 2024, we decided to go ahead with the design in Mbed-TLS/mbedtls#8815 at Mbed-TLS/mbedtls@d8ba2b8 with the change from passing method_length to passing method_data_length as proposed in Mbed-TLS/mbedtls#8815 (comment).

@gilles-peskine-arm
Copy link
Contributor Author

A design question came up: if a function takes an input via parameters const psa_production_parameters_t *params, size_t params_length, it is not a “buffer” as defined by the parameter conventions. So according to the letter of the specification, as it stands, it would not allow overlap and would not require the implementation to take any precautions in an environment with shared memory.

However, *params does contain a variable-length buffer. Consider an implementation with domain separation where parameters are passed through shared memory (needing care in the implementation of the function) and small, fixed-size parameters are copied by the RPC code via one shared memory zone (so the function doesn't need any particular care). It would make sense for the RPC code to treat this variable-length parameter as a buffer that just happens to have a fixed-format header, and is passed directly as shared memory.

Alternatively, for the current use case, it would make sense for implementations to define a reasonably short PSA_PRODUCTION_PARAMETERS_MAX_SIZE (that's maybe something we should have anyway?), and then the RPC code can always transmit that many bytes as a fixed-size buffer. (Though it might still want to keep it variable-length because the max size could still be high by the standards of a constrained device — typically one RSA key length whereas most calls will actually use 0 bytes.)

What do you think @athoelke ?

@athoelke
Copy link
Contributor

I think that as this has a variable length element, even if the current use cases do not have a wildly varying size, it might be best to categorise this as a 'buffer'-type parameter. This gives the implementation freedom to read-in-place, or to copy-to-cryptoprocessor (or decide at runtime based on the size of the parameter).

Note that this is an input parameter, and the calls in which it is used have (aside from the construction parameters):

  • One non-buffer-memory input parameter (the key attributes)
  • One non-buffer-memory output parameter (the key handle)

Therefore, however we classify the construction parameters, it makes no material difference to the behavior of the implementation in terms of 'overlap' or 'stability' - it cannot reasonably overlap with the output key handle (without forcing the compiler to cast pointers), and overlap with other inputs is irrelevant.

@gilles-peskine-arm
Copy link
Contributor Author

it makes no material difference to the behavior of the implementation in terms of 'overlap' or 'stability'

Overlap, no. But stability, yes. The implementation is allowed to assume stability for an input non-buffer (e.g. const psa_key_attributes_t *) but not for an input buffer (e.g. const uint8_t *key_data).

@athoelke
Copy link
Contributor

Overlap, no. But stability, yes. The implementation is allowed to assume stability for an input non-buffer (e.g. const psa_key_attributes_t *) but not for an input buffer (e.g. const uint8_t *key_data).

But in this case what does 'assuming stability' look like?

For these two APIs, the only agent that might be modifying the buffer while the function is in progress is the application (or something else outside of the implementation) - there is no output parameter that can legally overlap with this input, so the implementation can assume that it will not corrupt this input parameter as a side-effect of its operation.

Are we suggesting that the implementation is permitted to be written in a way that might fail badly (i.e. crash, divulge secrets, generate an invalid/insecure key (but report success)) if the data in this parameter content is changed (by the caller) while the function is in progress? I would expect that implementations that do not trust the caller are written to mitigate such misbehavior.

@athoelke
Copy link
Contributor

Perhaps the critical aspect here is that the flags field in the parameters is expected to control the behavior (code flow) within the implementation (e.g. by indicating a specific construction method), and the variable sized part is expected to provide data inputs to the construction methods. i.e. the flags will control the parsing of the variable length data.

Maybe ideally, the flags should be treated as a 'stable' parameter, but the data buffer not?

On the other hand, we already have an API that mixes control and data within a 'non-stable' parameter: for some key formats used with psa_import_key(), the input data includes control information that affects the parsing of the data in the buffer.

@athoelke
Copy link
Contributor

As noted in #194 (comment) and Mbed-TLS/mbedtls#9020, this API is unsupportable in a project that might be included and called from C++ source code.

We will have to separate the structure-like parts of the production parameters, from any variable-sized parameters. The suggestion is to rework the API definition to the following:

typedef struct psa_key_production_parameters_t {
    uint32_t flags;
    /* imp-def */
} psa_key_production_parameters_t;

psa_status_t psa_generate_key_ext(const psa_key_attributes_t *attributes,
                                  const psa_key_production_parameters_t *params,
                                  const uint8_t *params_data,
                                  size_t params_data_length,
                                  mbedtls_svc_key_id_t *key);

psa_status_t psa_key_derivation_output_key_ext(const psa_key_attributes_t *attributes,
                                               psa_key_derivation_operation_t *operation,
                                               const psa_key_production_parameters_t *params,
                                               const uint8_t *params_data,
                                               size_t params_data_length,
                                               mbedtls_svc_key_id_t *key);

The unbounded array data[] is removed from the production parameter structure, and passed as a separate params_data buffer to the extended key generation and derivation functions. The psa_key_production_parameters_t structure is retained to initially contain the flags that indicate the remaining parameter content, and for addition of fixed-size parameter values in future specifications (or for implementation-specific key-production procedures).

If params.flags == 0 && params_data_length == 0 then these APIs will use the default key generation/derivation procedures. Note that (params_data, params_data_length) is now a standard input buffer parameter.

This alternative API is now compatible with C++ compilation, but also no longer requires special consideration of whether a parameter is a 'buffer-type parameter'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design Related the design of the API Crypto API Issue or PR related to the Cryptography API enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

2 participants