From 32ffb2b8bc9f9229a83681ae4f1a21f15d0bd0eb 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 911644b7da9f..97a908c134e3 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -77,6 +77,7 @@ #include #include #include +#include #endif /* !_WIN32 || EFIX64 || EFI32 */ #endif @@ -1638,10 +1639,39 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) 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 168bcd684b3aea04ce5ff1a300804f3a9653c580 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 97a908c134e3..bf567e0fa42d 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1662,8 +1662,8 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) { if( errno == ENOENT ) { - /* Broken link */ - ret++; + /* Broken link - ignore this entry */ + continue; } else { From 2958bb37613c6d4010a98159be64e60fdfeea8d9 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 bf567e0fa42d..0e4279bf76fb 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1641,7 +1641,7 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) } 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 ) @@ -1650,13 +1650,12 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) 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 626b37859cc74328d2053ebe2888ccd33b790e2e 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 000000000000..52288dc0890a --- /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 6f227ee8e8fc0c5929722a989c6123ef582e236b 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 0e4279bf76fb..42f1fc22c5b0 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1639,37 +1639,23 @@ int mbedtls_x509_crt_parse_path( mbedtls_x509_crt *chain, const char *path ) 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 ) { - ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; - goto cleanup; + /* 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; } - - /* If the file is a symbolic link, we need to validate the real - * information using stat(2). */ - if( S_ISLNK( sb.st_mode ) ) + else { - /* 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; - } - } + /* Some other file error; report the error. */ + ret = MBEDTLS_ERR_X509_FILE_IO_ERROR; + goto cleanup; } }