The previous code performed architectural maximum record length checks
both before and after record decryption. Since MBEDTLS_SSL_IN_CONTENT_LEN
bounds the maximum length of the record plaintext, it suffices to check
only once after (potential) decryption.
This must not be confused with the internal check that the record
length is small enough to make the record fit into the internal input
buffer; this is done in mbedtls_ssl_fetch_input().
The check is in terms of the internal input buffer length and is
hence likely to be originally intended to protect against overflow
of the input buffer when fetching data from the underlying
transport in mbedtls_ssl_fetch_input(). For locality of reasoning,
it's better to perform such a check close to where it's needed,
and in fact, mbedtls_ssl_fetch_input() _does_ contain an equivalent
bounds check, too, rendering the bounds check in question redundant.
When looking for a parent, all candidates were considered time-invalid due to
the #ifdef incorrectly including the `parent_valid = 1` line.
When MBEDTLS_HAVE_TIME_DATE is unset the time-validity of certificates is
never checked and always treated as valid. This is usually achieved by proper
usage of mbedtls_x509_time_is_past() and mbedtls_x509_time_is_future() (and
their definition when we don't HAVE_TIME_DATE).
Here the calls to these functions needs to be guarded by
MBEDTLS_X509_CRT_REMOVE_TIME as they access struct members whose presence is
controlled by this option. But the "valid" branch should still always be taken.
(Note: MBEDTLS_X509_CRT_REMOVE_TIME being set forces MBEDTLS_HAVE_TIME_DATE to
be unset, as enforce by check_config.h.)
This bug was found by `all.sh test_baremetal` - no need for a new test.
This was found as a warning when running scripts/baremetal.sh --ram
--build-only manually, but it should have been found in a more automated way.
Adding -Werror so that future such issues will be caught by all.sh
(component_test_baremetal already invokes baremetal.sh --ram --build-only).
Asserting `*p == end` right after setting `end = *p + len` will always fail
unless `len == 0`, which is never the case with properly-formed certificates.
The function x509_skip_dates() is modelled after x509_get_dates() which between
setting `end` and comparing it to `*p` calls mbedtls_x509_get_time() which
advances `*p` to the expected value, which is why this test works in
get_dates().
Since `skip_dates()` has `skip`, not `validate` in its name, and the entire
point of `MBEDTLS_X509_CRT_REMOVE_TIME` is to save code, we don't want to
call the relatively large functions needed to properly parse (and validate)
dates before throwing the parsed dates away, we can just fast-forward to the
end of the sequence.
This makes updating `end` and comparing it to `*p` after the fast-forward
redundant, as the comparison will always be true (unlike the case where we
actually parse the contents of the sequence).
This bug was found by `all.sh test_baremetal` - no need for a new test.
This option builds the library, tests and example programs
in a minimally modified baremetal.h configuration (modifications
from baremetal_test.h) but doesn't execute any tests.
Breaking into a series of statements makes things easier when stepping through
the code in a debugger.
Previous comments we stating the opposite or what the code tested for (what we
want vs what we're erroring out on) which was confusing.
Also expand a bit on the reasons for these restrictions.
ssl_get_next_record() may pend fatal alerts in response to receiving
invalid records. Previously, however, those were never actually sent
because there was no code-path checking for pending alerts.
This commit adds a call to ssl_send_pending_fatal_alert() after
the invocation of ssl_get_next_record() to fix this.
This function is often called when there's already an error code to handle,
and one of the reasons to introduce the pending of alerts was to _not_ have
another potential error code to take care of. Reflect this by making `void`
the return type of `mbedtls_ssl_pend_fatal_alert()`.
