In an X.509 certificate, a basicConstraints extension where the cA
field (BOOLEAN) is omitted but the pathLenConstraint field (INTEGER )is
present is syntactically valid, but RFC 5280 states that CAs MUST NOT emit
such a certificate.
Historically, since XySSL 0.9 in 2008, if a basicConstraints extension
started with an INTEGER field, we interpreted that field as a cA value. This
was a deliberate change:
> Fixed x509_get_ext() to accept some rare certificates (like
> www.openssl.org:443) which have an INTEGER instead of a BOOLEAN for
> Extension::BasicConstraints::cA.
However, 18 years later, this does not seem to be relevant, and the details
seem to have been lost. Furthermore, major implementations follow RFC 5280
and interpret `SEQUENCE { INTEGER n }` with cA being set to its default
value (FALSE) and pathLenConstraint set to `n`. This means that such a
certificate with n != 0 would be interpreted as a CA certificate in Mbed TLS
but as a leaf certificate elsewhere, which is dangerous.
Since CAs are not supposed to emit such certificates, stop accepting them
altogether.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
It is now unreachable because:
- end_set == end_attr_data is enforced at library/x509_csr.c:236.
- end_exts == end_set is enforced at library/x509_csr.c:248.
- x509_csr_parse_extensions() only returns success if *p == end_exts, enforced
at library/x509_csr.c:188.
So after a successful x509_csr_parse_extensions() call, *p == end_exts ==
end_set == end_attr_data. The condition at line 257 cannot be true.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Don't allow parts of a compound (SEQUENCE or SET) to go beyond the
containing compound.
The inline CSR regression test is derived from the existing "X509 CSR
ASN.1 (OK)" DER test vector. In the extensionRequest attribute, the original
well-formed fragment is:
a029 3027 06092a864886f70d01090e 311a 3018 ...
where 31 1a is the SET containing the Extensions sequence and 30 18 is the
contained Extensions sequence. The malformed test changes only the SET
length byte, from 1a to 19:
a029 3027 06092a864886f70d01090e 3119 3018 ...
The attribute SEQUENCE length and the inner Extensions sequence length are
left unchanged. This makes the SET one byte too short for its containing
attribute, so the parser must reject it with
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH instead of parsing the trailing byte as
data outside the SET.
There is no known security impact, just some risk reduction.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When parsing the basicConstraints extension, reject junk after the SEQUENCE
inside the extension (which is probably benign), and reject a SEQUENCE that
extends beyond the extension (could be very dangerous).
Add a non-regression test where a certificate that is technically malformed,
but accepted as a leaf certificate by OpenSSL and other X.509
implementations, to be accepted as a CA certificate by Mbed TLS.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is required because if the user defined configuration file (not the
default one provided by mbedtls) includes files from the standard
C library then __STDC_WANT_LIB_EXT1__ won't be defined there which
cause weird build failures.
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
When MBEDTLS_SSL_PROTO_TLS1_3 is enabled ensure that at least one of the
related key exchanges is also enabled.
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Reject HRR selected_group unless it matches the client’s original
supported_groups and is locally supported, so unadvertised groups are not
accepted in the second ClientHello.
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
For each received ClientHello fragment, check
that its epoch is zero and update the
record-level sequence number.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
ECDHE-RSA only requires RSA signature, not encryption. This commits fixes
guards in "mbedtls_check_config.h".
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Follow the same pattern that was used in the past to remove dependency
on MBEDTLS_RSA_C and use PSA_WANT instead.
Relying on MBEDTLS_RSA_C is fine only when builtin drivers are compiled
since all PSA_WANT are converted to legacy build symbols. However when
builtin drivers are not built (ex: in case of TF-M), then part of the code
in TLS/X509 won't be compiled because MBEDTLS_RSA_C is not set. OTOH
it's not possible to declare that symbol in a configuration file because
it's a legacy one and it will be rejected by buildtime checks.
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
Simplify and improve error reporting and remove unnecessary
initialisation (the caller is responsible for initialising those
values).
Signed-off-by: Janos Follath <janos.follath@arm.com>
The caller is returning MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER if
ssl_parse_signature_algorithm() fails, but
ssl_parse_signature_algorithm() returns
MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE upon failure. There is no good reason
for this mismatch and better to be aligned.
Signed-off-by: Janos Follath <janos.follath@arm.com>
After the recent refactoring ssl_parse_signature_algorithm() sends an
alert on failure, but the caller also sends an alert on failure. Sending
two alerts is at least a protocol violation, and might not leave the SSL
context in a good state.
It is simpler to have the caller read the two bytes, and pass them to
this function.
Signed-off-by: Janos Follath <janos.follath@arm.com>
The logic was easier to follow before 693a47a, which removed the
ssl_parse_signature_algorithm function and introduced the bug being
fixed in this PR.
When validating multiple conditions, it's easier to read, easier to
debug and, as we can see, easier to get right if you validate them
separately.
Signed-off-by: Janos Follath <janos.follath@arm.com>
This bug caused the client accepting sig_algs used by the server that
it explicitly wanted to disallow.
Signed-off-by: Janos Follath <janos.follath@arm.com>
Replace MBEDTLS_ECP_C with PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY which is
already used in all other profiles in this file.
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
When we are doing PSK, we'd like to set verify_result to
MBEDTLS_X509_BADCERT_SKIP_VERIFY. Previously this was done in
mbedtls_ssl_set_hs_psk() but this is inadequate since this function may
be called for early data (where certificate verification happens later
in the handshake.
Instead, set this value after writing / processing the encrypted
extensions on the server / client respectively, so that we know whether
we are doing certificate verification or not for sure. This change is
effective only for TLS 1.3 as TLS 1.2 sets verify_result for PSK in
ssl_parse_certificate_coordinate().
Signed-off-by: David Horstmann <david.horstmann@arm.com>
The check was wrongly removed by the commit
"ssl_tls12_server.c: Move ClientHello message_seq adjustment".
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Move back the digest update just after
the call to mbedtls_ssl_read_record().
It fits well here as we explain in the
comment associated to the call to
mbedtls_ssl_read_record() that we
update it manually.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Define `_POSIX_C_SOURCE` and `_XOPEN_SOURCE` in a single place that
applies everywhere, to make things simple.
This may break some platforms that require special handling for POSIX
functions and types. Subsequent commits will add platform-specific hacks
as needed.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
We were using slightly different guards to decide whether to include
`<unistd.h>` in different places. Unify those.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
On MingW, temporarily force the use of the standard versions of `snprintf()`
and `vsnprintf()` (since we set `__USE_MINGW_ANSI_STDIO` in
`mbedtls_platform_requirements.h`). Do not honor `platform.h` configuration,
because with the current TF-PSA-Crypto, `MBEDTLS_PLATFORM_VSNPRINTF_ALT and
`MBEDTLS_PLATFORM_SNPRINTF_ALT` are always enabled on MinGW, so what matters
is the setting of `__USE_MINGW_ANSI_STDIO` when `platform.c` is built, and
until https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/694, the legacy printf
functions are used there.
Revert this commit once the `tf-psa-crypto` module is updated with the merge
of https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/694.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Always activate `__USE_MINGW_ANSI_STDIO` unless overridden on the command
line. This is necessary with older versions of MingW and/or Windows,
where snprintf does not always zero-terminate the buffer, and does
not support formats such as `"%zu"` for size_t and `"%lld"` for long long.
Simplify debug.h accordingly. The macros `MBEDTLS_PRINTF_SIZET`,
`MBEDTLS_PRINTF_SIZET_HAX` and `MBEDTLS_PRINTF_LONGLONG` are no longer
needed, but they are still used in our code base and must stay in debug.h
for backward compatibility.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In `test_suite_debug`, test `mbedtls_debug_snprintf()`, which uses
`mbedtls_vsnprintf()` like `mbedtls_debug_print_msg()`. Do this instead of
testing `mbedtls_snprintf()`, which might be subtly different (older
Windows runtimes had slightly different behavior for vsnprintf() vs
snprintf(); TF-PSA-Crypto might pick up a different function if the
platform configuration is different in TF-PSA-Crypto and Mbed TLS).
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
On some platforms, the system headers expose different interfaces depending
on what macros are defined, for example to provide different standards
compliance level. Create a common place where we can declare such macros,
so that our code can behave in the same way when it's in different files.
Individual .c files can still override these requirements by defining
macros before including the common header, if it's really necessary.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
We already have `x509_internal.h` which is common to all parts of the X.509
library, and `ssl_misc.h` which is common to all parts of the TLS library.
Also create `mbedtls_common.h` which is for the Mbed TLS project as a whole.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In mbedtls_x509_string_to_names() we were not checking for allocation
failures. An allocation failure would lead to a memcpy() to a null
pointer address.
Fix this by checking the result of the call to mbedtls_calloc() and
returning MBEDTLS_ERR_X509_ALLOC_FAILED in the error case.
Signed-off-by: David Horstmann <david.horstmann@arm.com>
In ssl_tls12_server.c:ssl_parse_client_hello(), remove
the code that directly reads the received data to read
the record expected to contain the ClientHello message.
The function already supported handling a ClientHello
read via mbedtls_ssl_read_record() in the following
cases:
- when the ClientHello was read as a post-handshake
message (renegotiation).
- when the ClientHello was read by
ssl_tls13_process_client_hello() during TLS 1.3 or
TLS 1.2 version negotiation.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
