Skip to content

Commit

Permalink
feat: Adding initial code to set an output error for secure file
Browse files Browse the repository at this point in the history 
Adding initial code to handle setting an error message to provide what went wrong when attempting to access a file and finding incorrect ownership or permissions during evaluation.

This will provide a user more information to determine where the security issue is so that they can address it properly and try again once it has been corrected.

[Seagate/openSeaChest#158]

Signed-off-by: Tyler Erickson <[email protected]>
  • Loading branch information
@vonericsen
vonericsen committed Oct 15, 2024
1 parent 752a44b commit 65bd2ff
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 69 deletions.
4 changes: 3 additions & 1 deletion include/secure_file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ extern "C"

M_NODISCARD fileUniqueIDInfo* os_Get_File_Unique_Identifying_Information(FILE* file);

M_NODISCARD bool os_Is_Directory_Secure(const char* fullpath);
//outputError can be a null pointer. If not null, an error message will be allocated for you.
//it can be free'd by calling safe_free() on it when you are done with the error
M_NODISCARD bool os_Is_Directory_Secure(const char* fullpath, char **outputError);

//Most members of this strucuture match the stat structure. There are some differences which is why we define it without that strucure.
//Main reason to NOT use struct stat is that Windows has a version, but to get the 64 version would make this a mess to define.
Expand Down
99 changes: 44 additions & 55 deletions src/posix_secure_file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <libgen.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>

M_NODISCARD fileAttributes* os_Get_File_Attributes_By_Name(const char* const filetoCheck)
{
Expand Down Expand Up @@ -154,9 +155,24 @@ static uid_t get_sudo_uid(void)
return sudouid;
}

FUNC_ATTR_PRINTF(2, 3) static void set_dir_security_output_error_message(char **outputError, const char* format, ...)
{
if (outputError != M_NULLPTR)
{
va_list args;
va_start(args, format);
int result = vasprintf(outputError, format, args);
va_end(args);
if (result < 0 && *outputError != M_NULLPTR)
{
safe_free(outputError);
}
}
}

#define MAX_SYMLINKS_IN_PATH 5

static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int num_symlinks)
static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int num_symlinks, char **outputError)
{
char* path_copy = M_NULLPTR;
char** dirs = M_NULLPTR;
Expand All @@ -174,28 +190,22 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
if (!fullpath || fullpath[0] != '/')
{
/* Handle error */
#if defined (_DEBUG)
printf("Full path does not start with /\n");
#endif
set_dir_security_output_error_message(outputError, "Full path does not start with /\n");
return false;
}

if (num_symlinks > MAX_SYMLINKS_IN_PATH)
{
/* Could be a symlink loop */
/* Handle error */
#if defined (_DEBUG)
printf("Too many symlinks\n");
#endif
set_dir_security_output_error_message(outputError, "Too many symlinks (must be less than %d links)\n", MAX_SYMLINKS_IN_PATH);
return false;
}

if (!(path_copy = strdup(fullpath)))
{
/* Handle error */
#if defined (_DEBUG)
printf("Cannot dup path\n");
#endif
set_dir_security_output_error_message(outputError, "Cannot strdup path (out of memory?)\n");
return false;
}

Expand All @@ -216,9 +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 */
#if defined (_DEBUG)
printf("Too many directories deep\n");
#endif
set_dir_security_output_error_message(outputError, "Too many directories deep to check permissions\n");
return false;
}
/* Now num_of_dirs indicates # of dirs we must check */
Expand All @@ -227,28 +235,22 @@ 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 */
#if defined (_DEBUG)
printf("Cannot malloc dirs array\n");
#endif
set_dir_security_output_error_message(outputError, "Cannot allocate memory for all directories needed to be checked\n");
return false;
}

if (!(dirs[num_of_dirs - 1] = strdup(fullpath)))
{
/* Handle error */
#if defined (_DEBUG)
printf("Cannot dup fullpath into dirs array\n");
#endif
set_dir_security_output_error_message(outputError, "Cannot strdup fullpath into dirs array: %s\n", fullpath);
safe_free(dirs);
return false;
}

if (!(path_copy = strdup(fullpath)))
{
/* Handle error */
#if defined (_DEBUG)
printf("Cannot dup fullpath to path copy\n");
#endif
set_dir_security_output_error_message(outputError, "Cannot strdup fullpath to path copy: %s\n", fullpath);
safe_free(dirs);
return false;
}
Expand All @@ -261,9 +263,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
if (!(dirs[i] = strdup(path_parent)))
{
/* Handle error */
#if defined (_DEBUG)
printf("Cannot dup path parent\n");
#endif
set_dir_security_output_error_message(outputError, "Cannot dup path parent: %s\n", path_parent);
secure = false;
break;
}
Expand Down Expand Up @@ -296,9 +296,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
if (lstat(dirs[i], &buf) != 0)
{
/* Handle error */
#if defined (_DEBUG)
printf("lstat failed\n");
#endif
set_dir_security_output_error_message(outputError, "lstat failed for %s\n", dirs[i]);
secure = false;
break;
}
Expand All @@ -311,47 +309,39 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
{
/* Handle error */
secure = false;
#if defined (_DEBUG)
printf("Bad link size\n");
#endif
set_dir_security_output_error_message(outputError, "Bad link size for %s\n", dirs[i]);
break;
}
linksize = buf.st_size + 1;
if (!(link = C_CAST(char*, safe_malloc(C_CAST(size_t, linksize)))))
{
/* Handle error */
secure = false;
#if defined (_DEBUG)
printf("link cannot allocate\n");
#endif
set_dir_security_output_error_message(outputError, "link cannot allocate to read origin\n");
break;
}

r = readlink(dirs[i], link, C_CAST(size_t, linksize));
if (r == -1)
{
/* Handle error */
#if defined (_DEBUG)
printf("readlink failed\n");
#endif
set_dir_security_output_error_message(outputError, "readlink failed: %s\n", dirs[i]);
secure = false;
safe_free(&link);
break;
}
else if (r >= linksize)
{
/* Handle truncation error */
#if defined (_DEBUG)
printf("link truncated\n");
#endif
set_dir_security_output_error_message(outputError, "link truncated: %s\n", dirs[i]);
secure = false;
safe_free(&link);
break;
}
link[r] = '\0';

num_symlinks++;
bool recurseSecure = internal_OS_Is_Directory_Secure(link, num_symlinks);
bool recurseSecure = internal_OS_Is_Directory_Secure(link, num_symlinks, outputError);
num_symlinks--;

if (!recurseSecure)
Expand All @@ -371,9 +361,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
if (!S_ISDIR(buf.st_mode))
{
/* Not a directory */
#if defined (_DEBUG)
printf("not a directory\n");
#endif
set_dir_security_output_error_message(outputError, "not a directory, cannot verify %s \n", dirs[i]);
secure = false;
break;
}
Expand All @@ -399,19 +387,15 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
{
/* Directory is owned by someone besides user or root */
secure = false;
#if defined (_DEBUG)
printf("Directory owned by someone other than user or root: %u my_uid: %u\n", buf.st_uid, my_uid);
#endif
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);
break;
}
}
else
{
/* Directory is owned by someone besides user or root */
secure = false;
#if defined (_DEBUG)
printf("Directory owned by someone other than user or root: %u my_uid: %u\n", buf.st_uid, my_uid);
#endif
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);
break;
}
}
Expand All @@ -421,9 +405,14 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
{
/* dir is writable by others */
secure = false;
#if defined (_DEBUG)
printf("Directory writable by others\n");
#endif
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]);
}
else
{
set_dir_security_output_error_message(outputError, "Directory (%s) writable by others. Disable write permissions for others\n", dirs[i]);
}
break;
}
}
Expand All @@ -437,10 +426,10 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
return secure;
}

bool os_Is_Directory_Secure(const char* fullpath)
bool os_Is_Directory_Secure(const char* fullpath, char **outputError)
{
unsigned int num_symlinks = 0;
return internal_OS_Is_Directory_Secure(fullpath, num_symlinks);
return internal_OS_Is_Directory_Secure(fullpath, num_symlinks, outputError);
}

bool os_Directory_Exists(const char* const pathToCheck)
Expand Down
19 changes: 10 additions & 9 deletions src/secure_file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,14 +397,16 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
//This flag can disable the file path security check.
//NOTE: Currently disabling _WIN32 due to new Windows security feature breaking how current code works.
// This will be reenabled once we have resolved the low-level issue.
char *dirSecurityError = M_NULLPTR;
#if defined (DISABLE_SECURE_FILE_PATH_CHECK) || defined (_WIN32)
#if !defined (_WIN32)
#pragma message ("WARNING: Disabling Cert-C directory security check. This is not recommended for production level code.")
#endif //!_WIN32
M_USE_UNUSED(dirSecurityError);
if (true)
#else
//Check for secure directory - This code must traverse the full path and validate permissions of the directories.
if (os_Is_Directory_Secure(pathOnly))
if (os_Is_Directory_Secure(pathOnly, &dirSecurityError))
#endif //DISABLE_SECURE_FILE_PATH_CHECK || _WIN32
{
fileInfo->file = M_NULLPTR;
Expand All @@ -413,6 +415,7 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
if (fopenError == 0 && fileInfo->file != M_NULLPTR)
#else //fopen_s not available
fileInfo->file = fopen(fileInfo->fullpath, internalmode);
errno_t fopenError = errno;
if (fileInfo->file != M_NULLPTR)
#endif //checking for MSFT secure functions or annex K of C11
{
Expand All @@ -436,6 +439,7 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
{
safe_free(&pathOnly);
}
safe_free(&dirSecurityError);
return fileInfo;
}
}
Expand All @@ -462,6 +466,7 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
{
safe_free(&pathOnly);
}
safe_free(&dirSecurityError);
return fileInfo;
}
#if defined (_WIN32)
Expand All @@ -482,6 +487,7 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
{
safe_free(&pathOnly);
}
safe_free(&dirSecurityError);
return fileInfo;
}
#endif //_WIN32
Expand All @@ -501,11 +507,7 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
}
else
{
#if defined (HAVE_C11_ANNEX_K) || defined (__STDC_SECURE_LIB__)
switch (fopenError)
#else
switch (errno)
#endif
{
//add other errno values to check for?
//https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
Expand All @@ -520,10 +522,9 @@ M_NODISCARD secureFileInfo* secure_Open_File(const char* filename, const char* m
else
{
fileInfo->error = SEC_FILE_INSECURE_PATH;
#if defined (_DEBUG)
printf("Insecure path\n");
#endif
printf("Insecure path detected: %s\n", dirSecurityError);
}
safe_free(&dirSecurityError);
if (pathOnly && allocatedLocalPathOnly)
{
safe_free(&pathOnly);
Expand Down Expand Up @@ -909,7 +910,7 @@ M_NODISCARD eSecureFileError secure_Delete_File_By_Name(const char* filename, eS
{
pathOnly = C_CAST(char*, fullpath);
}
if (!os_Is_Directory_Secure(pathOnly))
if (!os_Is_Directory_Secure(pathOnly, M_NULLPTR))
{
safe_free(&pathOnly);
return SEC_FILE_INSECURE_PATH;
Expand Down
11 changes: 7 additions & 4 deletions src/windows_secure_file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -807,14 +807,17 @@ static bool is_Folder_Secure(const char* securityDescriptorString, const char* d
#define MAX_SYMLINKS_IN_PATH 5

/* This function requires Windows style seperators (\) to function properly! */
static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int num_symlinks)
static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int num_symlinks, char **outputError)
{
char* path_copy = M_NULLPTR;
char** dirs = M_NULLPTR;
ssize_t num_of_dirs = 1;
bool secure = true;
ssize_t i = 0;

//TODO: Set error message appropriate for Windows errors detected
M_USE_UNUSED(outputError);

if (!fullpath || fullpath[0] == '\0')
{
/* Handle error */
Expand Down Expand Up @@ -992,7 +995,7 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
wcstombs(reparsePath, C_CAST(wchar_t*, reparseData->SymbolicLinkReparseBuffer.PathBuffer), bufferSize);
#endif
num_symlinks++;
bool recurseSecure = internal_OS_Is_Directory_Secure(reparsePath, num_symlinks);
bool recurseSecure = internal_OS_Is_Directory_Secure(reparsePath, num_symlinks, outputError);
num_symlinks--;
if (!recurseSecure)
{
Expand Down Expand Up @@ -1076,11 +1079,11 @@ static bool internal_OS_Is_Directory_Secure(const char* fullpath, unsigned int n
return secure;
}

M_NODISCARD bool os_Is_Directory_Secure(const char* fullpath)
M_NODISCARD bool os_Is_Directory_Secure(const char* fullpath, char **outputError)
{
//This was implemented as close as possible to https://wiki.sei.cmu.edu/confluence/display/c/FIO15-C.+Ensure+that+file+operations+are+performed+in+a+secure+directory
unsigned int num_symlinks = 0;
return internal_OS_Is_Directory_Secure(fullpath, num_symlinks);
return internal_OS_Is_Directory_Secure(fullpath, num_symlinks, outputError);
}

bool os_Directory_Exists(const char* const pathToCheck)
Expand Down

0 comments on commit 65bd2ff

Please sign in to comment.