From e1bfffc4f6836b5095969aa835b514f518626aa3 Mon Sep 17 00:00:00 2001 From: Eduardo Silva Date: Thu, 25 Apr 2019 10:43:26 -0600 Subject: [PATCH 1/5] x509_crt: handle properly broken links when looking for certificates 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: https://github.com/fluent/fluent-bit/issues/843#issuecomment-486388209 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 simbolic link, do a proper stat(2) for the target file, otherwise process normally. Upon find a broken symbolic link it will increase the counter of not processed certificates. Signed-off-by: Eduardo Silva --- library/x509_crt.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index b38dff0859..f73628e051 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -81,6 +81,7 @@ #else #include #endif /* __MBED__ */ +#include #endif /* !_WIN32 || EFIX64 || EFI32 */ #endif @@ -1655,10 +1656,39 @@ cleanup: ret = MBEDTLS_ERR_X509_BUFFER_TOO_SMALL; goto cleanup; } - else if( stat( entry_name, &sb ) == -1 ) + else { - ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; - goto cleanup; + /* determinate if the file entry could be a link, using lstat(2) + * is safer than just stat(2), otherwise a broken link will + * give us a false positive. */ + if( lstat( entry_name, &sb ) == -1 ) + { + ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; + goto cleanup; + } + + /* if the file is a symbolic link, we need to validate the real + * information using stat(2). */ + if( S_ISLNK( sb.st_mode ) ) + { + /* if stat(2) fails it could be a broken link or a generic + * error, if the link is broken, just report it as a + * certificate that could not be processed, otherwise + * just set a MBEDTLS_ERR_X509_FILE_IO_ERROR. */ + if( stat( entry_name, &sb ) == -1 ) + { + if( errno == ENOENT ) + { + /* Broken link */ + ret++; + } + else + { + ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; + goto cleanup; + } + } + } } if( !S_ISREG( sb.st_mode ) ) From 935154ef049299f581469cd1412e9422ee4e7447 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 20 Jul 2022 14:01:45 +0100 Subject: [PATCH 2/5] Don't increase failure count for dangling symlinks Signed-off-by: Dave Rodgman --- library/x509_crt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index f73628e051..3f00e43844 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1679,8 +1679,8 @@ cleanup: { if( errno == ENOENT ) { - /* Broken link */ - ret++; + /* Broken link - ignore this entry */ + continue; } else { From 103f8b6506f62939b650c328c5b7854caecf9bd5 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 1 Jul 2022 11:31:05 +0100 Subject: [PATCH 3/5] Spelling and grammar improvements Signed-off-by: Dave Rodgman --- library/x509_crt.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index 3f00e43844..e4f7945d4a 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1658,7 +1658,7 @@ cleanup: } else { - /* determinate if the file entry could be a link, using lstat(2) + /* Determine if the file entry could be a link. Using lstat(2) * is safer than just stat(2), otherwise a broken link will * give us a false positive. */ if( lstat( entry_name, &sb ) == -1 ) @@ -1667,13 +1667,12 @@ cleanup: goto cleanup; } - /* if the file is a symbolic link, we need to validate the real + /* If the file is a symbolic link, we need to validate the real * information using stat(2). */ if( S_ISLNK( sb.st_mode ) ) { - /* if stat(2) fails it could be a broken link or a generic - * error, if the link is broken, just report it as a - * certificate that could not be processed, otherwise + /* If stat(2) fails it could be a broken link or a generic + * error. If the link is broken, ignore it, otherwise * just set a MBEDTLS_ERR_X509_FILE_IO_ERROR. */ if( stat( entry_name, &sb ) == -1 ) { From c95cb6d6e5d9f6bfb6021b613aaed2939b8a9394 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 1 Jul 2022 12:57:21 +0100 Subject: [PATCH 4/5] Add Changelog entry Signed-off-by: Dave Rodgman --- ChangeLog.d/x509-broken-symlink-handling.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/x509-broken-symlink-handling.txt diff --git a/ChangeLog.d/x509-broken-symlink-handling.txt b/ChangeLog.d/x509-broken-symlink-handling.txt new file mode 100644 index 0000000000..52288dc089 --- /dev/null +++ b/ChangeLog.d/x509-broken-symlink-handling.txt @@ -0,0 +1,5 @@ +Bugfix + * Fix handling of broken symlinks when loading certificates using + mbedtls_x509_crt_parse_path(). Instead of returning an error as soon as a + broken link is encountered, skip the broken link and continue parsing + other certificate files. Contributed by Eduardo Silva in #2602. From fa40b02da3b423556cd6d054e6e5da90c2b69c5e Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 20 Jul 2022 16:08:00 +0100 Subject: [PATCH 5/5] Remove use of lstat lstat is not available on some platforms (e.g. Ubuntu 16.04). In this particular case stat is sufficient. Signed-off-by: Dave Rodgman --- library/x509_crt.c | 40 +++++++++++++--------------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index e4f7945d4a..a8f23c5fd5 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1656,38 +1656,24 @@ cleanup: ret = MBEDTLS_ERR_X509_BUFFER_TOO_SMALL; goto cleanup; } - else + else if( stat( entry_name, &sb ) == -1 ) { - /* Determine if the file entry could be a link. Using lstat(2) - * is safer than just stat(2), otherwise a broken link will - * give us a false positive. */ - if( lstat( entry_name, &sb ) == -1 ) + if( errno == ENOENT ) { + /* Broken symbolic link - ignore this entry. + stat(2) will return this error for either (a) a dangling + symlink or (b) a missing file. + Given that we have just obtained the filename from readdir, + assume that it does exist and therefore treat this as a + dangling symlink. */ + continue; + } + else + { + /* Some other file error; report the error. */ ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; goto cleanup; } - - /* If the file is a symbolic link, we need to validate the real - * information using stat(2). */ - if( S_ISLNK( sb.st_mode ) ) - { - /* If stat(2) fails it could be a broken link or a generic - * error. If the link is broken, ignore it, otherwise - * just set a MBEDTLS_ERR_X509_FILE_IO_ERROR. */ - if( stat( entry_name, &sb ) == -1 ) - { - if( errno == ENOENT ) - { - /* Broken link - ignore this entry */ - continue; - } - else - { - ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; - goto cleanup; - } - } - } } if( !S_ISREG( sb.st_mode ) )