When using this function to deserialize, it's not a problem to have a session
structure as input as we'll have one around anyway (most probably freshly
deserialised).
However for tests it's convenient to be able to build a transform without
having a session structure around.
Also, removing this structure from parameters makes the function signature
more uniform, the only exception left being the ssl param at the end that's
hard to avoid for now.
We called in tinycrypt in the file names, but uecc in config.h, all.sh and
other places, which could be confusing. Just use tinycrypt everywhere because
that's the name of the project and repo where we took the files.
The changes were made using the following commands (with GNU sed and zsh):
sed -i 's/uecc/tinycrypt/g' **/*.[ch] tests/scripts/all.sh
sed -i 's/MBEDTLS_USE_UECC/MBEDTLS_USE_TINYCRYPT/g' **/*.[ch] tests/scripts/all.sh scripts/config.pl
Configs with no DEBUG_C are use for example in test-ref-configs.pl, which also
runs parts of compat.sh or ssl-opt.sh on them, so the added 'ssl = NULL'
statements will be exercised in those tests at least.
Make it more explicit what's used. Unfortunately, we still need ssl as a
parameter for debugging, and because calc_verify wants it as a parameter (for
all TLS versions except SSL3 it would actually only need handshake, but SSL3
also accesses session_negotiate).
It's also because of calc_verify that we can't make it const yet, but see next
commit.
This commit adds tests exercising mutually inverse pairs of
record encryption and decryption transformations for the various
transformation types allowed in TLS: Stream, CBC, and AEAD.
The hash contexts `ssl_transform->md_ctx_{enc/dec}` are not used if
only AEAD ciphersuites are enabled. This commit removes them from the
`ssl_transform` struct in this case, saving a few bytes.
This commit guards code specific to AEAD, CBC and stream cipher modes
in `ssl_derive_keys` by the respective configuration flags, analogous
to the guards that are already in place in the record decryption and
encryption functions `ssl_decrypt_buf` resp. `ssl_decrypt_buf`.
Analogous to the previous commit, but concerning the record decryption
routine `ssl_decrypt_buf`.
An important change regards the checking of CBC padding:
Prior to this commit, the CBC padding check always read 256 bytes at
the end of the internal record buffer, almost always going past the
boundaries of the record under consideration. In order to stay within
the bounds of the given record, this commit changes this behavior by
always reading the last min(256, plaintext_len) bytes of the record
plaintext buffer and taking into consideration the last `padlen` of
these for the padding check. With this change, the memory access
pattern and runtime of the padding check is entirely determined by
the size of the encrypted record, in particular not giving away
any information on the validity of the padding.
The following depicts the different behaviors:
1) Previous CBC padding check
1.a) Claimed padding length <= plaintext length
+----------------------------------------+----+
| Record plaintext buffer | | PL |
+----------------------------------------+----+
\__ PL __/
+------------------------------------...
| read for padding check ...
+------------------------------------...
|
contents discarded
from here
1.b) Claimed padding length > plaintext length
+----------------------------------------+----+
| Record plaintext buffer | PL |
+----------------------------------------+----+
+-------------------------...
| read for padding check ...
+-------------------------...
|
contents discarded
from here
2) New CBC padding check
+----------------------------------------+----+
| Record plaintext buffer | | PL |
+----------------------------------------+----+
\__ PL __/
+---------------------------------------+
| read for padding check |
+---------------------------------------+
|
contents discarded
until here
The previous version of the record encryption function
`ssl_encrypt_buf` takes the entire SSL context as an argument,
while intuitively, it should only depend on the current security
parameters and the record buffer.
Analyzing the exact dependencies, it turned out that in addition
to the currently active `ssl_transform` instance and the record
information, the encryption function needs access to
- the negotiated protocol version, and
- the status of the encrypt-then-MAC extension.
This commit moves these two fields into `ssl_transform` and
changes the signature of `ssl_encrypt_buf` to only use an instance
of `ssl_transform` and an instance of the new `ssl_record` type.
The `ssl_context` instance is *solely* kept for the debugging macros
which need an SSL context instance.
The benefit of the change is twofold:
1) It avoids the need of the MPS to deal with instances of
`ssl_context`. The MPS should only work with records and
opaque security parameters, which is what the change in
this commit makes progress towards.
2) It significantly eases testing of the encryption function:
independent of any SSL context, the encryption function can
be passed some record buffer to encrypt alongside some arbitrary
choice of parameters, and e.g. be checked to not overflow the
provided memory.
The macro constant `MBEDTLS_SSL_MAC_ADD` defined in `ssl_internal.h`
defines an upper bound for the amount of space needed for the record
authentication tag. Its definition distinguishes between the
presence of an ARC4 or CBC ciphersuite suite, in which case the maximum
size of an enabled SHA digest is used; otherwise, `MBEDTLS_SSL_MAC_ADD`
is set to 16 to accomodate AEAD authentication tags.
This assignment has a flaw in the situation where confidentiality is
not needed and the NULL cipher is in use. In this case, the
authentication tag also uses a SHA digest, but the definition of
`MBEDTLS_SSL_MAC_ADD` doesn't guarantee enough space.
The present commit fixes this by distinguishing between the presence
of *some* ciphersuite using a MAC, including those using a NULL cipher.
For that, the previously internal macro `SSL_SOME_MODES_USE_MAC` from
`ssl_tls.c` is renamed and moved to the public macro
`MBEDTLS_SOME_MODES_USE_MAC` defined in `ssl_internal.h`.
Prior to this commit, the security parameter struct `ssl_transform`
contained a `ciphersuite_info` field pointing to the information
structure for the negotiated ciphersuite. However, the only
information extracted from that structure that was used in the core
encryption and decryption functions `ssl_encrypt_buf`/`ssl_decrypt_buf`
was the authentication tag length in case of an AEAD cipher.
The present commit removes the `ciphersuite_info` field from the
`ssl_transform` structure and adds an explicit `taglen` field
for AEAD authentication tag length.
This is in accordance with the principle that the `ssl_transform`
structure should contain the raw parameters needed for the record
encryption and decryption functions to work, but not the higher-level
information that gave rise to them. For example, the `ssl_transform`
structure implicitly contains the encryption/decryption keys within
their cipher contexts, but it doesn't contain the SSL master or
premaster secrets. Likewise, it contains an explicit `maclen`, while
the status of the 'Truncated HMAC' extension -- which determines the
value of `maclen` when the `ssl_transform` structure is created in
`ssl_derive_keys` -- is not contained in `ssl_transform`.
The `ciphersuite_info` pointer was used in other places outside
the encryption/decryption functions during the handshake, and for
these functions to work, this commit adds a `ciphersuite_info` pointer
field to the handshake-local `ssl_handshake_params` structure.
The `ssl_transform` security parameter structure contains opaque
cipher contexts for use by the record encryption/decryption functions
`ssl_decrypt_buf`/`ssl_encrypt_buf`, while the underlying key material
is configured once in `ssl_derive_keys` and is not explicitly dealt with
anymore afterwards. In particular, the key length is not needed
explicitly by the encryption/decryption functions but is nonetheless
stored in an explicit yet superfluous `keylen` field in `ssl_transform`.
This commit removes this field.
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.
* restricted/pr/553:
Fix mbedtls_ecdh_get_params with new ECDH context
Add changelog entry for mbedtls_ecdh_get_params robustness
Fix ecdh_get_params with mismatching group
Add test case for ecdh_get_params with mismatching group
Add test case for ecdh_calc_secret
Fix typo in documentation
* origin/pr/2436:
Use certificates from data_files and refer them
Specify server certificate to use in SHA-1 test
refactor CA and SRV certificates into separate blocks
refactor SHA-1 certificate defintions and assignment
refactor server SHA-1 certificate definition into a new block
define TEST_SRV_CRT_RSA_SOME in similar logic to TEST_CA_CRT_RSA_SOME
server SHA-256 certificate now follows the same logic as CA SHA-256 certificate
add entry to ChangeLog
* restricted/pr/550:
Update query_config.c
Fix failure in SSLv3 per-version suites test
Adjust DES exclude lists in test scripts
Clarify 3DES changes in ChangeLog
Fix documentation for 3DES removal
Exclude 3DES tests in test scripts
Fix wording of ChangeLog and 3DES_REMOVE docs
Reduce priority of 3DES ciphersuites
If mbedtls_ecdh_get_params is called with keys belonging to
different groups, make it return an error the second time, rather than
silently interpret the first key as being on the second curve.
This makes the non-regression test added by the previous commit pass.
Refactor the function mbedtls_asn1_write_bitstring() that removes
trailing 0s at the end of DER encoded bitstrings. The function is
implemented according to Hanno Becker's suggestions.
This commit also changes the functions x509write_crt_set_ns_cert_type
and crt_set_key_usage to call the new function as the use named
bitstrings instead of the regular bitstrings.
In mbedtls_mpi_exp_mod(), the limit check on wsize is never true when
MBEDTLS_MPI_WINDOW_SIZE is at least 6. Wrap in a preprocessor guard
to remove the dead code and resolve a Coverity finding from the
DEADCODE checker.
Change-Id: Ice7739031a9e8249283a04de11150565b613ae89
Return the error code if failed, instead of returning value `1`.
If not failed, return the call of the underlying function,
in `mbedtls_ecdsa_genkey()`.
Use `cmake -D CMAKE_BUILD_TYPE=Asan` rather than manually setting
`-fsanitize=address`. This lets cmake determine the necessary compiler
and linker flags.
With UNSAFE_BUILD on, force -Wno-error. This is necessary to build
with MBEDTLS_TEST_NULL_ENTROPY.
mbedtls_mpi_read_binary() calls memcpy() with the source pointer being
the source pointer passed to mbedtls_mpi_read_binary(), the latter may
be NULL if the buffer length is 0 (and this happens e.g. in the ECJPAKE
test suite). The behavior of memcpy(), in contrast, is undefined when
called with NULL source buffer, even if the length of the copy operation
is 0.
This commit fixes this by explicitly checking that the source pointer is
not NULL before calling memcpy(), and skipping the call otherwise.
Context: The function `mbedtls_mpi_fill_random()` uses a temporary stack
buffer to hold the random data before reading it into the target MPI.
Problem: This is inefficient both computationally and memory-wise.
Memory-wise, it may lead to a stack overflow on constrained devices with
limited stack.
Fix: This commit introduces the following changes to get rid of the
temporary stack buffer entirely:
1. It modifies the call to the PRNG to output the random data directly
into the target MPI's data buffer.
This alone, however, constitutes a change of observable behaviour:
The previous implementation guaranteed to interpret the bytes emitted by
the PRNG in a big-endian fashion, while rerouting the PRNG output into the
target MPI's limb array leads to an interpretation that depends on the
endianness of the host machine.
As a remedy, the following change is applied, too:
2. Reorder the bytes emitted from the PRNG within the target MPI's
data buffer to ensure big-endian semantics.
Luckily, the byte reordering was already implemented as part of
`mbedtls_mpi_read_binary()`, so:
3. Extract bigendian-to-host byte reordering from
`mbedtls_mpi_read_binary()` to a separate internal function
`mpi_bigendian_to_host()` to be used by `mbedtls_mpi_read_binary()`
and `mbedtls_mpi_fill_random()`.
The calls to cipher_finish didn't actually do anything:
- the cipher mode is always ECB
- in that case cipher_finish() only sets *olen to zero, and returns either 0
or an error depending on whether there was pending data
- olen is a local variable in the caller, so setting it to zero right before
returning is not essential
- the return value of cipher_finis() was not checked by the caller so that's
not useful either
- the cipher layer does not have ALT implementations so the behaviour
described above is unconditional on ALT implementations (in particular,
cipher_finish() can't be useful to hardware as (with ECB) it doesn't call any
functions from lower-level modules that could release resources for example)
Since the calls are causing issues with parameter validation, and were no
serving any functional purpose, it's simpler to just remove them.
Somehow, mbedtls_sha256_ret() is defined even if MBEDTLS_SHA256_ALT
is set, and it is using SHA256_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA256_ALT does _not_ replace
the entire module, but only the core SHA-256 functions.
Somehow, mbedtls_sha512_ret() is defined even if MBEDTLS_SHA512_ALT
is set, and it is using SHA512_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA512_ALT does _not_ replace
the entire module, but only the core SHA-512 functions.
Somehow, mbedtls_sha1_ret() is defined even if MBEDTLS_SHA1_ALT
is set, and it is using SHA1_VALIDATE_RET. The documentation should
be enhanced to indicate that MBEDTLS_SHA1_ALT does _not_ replace
the entire module, but only the core SHA-1 functions.
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.
The MPI_VALIDATE_RET() macro cannot be used for parameter
validation of mbedtls_mpi_lsb() because this function returns
a size_t.
Use the underlying MBEDTLS_INTERNAL_VALIDATE_RET() insteaed,
returning 0 on failure.
Also, add a test for this behaviour.
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.
Add checks for null pointers under MBEDTLS_CHECK_PARAMS.
In functions that perform operations with a context, only check if the
context pointer is non-null under MBEDTLS_CHECK_PARAMS. In the default
configuration, unconditionally dereference the context pointer.
In functions that query a context, support NULL as a
pointer-to-context argument, and return the same value as for a
context which has been initialized but not set up.
- The validity of the input and output parameters is checked by
parameter validation.
- A PRNG is required in public mode only (even though it's also
recommended in private mode), so move the check to the
corresponding branch.
The check was already done later when calling ECB, (as evidenced by the tests
passing, which have a call with data_unit set to NULL), but it's more readable
to have it here too, and more helpful when debugging.
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.
Function calls to alternative implementations have to follow certain
rules in order to preserve correct functionality. To avoid accidentally
breaking these rules we state them explicitly in the ECP module for
ourselves and every contributor to see.
We initialized the ECC hardware before calling
mbedtls_ecp_mul_shortcuts(). This in turn calls
mbedtls_ecp_mul_restartable(), which initializes and frees the hardware
too. This issue has been introduced by recent changes and caused some
accelerators to hang.
We move the initialization after the mbedtle_ecp_mul_shortcuts() calls
to avoid double initialization.
The SSL module accesses ECDH context members directly. This can't work
with the new context, where we can't make any assumption about the
implementation of the context.
This commit makes use of the new functions to avoid accessing ECDH
members directly. The only members that are still accessed directly are
the group ID and the point format and they are independent from the
implementation.
The SSL module accesses ECDH context members directly to print debug
information. This can't work with the new context, where we can't make
assumptions about the implementation of the context. This commit adds
new debug functions to complete the encapsulation of the ECDH context
and work around the problem.
The functionality from public API functions are moved to
`xxx_internal()` functions. The public API functions are modified to do
basic parameter validation and dispatch the call to the right
implementation.
There is no intended change in behaviour when
`MBEDTLS_ECDH_LEGACY_CONTEXT` is enabled.
In the future we want to support alternative ECDH implementations. We
can't make assumptions about the structure of the context they might
use, and therefore shouldn't access the members of
`mbedtls_ecdh_context`.
Currently the lifecycle of the context can't be done without direct
manipulation. This commit adds `mbedtls_ecdh_setup()` to complete
covering the context lifecycle with functions.
`mbedtls_ecp_tls_read_group()` both parses the group ID and loads the
group into the structure provided. We want to support alternative
implementations of ECDH in the future and for that we need to parse the
group ID without populating an `mbedtls_ecp_group` structure (because
alternative implementations might not use that).
This commit moves the part that parses the group ID to a new function.
There is no need to test the new function directly, because the tests
for `mbedtls_ecp_tls_read_group()` are already implicitly testing it.
There is no intended change in behaviour in this commit.
Refactor `mpi_write_hlp()` to not be recursive, to fix stack overflows.
Iterate over the `mbedtls_mpi` division of the radix requested,
until it is zero. Each iteration, put the residue in the next LSB
of the output buffer. Fixes#2190
Refactor mbedtls_ctr_drbg_update_seed_file and
mbedtls_hmac_drbg_update_seed_file to make the error logic clearer.
The new code does not use fseek, so it works with non-seekable files.
In mbedtls_mpi_write_binary, avoid leaking the size of the number
through timing or branches, if possible. More precisely, if the number
fits in the output buffer based on its allocated size, the new code's
trace doesn't depend on the value of the number.
Deprecate the module-specific XXX_HW_ACCEL_FAILED and
XXX_FEATURE_UNAVAILABLE errors, as alternative implementations should now
return `MBEDTLS_ERR_PLATFORM_HW_FAILED` and
`MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`.
ssl_write_handshake_msg() includes the assertion that
`ssl->handshake != NULL` when handling a record which is
(a) a handshake message, and NOT
(b) a HelloRequest.
However, it later calls `ssl_append_flight()` for any
record different from a HelloRequest handshake record,
that is, records satisfying !(a) || !(b), instead of
(a) && !(b) as covered by the assertion (specifically,
CCS or Alert records).
Since `ssl_append_flight()` assumes that `ssl->handshake != NULL`,
this rightfully triggers static analyzer warnings.
This commit expands the scope of the assertion to check
that `ssl->handshake != NULL` for any record which is not
a HelloRequest.
Revert changes for checking whether `MBEDTLS_ECP_RESTARTABLE`
is defined, since it broke the CI. The context is used whether the
restartable feature is defined or not.
1. Checge to check for `MBEDTLS_ECP_RESTARTABLE` for all definitions
of `rs_ctx`.
2. Remove checks for `_ALT` when using `rs_ctx` as they cannot coexist
with the Restartable configuration.
Previously, when checking whether a CRT was revoked through
one of the configured CRLs, the library would only consider
those CRLs whose `issuer` field binary-matches the `subject`
field of the CA that has issued the CRT in question. If those
fields were not binary equivalent, the corresponding CRL was
discarded.
This is not in line with RFC 5280, which demands that the
comparison should be format- and case-insensitive. For example:
- If the same string is once encoded as a `PrintableString` and
another time as a `UTF8String`, they should compare equal.
- If two strings differ only in their choice of upper and lower case
letters, they should compare equal.
This commit fixes this by using the dedicated x509_name_cmp()
function to compare the CRL issuer with the CA subject.
Fixes#1784.
library/certs.c provides some hardcoded certificates that
are used e.g. by the test applications ssl_server2, ssl_client2
in case no certificates are provided on the command line.
The certificates used are from the tests/data_files folder
and have been updated in the latest commits. This commit
updates their copies in certs.c. It also adds comments
indicating the files from which the data is taken, in
order to ease update in the future.
Previous commits have added or modified build instructions for
server1*, server2*, server5*, test-ca*, cli-rsa* in the Makefile
tests/data_files/Makefile, or the apps they invoke have been changed.
This commit regenerates those files to make sure they are in match with
the build instructions.
Previously, when checking whether a CRT was revoked through
one of the configured CRLs, the library would only consider
those CRLs whose `issuer` field binary-matches the `subject`
field of the CA that has issued the CRT in question. If those
fields were not binary equivalent, the corresponding CRL was
discarded.
This is not in line with RFC 5280, which demands that the
comparison should be format- and case-insensitive. For example:
- If the same string is once encoded as a `PrintableString` and
another time as a `UTF8String`, they should compare equal.
- If two strings differ only in their choice of upper and lower case
letters, they should compare equal.
This commit fixes this by using the dedicated x509_name_cmp()
function to compare the CRL issuer with the CA subject.
Fixes#1784.
Return the condition compilation flags surrounding
`mbedtls_ecdh_compute_shared()`, `mbedtls_ecdh_gen_public()`,
`mbedtls_ecdsa_sign()` and `mbedtls_ecdsa_verify()` that were accidentally
removed in a previous merge.
Resolves#2163
This commit modifies a bounds check in `mbedtls_ecp_check_budget()` to
be correct even if the requested number of ECC operations would overflow
the operation counter.
Use `( x >> y ) & z` instead of `x >> y & z`. Both are equivalent
by operator precedence, but the former is more readable and the
commonly used idiom in the library.
Correct a typo in an AES XTS implementation comment where the relevant
NIST standard was incorrectly referred to as NIST 80-38E instead of NIST
800-38E.
It is inaccurate to call a data unit a "sector". A disk sector is a
common use case for the data unit, but there exist other types of data
units that are not sectors.
This commit fixes issue #1212 related to platform-specific entropy
polling in an syscall-emulated environment.
Previously, the implementation of the entropy gathering function
`mbedtls_platform_entropy_poll()` for linux machines used the
following logic to determine how to obtain entropy from the kernel:
1. If the getrandom() system call identifier SYS_getrandom is present and
the kernel version is 3.17 or higher, use syscall( SYS_getrandom, ... )
2. Otherwise, fall back to reading from /dev/random.
There are two issues with this:
1. Portability:
When cross-compiling the code for a different
architecture and running it through system call
emulation in qemu, qemu reports the host kernel
version through uname but, as of v.2.5.0,
doesn't support emulating the getrandom() syscall.
This leads to `mbedtls_platform_entropy_poll()`
failing even though reading from /dev/random would
have worked.
2. Style:
Extracting the linux kernel version from
the output of `uname` is slightly tedious.
This commit fixes both by implementing the suggestion in #1212:
- It removes the kernel-version detection through uname().
- Instead, it checks whether `syscall( SYS_getrandom, ... )`
fails with errno set to ENOSYS indicating an unknown system call.
If so, it falls through to trying to read from /dev/random.
Fixes#1212.
This commit changes the behavior of the record decryption routine
`ssl_decrypt_buf()` in the following situation:
1. A CBC ciphersuite with Encrypt-then-MAC is used.
2. A record with valid MAC but invalid CBC padding is received.
In this situation, the previous code would not raise and error but
instead forward the decrypted packet, including the wrong padding,
to the user.
This commit changes this behavior to return the error
MBEDTLS_ERR_SSL_INVALID_MAC instead.
While erroneous, the previous behavior does not constitute a
security flaw since it can only happen for properly authenticated
records, that is, if the peer makes a mistake while preparing the
padded plaintext.
This commit duplicates the public function mbedtls_asn1_find_named_data()
defined in library/asn1parse.c within library/asn1write.c in order to
avoid a dependency of the ASN.1 writing module on the ASN.1 parsing module.
The duplication is unproblematic from a semantic and an efficiency
perspective becasue it is just a short list traversal that doesn't
actually do any ASN.1 parsing.
Previously, mbedtls_pkcs5_pbes2() was unconditionally declared
in `pkcs5.h` but defined as a stub returning
`MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE` in case
MBEDTLS_ASN1_PARSE_C was not defined.
In line with the previous commits, this commit removes declaration
and definition from both `pkcs5.h` and `pkcs5.c` in case
MBEDTLS_ASN1_PARSE_C is not defined.
Our API makes no guarantee that you can use a context after free()ing it
without re-init()ing it first, so better not give the wrong impression that we
do, while it's not policy and the rest of the code might not allow it.
Rename the PLATFORM HW error, to avoid ABI breakage with Mbed OS.
The value changed as well, as previous value was not in the range of
Mbed TLS low level error codes.
* development:
ssl-opt.sh: change expected output for large srv packet test with SSLv3
Adapt ChangeLog
Fix bug in SSL ticket implementation removing keys of age < 1s
ssl-opt.sh: Add DTLS session resumption tests
Add ChangeLog entry
Fix typo
Fix hmac_drbg failure in benchmark, with threading
Remove trailing whitespace
Remove trailing whitespace
ssl_server2: add buffer overhead for a termination character
Add missing large and small packet tests for ssl_server2
Added buffer_size and response_size options for ssl-server2. Added appropriate tests.
Solving a conflict in tests/ssl-opt.sh: two set of tests were added at the
same place (just after large packets):
- restartable ECC tests (in this branch)
- server-side large packets (in development)
Resolution was to move the ECC tests after the newly added server large packet
ones.
