When parsing a certificate with the basic constraints extension
the max_pathlen that was read from it was incremented regardless
of its value. However, if the max_pathlen is equal to INT_MAX (which
is highly unlikely), an undefined behaviour would occur.
This commit adds a check to ensure that such value is not accepted
as valid. Relevant tests for INT_MAX and INT_MAX-1 are also introduced.
Certificates added in this commit were generated using the
test_suite_x509write, function test_x509_crt_check. Input data taken
from the "Certificate write check Server1 SHA1" test case, so the generated
files are like the "server1.crt", but with the "is_ca" field set to 1 and
max_pathlen as described by the file name.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Signed-off-by: Piotr Nowicki <piotr.nowicki@arm.com>
* origin/pr/2481:
Document support for MD2 and MD4 in programs/x509/cert_write
Correct name of X.509 parsing test for well-formed, ill-signed CRT
Add test cases exercising successful verification of MD2/MD4/MD5 CRT
Add test case exercising verification of valid MD2 CRT
Add MD[245] test CRTs to tree
Add instructions for MD[245] test CRTs to tests/data_files/Makefile
Add suppport for MD2 to CSR and CRT writing example programs
Convert further x509parse tests to use lower-case hex data
Correct placement of ChangeLog entry
Adapt ChangeLog
Use SHA-256 instead of MD2 in X.509 CRT parsing tests
Consistently use lower case hex data in X.509 parsing tests
Some functions within the X.509 module return an ASN.1 low level
error code where instead this error code should be wrapped by a
high-level X.509 error code as in the bulk of the module.
Specifically, the following functions are affected:
- mbedtls_x509_get_ext()
- x509_get_version()
- x509_get_uid()
This commit modifies these functions to always return an
X.509 high level error code.
Care has to be taken when adapting `mbetls_x509_get_ext()`:
Currently, the callers `mbedtls_x509_crt_ext()` treat the
return code `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG` specially to
gracefully detect and continue if the extension structure is not
present. Wrapping the ASN.1 error with
`MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and adapting the check
accordingly would mean that an unexpected tag somewhere
down the extension parsing would be ignored by the caller.
The way out of this is the following: Luckily, the extension
structure is always the last field in the surrounding structure,
so if there is some data remaining, it must be an Extension
structure, so we don't need to deal with a tag mismatch gracefully
in the first place.
We may therefore wrap the return code from the initial call to
`mbedtls_asn1_get_tag()` in `mbedtls_x509_get_ext()` by
`MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and simply remove
the special treatment of `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG`
in the callers `x509_crl_get_ext()` and `x509_crt_get_ext()`.
This renders `mbedtls_x509_get_ext()` unsuitable if it ever
happened that an Extension structure is optional and does not
occur at the end of its surrounding structure, but for CRTs
and CRLs, it's fine.
The following tests need to be adapted:
- "TBSCertificate v3, issuerID wrong tag"
The issuerID is optional, so if we look for its presence
but find a different tag, we silently continue and try
parsing the subjectID, and then the extensions. The tag '00'
used in this test doesn't match either of these, and the
previous code would hence return LENGTH_MISMATCH after
unsucessfully trying issuerID, subjectID and Extensions.
With the new code, any data remaining after issuerID and
subjectID _must_ be Extension data, so we fail with
UNEXPECTED_TAG when trying to parse the Extension data.
- "TBSCertificate v3, UIDs, invalid length"
The test hardcodes the expectation of
MBEDTLS_ERR_ASN1_INVALID_LENGTH, which needs to be
wrapped in MBEDTLS_ERR_X509_INVALID_FORMAT now.
Fixes#2431.
When parsing a substructure of an ASN.1 structure, no field within
the substructure must exceed the bounds of the substructure.
Concretely, the `end` pointer passed to the ASN.1 parsing routines
must be updated to point to the end of the substructure while parsing
the latter.
This was previously not the case for the routines
- x509_get_attr_type_and_value(),
- mbedtls_x509_get_crt_ext(),
- mbedtls_x509_get_crl_ext().
These functions kept using the end of the parent structure as the
`end` pointer and would hence allow substructure fields to cross
the substructure boundary. This could lead to successful parsing
of ill-formed X.509 CRTs.
This commit fixes this.
Care has to be taken when adapting `mbedtls_x509_get_crt_ext()`
and `mbedtls_x509_get_crl_ext()`, as the underlying function
`mbedtls_x509_get_ext()` returns `0` if no extensions are present
but doesn't set the variable which holds the bounds of the Extensions
structure in case the latter is present. This commit addresses
this by returning early from `mbedtls_x509_get_crt_ext()` and
`mbedtls_x509_get_crl_ext()` if parsing has reached the end of
the input buffer.
The following X.509 parsing tests need to be adapted:
- "TBSCertificate, issuer two inner set datas"
This test exercises the X.509 CRT parser with a Subject name
which has two empty `AttributeTypeAndValue` structures.
This is supposed to fail with `MBEDTLS_ERR_ASN1_OUT_OF_DATA`
because the parser should attempt to parse the first structure
and fail because of a lack of data. Previously, it failed to
obey the (0-length) bounds of the first AttributeTypeAndValue
structure and would try to interpret the beginning of the second
AttributeTypeAndValue structure as the first field of the first
AttributeTypeAndValue structure, returning an UNEXPECTED_TAG error.
- "TBSCertificate, issuer, no full following string"
This test exercises the parser's behaviour on an AttributeTypeAndValue
structure which contains more data than expected; it should therefore
fail with MBEDTLS_ERR_ASN1_LENGTH_MISMATCH. Because of the missing bounds
check, it previously failed with UNEXPECTED_TAG because it interpreted
the remaining byte in the first AttributeTypeAndValue structure as the
first byte in the second AttributeTypeAndValue structure.
- "SubjectAltName repeated"
This test should exercise two SubjectAltNames extensions in succession,
but a wrong length values makes the second SubjectAltNames extension appear
outside of the Extensions structure. With the new bounds in place, this
therefore fails with a LENGTH_MISMATCH error. This commit adapts the test
data to put the 2nd SubjectAltNames extension inside the Extensions
structure, too.
The X.509 parsing test suite test_suite_x509parse contains a test
exercising X.509 verification for a valid MD4/MD5 certificate in a
profile which doesn't allow MD4/MD5. This commit adds an analogous
test for MD2.
- Replace 'RSA with MD2' OID '2a864886f70d010102' by
'RSA with SHA-256' OID '2a864886f70d01010b':
Only the last byte determines the hash, and
`MBEDTLS_OID_PKCS1_MD2 == MBEDTLS_OID_PKCS1 "\x02"`
`MBEDTLS_OID_PKCS1_SHA256 == MBEDTLS_OID_PKCS1 "\x0b"`
See oid.h.
- Replace MD2 dependency by SHA256 dependency.
- Adapt expected CRT info output.
This commit introduces variants test-ca_utf8.crt,
test-ca_printablestring.crt and test-ca_uppercase.crt
of tests/data_files/test-ca.crt which differ from
test-ca.crt in their choice of string encoding and
upper and lower case letters in the DN field. These
changes should be immaterial to the recovation check,
and three tests are added that crl.pem, which applies
to test-ca.crt, is also considered as applying to
test-ca_*.crt.
Summary of merge conflicts:
include/mbedtls/ecdh.h -> documentation style
include/mbedtls/ecdsa.h -> documentation style
include/mbedtls/ecp.h -> alt style, new error codes, documentation style
include/mbedtls/error.h -> new error codes
library/error.c -> new error codes (generated anyway)
library/ecp.c:
- code of an extracted function was changed
library/ssl_cli.c:
- code addition on one side near code change on the other side
(ciphersuite validation)
library/x509_crt.c -> various things
- top fo file: helper structure added near old zeroize removed
- documentation of find_parent_in()'s signature: improved on one side,
added arguments on the other side
- documentation of find_parent()'s signature: same as above
- verify_chain(): variables initialised later to give compiler an
opportunity to warn us if not initialised on a code path
- find_parent(): funcion structure completely changed, for some reason git
tried to insert a paragraph of the old structure...
- merge_flags_with_cb(): data structure changed, one line was fixed with a
cast to keep MSVC happy, this cast is already in the new version
- in verify_restratable(): adjacent independent changes (function
signature on one line, variable type on the next)
programs/ssl/ssl_client2.c:
- testing for IN_PROGRESS return code near idle() (event-driven):
don't wait for data in the the socket if ECP_IN_PROGRESS
tests/data_files/Makefile: adjacent independent additions
tests/suites/test_suite_ecdsa.data: adjacent independent additions
tests/suites/test_suite_x509parse.data: adjacent independent additions
* development: (1059 commits)
Change symlink to hardlink to avoid permission issues
Fix out-of-tree testing symlinks on Windows
Updated version number to 2.10.0 for release
Add a disabled CMAC define in the no-entropy configuration
Adapt the ARIA test cases for new ECB function
Fix file permissions for ssl.h
Add ChangeLog entry for PR#1651
Fix MicroBlaze register typo.
Fix typo in doc and copy missing warning
Fix edit mistake in cipher_wrap.c
Update CTR doc for the 64-bit block cipher
Update CTR doc for other 128-bit block ciphers
Slightly tune ARIA CTR documentation
Remove double declaration of mbedtls_ssl_list_ciphersuites
Update CTR documentation
Use zeroize function from new platform_util
Move to new header style for ALT implementations
Add ifdef for selftest in header file
Fix typo in comments
Use more appropriate type for local variable
...
Conflict resolution:
* ChangeLog
* tests/data_files/Makefile: concurrent additions, order irrelevant
* tests/data_files/test-ca.opensslconf: concurrent additions, order irrelevant
* tests/scripts/all.sh: one comment change conflicted with a code
addition. In addition some of the additions in the
iotssl-1381-x509-verify-refactor-restricted branch need support for
keep-going mode, this will be added in a subsequent commit.
The 'critical' boolean can be set to false in two ways:
- by leaving it implicit (test data generated by openssl)
- by explicitly setting it to false (generated by hand)
This covers all lines added in the previous commit. Coverage was tested using:
make CFLAGS='--coverage -g3 -O0'
(cd tests && ./test_suite_x509parse)
make lcov
firefox Coverage/index.html # then visual check
Test data was generated by taking a copy of tests/data_files/crl-idp.pem,
encoding it as hex, and then manually changing the values of some bytes to
achieve the desired errors, using https://lapo.it/asn1js/ for help in locating
the desired bytes.
* development: (557 commits)
Add attribution for #1351 report
Adapt version_features.c
Note incompatibility of truncated HMAC extension in ChangeLog
Add LinkLibraryDependencies to VS2010 app template
Add ChangeLog entry for PR #1382
MD: Make deprecated functions not inline
Add ChangeLog entry for PR #1384
Have Visual Studio handle linking to mbedTLS.lib internally
Mention in ChangeLog that this fixes#1351
Add issue number to ChangeLog
Note in the changelog that this fixes an interoperability issue.
Style fix in ChangeLog
Add ChangeLog entries for PR #1168 and #1362
Add ChangeLog entry for PR #1165
ctr_drbg: Typo fix in the file description comment.
dhm: Fix typo in RFC 5114 constants
tests_suite_pkparse: new PKCS8-v2 keys with PRF != SHA1
data_files/pkcs8-v2: add keys generated with PRF != SHA1
tests/pkcs5/pbkdf2_hmac: extend array to accommodate longer results
tests/pkcs5/pbkdf2_hmac: add unit tests for additional SHA algorithms
...
For selection of test cases, see comments added in the commit.
It makes the most sense to test with chains using ECC only, so for the chain
of length 2 we use server10 -> int-ca3 -> int-ca2 and trust int-ca2 directly.
Note: server10.crt was created by copying server10_int3_int-ca2.crt and
manually truncating it to remove the intermediates. That base can now be used
to create derived certs (without or with a chain) in a programmatic way.
Our current behaviour is a bit inconsistent here:
- when the bad signature is made by a trusted CA, we stop here and don't
include the trusted CA in the chain (don't call vrfy on it)
- otherwise, we just add NOT_TRUSTED to the flags but keep building the chain
and call vrfy on the upper certs
This ensures that the callback can actually clear that flag, and that it is
seen by the callback at the right level. This flag is not set at the same
place than others, and this difference will get bigger in the upcoming
refactor, so let's ensure we don't break anything here.
When a trusted CA is rolling its root keys, it could happen that for some
users the list of trusted roots contains two versions of the same CA with the
same name but different keys. Currently this is supported but wasn't tested.
Note: the intermediate file test-ca-alt.csr is commited on purpose, as not
commiting intermediate files causes make to regenerate files that we don't
want it to touch.
As we accept EE certs that are explicitly trusted (in the list of trusted
roots) and usually look for parent by subject, and in the future we might want
to avoid checking the self-signature on trusted certs, there could a risk that we
incorrectly accept a cert that looks like a trusted root except it doesn't
have the same key. This test ensures this will never happen.
The tests cover chains of length 0, 1 and 2, with one error, located at any of
the available levels in the chain. This exercises all three call sites of
f_vrfy (two in verify_top, one in verify_child). Chains of greater length
would not cover any new code path or behaviour that I can see.
So far there was no test ensuring that the flags passed to the vrfy callback
are correct (ie the flags for the current certificate, not including those of
the parent).
Actual tests case making use of that test function will be added in the next
commit.
