From a5dc85f8fffe381dd273192f6eb0268f54ed1fdd Mon Sep 17 00:00:00 2001 From: Tyler Erickson Date: Mon, 28 Oct 2024 12:34:47 -0600 Subject: [PATCH] feat: Improving secure file error messages Further improvement to the error messages from the POSIX secure file. This will provide the user with better information about how to correct the situation and when possible this function will also provide recommendations for commands to run (chown or chmod) as necessary. [Seagate/openSeaChest#161] Signed-off-by: Tyler Erickson --- src/posix_secure_file.c | 48 +++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/src/posix_secure_file.c b/src/posix_secure_file.c index c158ef8..2c36380 100644 --- a/src/posix_secure_file.c +++ b/src/posix_secure_file.c @@ -190,7 +190,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (!fullpath || fullpath[0] != '/') { /* Handle error */ - set_dir_security_output_error_message(outputError, "Full path does not start with /\n"); + set_dir_security_output_error_message(outputError, "Error: Full path must start with \"/\".\n"); return false; } @@ -198,14 +198,14 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n { /* Could be a symlink loop */ /* Handle error */ - set_dir_security_output_error_message(outputError, "Too many symlinks (must be less than %d links)\n", MAX_SYMLINKS_IN_PATH); + set_dir_security_output_error_message(outputError, "Error: Too many symbolic links (must be fewer than %d links)\n", MAX_SYMLINKS_IN_PATH); return false; } if (!(path_copy = strdup(fullpath))) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Cannot strdup path (out of memory?)\n"); + set_dir_security_output_error_message(outputError, "Error: Unable to duplicate fullpath to path copy: %s (Out of memory)\n", fullpath); return false; } @@ -226,7 +226,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (num_of_dirs == SSIZE_MAX) { /* out of room to compare this many directories deep */ - set_dir_security_output_error_message(outputError, "Too many directories deep to check permissions\n"); + set_dir_security_output_error_message(outputError, "Error: The path contains too many directories. Please reduce the number of directories in the path and try again. Must be less than %zd directories in the path.\n", SSIZE_MAX); return false; } /* Now num_of_dirs indicates # of dirs we must check */ @@ -235,14 +235,14 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (!(dirs = C_CAST(char**, safe_malloc(C_CAST(size_t, num_of_dirs) * sizeof(char*))))) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Cannot allocate memory for all directories needed to be checked\n"); + set_dir_security_output_error_message(outputError, "Error: Unable to allocate memory for directory validation. Please ensure sufficient memory is available and try again.\n"); return false; } if (!(dirs[num_of_dirs - 1] = strdup(fullpath))) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Cannot strdup fullpath into dirs array: %s\n", fullpath); + set_dir_security_output_error_message(outputError, "Error: Unable to duplicate fullpath into dirs array: %s (Out of memory)\n", fullpath); safe_free(dirs); return false; } @@ -250,7 +250,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (!(path_copy = strdup(fullpath))) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Cannot strdup fullpath to path copy: %s\n", fullpath); + set_dir_security_output_error_message(outputError, "Error: Unable to duplicate fullpath to path copy: %s (Out of memory)\n", fullpath); safe_free(dirs); return false; } @@ -263,7 +263,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (!(dirs[i] = strdup(path_parent))) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Cannot dup path parent: %s\n", path_parent); + set_dir_security_output_error_message(outputError, "Error: Unable to duplicate path parent: %s (Out of memory)\n", path_parent); secure = false; break; } @@ -296,7 +296,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (lstat(dirs[i], &buf) != 0) { /* Handle error */ - set_dir_security_output_error_message(outputError, "Failed to read file status for %s\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: Failed to read file status for %s. This operation is necessary to retrieve ownership and permission details. Please check the path and ensure you have the required permissions.\n", dirs[i]); secure = false; break; } @@ -309,7 +309,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n { /* Handle error */ secure = false; - set_dir_security_output_error_message(outputError, "Bad link size for %s\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: Invalid link size for %s. The size of the symbolic link is negative, which indicates a potential filesystem issue.\n", dirs[i]); break; } linksize = buf.st_size + 1; @@ -317,7 +317,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n { /* Handle error */ secure = false; - set_dir_security_output_error_message(outputError, "link cannot allocate to read origin location\n"); + set_dir_security_output_error_message(outputError, "Error: Unable to allocate memory to read the link for %s. (Out of memory)\n", dirs[i]); break; } @@ -325,7 +325,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (r == -1) { /* Handle error */ - set_dir_security_output_error_message(outputError, "readlink failed: %s\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: Failed to read the symbolic link for %s. Please check the path and ensure the link exists and is accessible.\n", dirs[i]); secure = false; safe_free(&link); break; @@ -333,7 +333,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n else if (r >= linksize) { /* Handle truncation error */ - set_dir_security_output_error_message(outputError, "link truncated: %s\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: The symbolic link for %s is truncated. The link is too long to be read completely.\n", dirs[i]); secure = false; safe_free(&link); break; @@ -361,7 +361,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n if (!S_ISDIR(buf.st_mode)) { /* Not a directory */ - set_dir_security_output_error_message(outputError, "not a directory, cannot verify %s for secure path\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: %s is not a directory. Cannot verify for secure path.\n", dirs[i]); secure = false; break; } @@ -383,11 +383,21 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n #if defined (_DEBUG) printf("UID detected: %u\n", sudouid); #endif - if (sudouid != ROOT_UID_VAL && buf.st_uid != sudouid) + if (sudouid != ROOT_UID_VAL) + { + if (buf.st_uid != sudouid) + { + /* Directory is owned by someone besides user or root */ + secure = false; + set_dir_security_output_error_message(outputError, "Error: Directory (%s) owned by someone other than user or root. Owner: %u Current User: %u. Recommended action: \"chown %u:%u %s\"\n", dirs[i], buf.st_uid, sudouid, sudouid, sudouid, dirs[i]); + break; + } + } + else { /* Directory is owned by someone besides user or root */ secure = false; - set_dir_security_output_error_message(outputError, "Directory (%s) owned by someone other than user or root. Owner: %u Current User: %u\n", dirs[i], buf.st_uid, sudouid); + set_dir_security_output_error_message(outputError, "Error: Directory (%s) owned by someone other than user or root. Owner: %u Current User: %u. Recommended action: \"chown %u:%u %s\"\n", dirs[i], buf.st_uid, my_uid, my_uid, my_uid, dirs[i]); break; } } @@ -395,7 +405,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n { /* Directory is owned by someone besides user or root */ secure = false; - set_dir_security_output_error_message(outputError, "Directory (%s) owned by someone other than user or root. Owner: %u Current User: %u\n", dirs[i], buf.st_uid, my_uid); + set_dir_security_output_error_message(outputError, "Error: Directory (%s) owned by someone other than user or root. Owner: %u Current User: %u. Recommended action: \"chown %u:%u %s\"\n", dirs[i], buf.st_uid, my_uid, my_uid, my_uid, dirs[i]); break; } } @@ -407,11 +417,11 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n secure = false; if (buf.st_mode & S_IWGRP) { - set_dir_security_output_error_message(outputError, "Directory (%s) writable by others. Disable write permissions for groups\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: Directory (%s) writable by group. Disable write permissions for groups. Recommended action: \"chmod u=rwx,g=rx,o=rx %s\"\n", dirs[i], dirs[i]); } else { - set_dir_security_output_error_message(outputError, "Directory (%s) writable by others. Disable write permissions for others\n", dirs[i]); + set_dir_security_output_error_message(outputError, "Error: Directory (%s) writable by others. Disable write permissions for others. Recommended action: \"chmod u=rwx,g=rx,o=rx %s\"\n", dirs[i], dirs[i]); } break; }