This commit improves hygiene and formatting of macro definitions
throughout the library. Specifically:
- It adds brackets around parameters to avoid unintended
interpretation of arguments, e.g. due to operator precedence.
- It adds uses of the `do { ... } while( 0 )` idiom for macros that
can be used as commands.
This is the first in a series of commits adding client-side
support for PSA-based ECDHE.
Previously, the state of an ECDHE key agreement was maintained
in the field mbedtls_ssl_handshake_params::ecdh_ctx, of type
::mbedtls_ecdh_context and manipulated through the ECDH API.
The ECDH API will be superseeded by the PSA Crypto API for key
agreement, which needs the following data:
(a) A raw buffer holding the public part of the key agreement
received from our peer.
(b) A key slot holding the private part of the key agreement.
(c) The algorithm to use.
The commit adds fields to ::mbedtls_ssl_handshake_params
representing these three inputs to PSA-based key agreement.
Specifically, it adds a field for the key slot holding the
ECDH private key, a field for the EC curve identifier, and
a buffer holding the peer's public key.
Note: Storing the peer's public key buffer is slightly
inefficient, as one could perform the ECDH computation
as soon as the peer sends its public key, either working
with in-place or using a stack-buffer to reformat the
public key before passing it to PSA. This optimization
is left for a later commit.
Silence a compiler warning about implicit fallthrough by using a comment
format the compiler understand to mean that the fallthrough is
intentional.
In file included from library/cipher.c:63:0:
include/mbedtls/psa_util.h: In function ‘mbedtls_psa_translate_cipher_mode’:
include/mbedtls/psa_util.h:91:15: error: this statement may fall through [-Werror=implicit-fallthrough=]
if( taglen == 0 )
^
include/mbedtls/psa_util.h:94:9: note: here
default:
^~~~~~~
cc1: all warnings being treated as errors
$ gcc --version
gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Context:
The existing API `mbedtls_x509_parse_crt_der()` for parsing DER
encoded X.509 CRTs unconditionally makes creates a copy of the
input buffer in RAM. While this comes at the benefit of easy use,
-- specifically: allowing the user to free or re-use the input
buffer right after the call -- it creates a significant memory
overhead, as the CRT is duplicated in memory (at least temporarily).
This might not be tolerable a resource constrained device.
As a remedy, this commit adds a new X.509 API call
`mbedtls_x509_parse_crt_der_nocopy()`
which has the same signature as `mbedtls_x509_parse_crt_der()`
and almost the same semantics, with one difference: The input
buffer must persist and be unmodified for the lifetime of the
established instance of `mbedtls_x509_crt`, that is, until
`mbedtls_x509_crt_free()` is called.
Resolve incompatibilties in the RSA module where changes made for
parameter validation prevent Mbed Crypto from working. Mbed Crypto
depends on being able to pass zero-length buffers that are NULL to RSA
encryption functions.
This reverts commit 2f660d047d.
Context: There are two public key writing functions in Mbed TLS. First,
mbedtls_pk_write_pubkey(), which exports a public key in the form of a
SubjectPublicKey structure containing the raw keying material
(for example, EC point coordinates for an EC public key, without
reference to the underlying curve). Secondly, mbedtls_pk_write_pubkey_der(),
which exports a public key in the form of a SubjectPublicKeyInfo structure,
wrapping the SubjectPublicKey structure by additional information
identifying the type of public key (and for ECC, e.g., it'd also contain
the ECC group identifier). The implementation of mbedtls_pk_write_pubkey_der()
calls mbedtls_pk_write_pubkey() first and then adds the corresponding
algorithm identifier wrapper.
Both of these functions need to be provided for PSA-based opaque PK contexts,
based on PSA's public key export function.
Previously, PSA used the SubjectPublicKeyInfo structure as its export format,
so mbedtls_pk_write_pubkey_der() could be easily implemented, while
mbedtls_pk_write_pubkey() would need to trim the output of the PSA export.
The previous implementation of mbedtls_pk_write_pubkey() is not quite right
because it calls PSA export doesn't do any trimming, hence exporting the large
SubjectPublicKeyInfo structure instead of the small SubjectPublicKey.
mbedtls_pk_write_pubkey_der(), in turn, immediately returns after calling
mbedtls_pk_write_pubkey(), hence also returning the SubjectPublicKeyInfo
structure, which is correct.
By now, the PSA public key export format has changed to the smaller
SubjectPublicKey structure. This means that, now, mbedtls_pk_write_pubkey()
can be implemented by just calling the PSA export, and that
mbedtls_pk_write_pubkey_der() needs to add the algorithm information around
it, just as in the other types of PK contexts. While not correct for the
old format, the existing code for mbedtls_pk_write_pubkey() is therefore
correct for the new PSA public key format, and needs no change apart from
the missing pointer shift in the last commit.
The implementation of mbedtls_pk_write_pubkey_der() needs a special code
path for PSA-based opaque PK contexts, as the PK context only contains
the PSA key handle, and the PSA API needs to be used to extract the
underlying EC curve to be able to write the AlgorithmParameter structure
that's part of the SubjectPublicKeyInfo structure.
That's what this commit does, (hopefully) making both
mbedtls_pk_write_pubkey() and mbedtls_pk_write_pubkey_der() export
the correctly formatted public key based on the new PSA public key format.
Additional changes to temporarily enable running tests:
ssl_srv.c and test_suite_ecdh use mbedtls_ecp_group_load instead of
mbedtls_ecdh_setup
test_suite_ctr_drbg uses mbedtls_ctr_drbg_update instead of
mbedtls_ctr_drbg_update_ret
The file oid.c had conditional inclusion of functions based on a config.h
define that belongs to X.509, which is backwards. For now, just include those
functions unconditionally and rely on the linker to garbage-collect them if
not used.
In the longer term X.509-specific functions are likely to be removed from
libmbedcrypto, but at this step the goal is to preserve the API (and even ABI)
of libmbedcrypto for as long as possible while separating the source trees of
Mbed Crypto and Mbed TLS.
As agreed during the workshop, temporarily move definitions to oid.h even if
they might not semantically belong here, as a short-term measure allowing to
build libmbecrypto on its own (without X.509 files present in the source tree)
but still provide all the things Mbed TLS currently expects, and more
specifically preserve the API and ABI exposed by libmbedtls.
As there are some definitions that are defined regardless of
whether MBEDTLS_ECP_RESTARTABLE is defined or not, these definitions
need to be moved outside the MBEDTLS_ECP_ALT guards. This is a simple
move as MBEDTLS_ECP_ALT and MBEDTLS_ECP_RESTARTABLE are mutually
exclusive options.
Document when a context must be initialized or not, when it must be
set up or not, and whether it needs a private key or a public key will
do.
The implementation is sometimes more liberal than the documentation,
accepting a non-set-up context as a context that can't perform the
requested information. This preserves backward compatibility.
For mbedtls_pk_parse_key and mbedtls_pk_parse_keyfile, the password is
optional. Clarify what this means: NULL is ok and means no password.
Validate parameters and test accordingly.
A 0-length buffer for the key is a legitimate edge case. Ensure that
it works, even with buf=NULL. Document the key and keylen parameters.
There are already test cases for parsing an empty buffer. A subsequent
commit will add tests for writing to an empty buffer.
The previous introduction of constant deprecation macros
in platform_util.h lead to failure of tests/scrips/check-names.sh
because the regular expressions in the latter choked on the brackets
in the part `__attribute__((deprecated))` of the definition of the
helper type `mbedtls_deprecated_{numeric|string}_constant_t`.
Postponing any further study and potential robustness improvements
in check-names.sh to another time, this commit circumvents this
problem by temporarily abbreviating `__attribute__((deprecated))`
as `MBEDTLS_DEPRECATED`, which doesn't lead to problems with
check-names.sh.
This commit introduces macros
* MBEDTLS_DEPRECATED_STRING_CONSTANT
* MBEDTLS_DEPRECATED_NUMERIC_CONSTANT
to platform_util.h which can be used to deprecate public macro constants.
Their definition is essentially taken from dhm.h where the
MBEDTLS_DEPRECATED_STRING_CONSTANT was used to deprecate
insecure hardcoded DHM primes.
Add inclusion to configration file in header files,
instead of relying on other header files to include
the configuration file. This issue resolves#1371
- Be specific about the constraints: be a readable/writable buffer of length
X, be an initialized context, be a context initialized and bound to a key...
- Always use full sentences with all the required pronouns.
It's better if the macro receives the condition as an expression rather than a
string - that way it can choose to use it as is or stringify it. Also, the
documentation states that the parameter is an expression, not a string.
The test framework for validation of parameters depends on the macro
MBEDTLS_PARAM_FAILED() being set to its default value when building the
library. So far the test framework attempted to define this macro but this was
the wrong place - this definition wouldn't be picked by the library.
Instead, a different approach is taken: skip those tests when the macro is
defined in config.h, as in that case we have no way to know if it will indeed
end up calling mbedtls_param_failed() as we need it to.
This commit was tested by manually ensuring that aes_invalid_params:
- passes (and is not skipped) in the default configuration
- is skipped when MBEDTLS_PARAM_FAILED() is defined in config.h
The previous prototype gave warnings are the strings produced by #cond and
__FILE__ are const, so we shouldn't implicitly cast them to non-const.
While at it modifying most example programs:
- include the header that has the function declaration, so that the definition
can be checked to match by the compiler
- fix whitespace
- make it work even if PLATFORM_C is not defined:
- CHECK_PARAMS is not documented as depending on PLATFORM_C and there is
no reason why it should
- so, remove the corresponding #if defined in each program...
- and add missing #defines for mbedtls_exit when needed
The result has been tested (make all test with -Werror) with the following
configurations:
- full with CHECK_PARAMS with PLATFORM_C
- full with CHECK_PARAMS without PLATFORM_C
- full without CHECK_PARAMS without PLATFORM_C
- full without CHECK_PARAMS with PLATFORM_C
Additionally, it has been manually tested that adding
mbedtls_aes_init( NULL );
near the normal call to mbedtls_aes_init() in programs/aes/aescrypt2.c has the
expected effect when running the program.
It was inconsistent between files: sometimes 3 arguments, sometimes one.
Align to 1 argument for the macro and 3 for the function, because:
- we don't need 3 arguments for the macro, it can add __FILE__ and __LINE__
in its expansion, while the function needs them as parameters to be correct;
- people who re-defined the macro should have flexibility, and 3 arguments
can give the impression they they don't have as much as they actually do;
- the design document has the macro with 1 argument, so let's stick to that.
The function called through the macro MBEDTLS_PARAM_FAILED() must be supplied by
users and makes no sense as a library function, apart from debug and test.
