In case of argument change, freeing everything is not the most efficient
(wastes one free()+calloc()) but makes the code simpler, which is probably
more important here
We'll need to store MPIs and other things that allocate memory in this
context, so we need a place to free it. We can't rely on doing it before
returning from ecp_mul() as we might return MBEDTLS_ERR_ECP_IN_PROGRESS (thus
preserving the context) and never be called again (for example, TLS handshake
aborted for another reason). So, ecp_group_free() looks like a good place to
do this, if the restart context is part of struct ecp_group.
This means it's not possible to use the same ecp_group structure in different
threads concurrently, but:
- that's already the case (and documented) for other reasons
- this feature is precisely intended for environments that lack threading
An alternative option would be for the caller to have to allocate/free the
restart context and pass it explicitly, but this means creating new functions
that take a context argument, and putting a burden on the user.
The plan is to count basic operations as follows:
- call to ecp_add_mixed() -> 11
- call to ecp_double_jac() -> 8
- call to mpi_mul_mpi() -> 1
- call to mpi_inv_mod() -> 120
- everything else -> not counted
The counts for ecp_add_mixed() and ecp_double_jac() are based on the actual
number of calls to mpi_mul_mpi() they they make.
The count for mpi_inv_mod() is based on timing measurements on K64F and
LPC1768 boards, and are consistent with the usual very rough estimate of one
inversion = 100 multiplications. It could be useful to repeat that measurement
on a Cortex-M0 board as those have smaller divider and multipliers, so the
result could be a bit different but should be the same order of magnitude.
The documented limitation of 120 basic ops is due to the calls to mpi_inv_mod()
which are currently not interruptible nor planned to be so far.
This is the first step towards making verify_chain() iterative. While from a
readability point of view the current recursive version is fine, one of the
goals of this refactoring is to prepare for restartable ECC integration, which
will need the explicit stack anyway.
Besides avoiding near-duplication, this avoids having three generations of
certificate (child, parent, grandparent) in one function, with all the
off-by-one opportunities that come with it.
This also allows to simplify the signature of verify_child(), which will be
done in next commit.
This is from the morally 5th (and soon obsolete) invocation of this function
in verify_top().
Doing this badtime-skipping when we search for a parent in the provided chain
is a change of behaviour, but it's backwards-compatible: it can only cause us
to accept valid chains that we used to reject before. Eg if the peer has a
chain with two version of an intermediate certificate with different validity
periods, the first non valid and the second valid - such cases are probably
rare or users would have complained already, but it doesn't hurt to handle it
properly as it allows for more uniform code.
This may look like a behaviour change because one check has been added to the
function that was previously done in only one of the 3 call sites. However it
is not, because:
- for the 2 call sites in verify(), the test always succeeds as path_cnt is 0.
- for the call site in verify_child(), the same test was done later anyway in
verify_top()
There are 3 instance that were replaced, but 2 instances of variants of this
function exist and will be handled next (the extra parameter that isn't used
so far is in preparation for that):
- one in verify_child() where path_cnt constraint is handled too
- one in verify_top() where there is extra logic to skip parents that are
expired or future, but only if there are better parents to be found
This is a slight change of behaviour in that the previous condition was:
- same subject
- signature matches
while the new condition is:
- exact same certificate
However the documentation for mbedtls_x509_crt_verify() (note on trust_ca)
mentions the new condition, so code that respected the documentation will keep
working.
In addition, this is a bit faster as it doesn't check the self-signature
(which never needs to be checked for certs in the trusted list).
When we're looking for a parent, in trusted CAs, 'top' should be 1.
This only impacted which call site for verify_top() was chosen, and the error
was then fixed inside verify_top() by iterating over CAs again, this time
correctly setting 'top' to 1.
