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.
In x509_info_subject_alt_name() we silently dropped names that we
couldn't parse because they are not supported or are malformed. (Being
malformed might mean damaged file, but can be a sign of incompatibility
between applications.)
This commit adds code notifying the user that there is something, but
we can't parse it.
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.
We have code to skip them but didn't have explicit tests ensuring they are
(the corresponding branch was never taken).
While at it, remove extra copy of the chain in server10*.crt, which was
duplicated for no reason.
This shows inconsistencies in how flags are handled when callback fails:
- sometimes the flags set by the callback are transmitted, sometimes not
- when the cert if not trusted, sometimes BADCERT_NOT_TRUSTED is set,
sometimes not
This adds coverage for 9 lines and 9 branches. Now all lines related to
callback failure are covered.
Now all checks related to profile are covered in:
- verify_with_profile()
- verify_child()
- verify_top()
(that's 10 lines that were previously not covered)
Leaving aside profile enforcement in CRLs for now, as the focus is on
preparing to refactor cert verification.
Previously flags was left to whatever value it had before. It's cleaner to
make sure it has a definite value, and all bits set looks like the safest way
for when it went very wrong.
If we didn't walk the whole chain, then there may be any kind of errors in the
part of the chain we didn't check, so setting all flags looks like the safe
thing to do.
Inspired by test code provided by Nicholas Wilson in PR #351.
The test will fail if someone sets MAX_INTERMEDIATE_CA to a value larger than
18 (default is 8), which is hopefully unlikely and can easily be fixed by
running long.sh again with a larger value if it ever happens.
Current behaviour is suboptimal as flags are not set, but currently the goal
is only to document/test existing behaviour.
By default, keep allowing SHA-1 in key exchange signatures. Disabling
it causes compatibility issues, especially with clients that use
TLS1.2 but don't send the signature_algorithms extension.
SHA-1 is forbidden in certificates by default, since it's vulnerable
to offline collision-based attacks.
There is now one test case to validate that SHA-1 is rejected in
certificates by default, and one test case to validate that SHA-1 is
supported if MBEDTLS_TLS_DEFAULT_ALLOW_SHA1 is #defined.
SHA-1 is now disabled by default in the X.509 layer. Explicitly enable
it in our tests for now. Updating all the test data to SHA-256 should
be done over time.
Fixes a regression introduced by an earlier commit that modified
x509_crt_verify_top() to ensure that valid certificates that are after past or
future valid in the chain are processed. However the change introduced a change
in behaviour that caused the verification flags MBEDTLS_X509_BADCERT_EXPIRED and
MBEDTLS_BADCERT_FUTURE to always be set whenever there is a failure in the
verification regardless of the cause.
The fix maintains both behaviours:
* Ensure that valid certificates after future and past are verified
* Ensure that the correct verification flags are set.
The tests load certificate chains from files. The CA chains contain a
past or future certificate and an invalid certificate. The test then
checks that the flags set are MBEDTLS_X509_BADCERT_EXPIRED or
MBEDTLS_X509_BADCERT_FUTURE.
