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
Fixes memory leak in mpi_miller_rabin() that occurs when the function has
failed to obtain a usable random 'A' 30 turns in a row.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
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.
