Announce the security fix together with the overflow fix, since the two bugs
are pretty much indistinguishable from a black-box perspective, despite
being due to independent problems in the code.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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>
In test cases for pathLenConstraint parsing in a basicConstraints extension
of an X.509 certificate, use the standard type for the leading cA field,
namely BOOLEAN. The test data was encoding that field as INTEGER, which was
accepted as a deviation from the standard since XySSL 0.9.
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>
Since __packed is a reserved keyword for IAR compilers, and
Zephyr defines it to attribute(__packed__), some typedef constructs
in mbedtls does not work with attribute(packed), only with the
keyword packed.
This fix temporary undefs the macro and restores it after the typedefs.
Signed-off-by: Lars-Ove Karlsson <lars-ove.karlsson@iar.com>
Narrow TLS 1.2 RSA-PSS handling to the client ServerKeyExchange parse path and add OpenSSL and GnuTLS interoperability tests.
Signed-off-by: Viktor Sokolovskiy <maokaman@gmail.com>
Cast from 'unsigned char *' to 'memory_header *' through an
intermediate 'void *' to suppress -Wcast-align warnings.
Some Clang-based toolchains (e.g. MetaWare/ARC) enable -Wcast-align
as part of -Wall, unlike standard Clang on x86/ARM. Combined with
-Werror this turns the casts into fatal build errors.
The casts are already alignment-safe at runtime:
- In mbedtls_memory_buffer_alloc_init(), buf is explicitly aligned
to MBEDTLS_MEMORY_ALIGN_MULTIPLE before the cast.
- In buffer_alloc_calloc(), p is computed from an aligned base plus
aligned offsets (sizeof(memory_header) and len are both multiples
of MBEDTLS_MEMORY_ALIGN_MULTIPLE).
- In buffer_alloc_free(), p is derived from a previously aligned
allocation pointer minus the aligned header size.
Signed-off-by: Mohamed Moawad <moawad@synopsys.com>
Namespace BEFORE_COLON and BC defines by prepending MBEDTLS_
and expanding BC to BEFORE_COLON_STR. This is to avoid naming
conflicts with third-party code. No functional change.
Signed-off-by: Stefan Gloor <stefan.gloor@siemens.com>
BEFORE_COLON and BC defines with the accompanying comment are only
required in x509_crt and x509_csr, but not used in x509_crl.c.
Signed-off-by: Stefan Gloor <stefan.gloor@siemens.com>
A bug there was fixed in a previous commit, but without complete tests
or a ChangeLog entry. This commit completes the fix.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Don't call a macro that does `goto exit` on failure after the `exit:` label:
that would cause an infinite loop if something does go wrong.
Generally, cleanup functions don't error out, so it is unlikely to be a
problem in practice. If an error does happen during cleanup, it's probably
due to memory corruption caused by a bug that happened earlier, and that is
likely to have been detected in an earlier function. So we don't really need
to assert the return code of functions called during cleanup, and normally
we don't. Only a few places did so, wrongly.
I found the problematic places with
```
ag 'exit:[^}]*(PSA_ASSERT|TEST_ASSERT|TEST_EQUAL)' tests/suites/*.function
```
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In a PAKE operation that has been initialized with `PSA_PAKE_OPERATION_INIT`
or `psa_pake_operation_init()`, the content of the driver-specific part is
indeterminate. It is actually all-bits-zero on most platforms, but not all,
e.g. not with GCC 15 or CompCert. So don't assert anything about it.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This directory is currently excluded from `check-python-files.sh`, because
we run it on the CI in an old Python version that doesn't support some of
our new maintainer scripts.
There are no such scripts in mbedtls for now (only in TF-PSA-Crypto), but be
ready if we want to add some.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When initializing a PAKE operation structure, use an
auxiliary function that doesn't initialize union members to all-bits-zero.
Context: on most compilers, initializing a union to `{0}` initializes it to
all bits zero; but on some compilers, the trailing part of members other
than the first is left uninitialized. This way, we can run the tests on any
platform and validate that the code would work correctly on platforms where
union initialization is short, such as GCC 15.
This commit extends 93dd99571b46f012a462c7a2494ccfe9ec0603a1 to
`test_suite_psa_crypto_pake.function`.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Add a couple of missing ifdefs to avoid having unreachable code with
AT_LEAST_ONE_BUILTIN_KDF not defined, which otherwise causes a build
warning with clang.
Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
The documentation says "or another error code", so it's OK to change the
error code returned here for a more correct one.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
