x509_crt: handle properly broken links when looking for certificates

[edsiper]

Description

On non-windows environments, when loading certificates from a given
path through mbedtls_x509_crt_parse_path() function, if a symbolic
link is found and is broken (meaning the target file don't exists),
the function is returning MBEDTLS_ERR_X509_FILE_IO_ERROR which is
not honoring the default behavior of just skip the bad certificate file
and increase the counter of wrong files.

The problem have been raised many times in our open source project
called Fluent Bit which depends on MbedTLS:

fluent/fluent-bit#843 (comment)

The expected behavior is that if a simple certificate cannot be processed,
it should just be skipped.

This patch implements a workaround with lstat(2) and stat(2) to determinate
first, if the entry found in the directory is a symbolic link or not, if is
a symbolic link, do a proper stat(2) for the target file, otherwise process
normally. Upon finding a broken symbolic link it will increase the counter of
not processed certificates.

Status

READY

Backport in #6161

[hanno-becker]

Minor: determine instead of determinate?

Or, alternatively, a suggestion for the entire comment: "Use lstat instead of stat to not fail in case of a broken link. In case of a valid link, another call to stat is needed below to query the properties of the file the link points to."

[hanno-becker]

Minor: Split the comment into multiple sentences. E.g. "If stat(2) fails, it could be a broken link or a generic error. If the link is broken, report is as a certificate that could not be processed; otherwise, return the generic MBEDTLS_ERR_X509_FILE_IO_ERROR."

[hanno-becker]

@edsiper Thank you for your report and for providing the fix!

Overall, the PR looks good to me. I left some nitpicks, as well as a question in regards to the behavior in case of a broken link. Apart from that, I think the change merits an entry in the ChangeLog (probably in the Bugfix section), and we should have a test exercising mbedtls_x509_crt_parse_path() on a directory including a broken link (in the test suite tests/suites/test_suite_x509parse.datax). Could you take care of both?

Let us know if you need any support with it, and thanks again!

[gilles-peskine-arm]

I'm a bit uncomfortable with the preexisting ask-for-permission (as opposed to ask-for-forgiveness) here, complete with race condition: stat then open. Granted, a directory containing certificates doesn't change often, and the use of opendir/readdir means that we don't get a consistent view of the directory if files are added, removed or renamed. But I'd still prefer to minimize such effects. In addition, the logic is getting more complex here.

If mbedtls_x509_crt_parse_file returns MBEDTLS_ERR_PK_FILE_IO_ERROR, it means that either fopen or fread failed in mbedtls_pk_load_file. In that case errno still contains the corresponding error code. Would it be possible to use this to switch to a beg-for-forgiveness approach? Always call mbedtls_x509_crt_parse_file, then if it returns MBEDTLS_ERR_PK_FILE_IO_ERROR, analyze errno to determine whether to count the file as silently skipped (ENOENT) or to error out (any other errno value).

This would change the behavior on non-regular, but fopen/fread-able files. For named pipes, that's rather an improvements. It would be a problem on systems where directories allow fopen and fread. What platforms allow that these days?

[daverodgman]

Rebase and address feedback.

I haven't reworked to eliminate the stat-then-open issue @gilles-peskine-arm points out. It's a pre-existing issue, and it would be hard to fully eliminate this kind of race - the directory could be modified during iteration. It also sounds like the kind of thing where we might run into platform issues. So I think fixing the stat-then-open race is out-of-scope for this PR. I will open a separate issue (#6108)

[tom-cosgrove-arm]

LGTM

[tom-daubney-arm]

LGTM

Hanno's changes have been addressed.