diff --git a/CHANGELOG.md b/CHANGELOG.md index 6386befe..eb59080c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ The git repositories are hosted at the following sites: * https://bacnet.sourceforge.net/ * https://github.com/bacnet-stack/bacnet-stack/ -## [Unreleased] - 2026-01-03 +## [Unreleased] - 2026-01-05 ### Security @@ -26,9 +26,15 @@ The git repositories are hosted at the following sites: Fixed ubasic string variables to initialize with zeros. Fixed compile errors when UBASIC_DEBUG_STRINGVARIABLES is defined. Added ubasic string variables user accessor API and unit testing. (#1196) +* Secured BACnet file object pathname received from BACnet AtomicWriteFile + or ReadFile service used without validation which was vulnerable to + directory traversal attacks. (#1197) ### Added +* Added file path name checking for AtomicReadFile and AtomicWriteFile + example applications. Prohibits use of relative and absolute file paths + when BACNET_FILE_PATH_RESTRICTED is defined non-zero. (#1197) * Added API and optional properties to basic load control object example Refactored BACnetShedLevel encoding, decoding, and printing into separate file. Added BACnetShedLevel validation testing. (#1187) diff --git a/apps/readfile/main.c b/apps/readfile/main.c index 4213e800..dd0a96e4 100644 --- a/apps/readfile/main.c +++ b/apps/readfile/main.c @@ -286,6 +286,10 @@ int main(int argc, char *argv[]) /* decode the command line parameters */ Target_Device_Object_Instance = strtol(argv[1], NULL, 0); Target_File_Object_Instance = strtol(argv[2], NULL, 0); + if (!filename_path_valid(argv[3])) { + fprintf(stderr, "Invalid file path: %s\n", argv[3]); + return 1; + } Local_File_Name = argv[3]; if (Target_Device_Object_Instance > BACNET_MAX_INSTANCE) { fprintf( diff --git a/apps/writefile/main.c b/apps/writefile/main.c index 84fe9075..6f0e6995 100644 --- a/apps/writefile/main.c +++ b/apps/writefile/main.c @@ -159,6 +159,10 @@ int main(int argc, char *argv[]) /* decode the command line parameters */ Target_Device_Object_Instance = strtol(argv[1], NULL, 0); Target_File_Object_Instance = strtol(argv[2], NULL, 0); + if (!filename_path_valid(argv[3])) { + fprintf(stderr, "Invalid file path: %s\n", argv[3]); + return 1; + } Local_File_Name = argv[3]; if (Target_Device_Object_Instance > BACNET_MAX_INSTANCE) { fprintf( diff --git a/ports/posix/bacfile-posix.c b/ports/posix/bacfile-posix.c index 1dcc3425..c7c9e071 100644 --- a/ports/posix/bacfile-posix.c +++ b/ports/posix/bacfile-posix.c @@ -13,11 +13,14 @@ #include /* BACnet Stack defines - first */ #include "bacnet/bacdef.h" -#include "bacnet/basic/sys/debug.h" #include "bacnet/basic/object/bacfile.h" +#include "bacnet/basic/sys/debug.h" +#include "bacnet/basic/sys/filename.h" +/* me! */ +#include "bacfile-posix.h" -#ifndef FILE_RECORD_SIZE -#define FILE_RECORD_SIZE MAX_OCTET_STRING_BYTES +#ifndef BACNET_FILE_POSIX_RECORD_SIZE +#define BACNET_FILE_POSIX_RECORD_SIZE MAX_OCTET_STRING_BYTES #endif /** @@ -50,7 +53,7 @@ size_t bacfile_posix_file_size(const char *pathname) long file_position = 0; size_t file_size = 0; - if (pathname) { + if (filename_path_valid(pathname)) { pFile = fopen(pathname, "rb"); if (pFile) { file_position = fsize(pFile); @@ -100,7 +103,7 @@ size_t bacfile_posix_read_stream_data( FILE *pFile = NULL; size_t len = 0; - if (pathname) { + if (filename_path_valid(pathname)) { pFile = fopen(pathname, "rb"); if (pFile) { (void)fseek(pFile, fileStartPosition, SEEK_SET); @@ -131,7 +134,7 @@ size_t bacfile_posix_write_stream_data( size_t bytes_written = 0; FILE *pFile = NULL; - if (pathname) { + if (filename_path_valid(pathname)) { if (fileStartPosition == 0) { /* open the file as a clean slate when starting at 0 */ pFile = fopen(pathname, "wb"); @@ -177,11 +180,11 @@ bool bacfile_posix_write_record_data( bool status = false; FILE *pFile = NULL; uint32_t i = 0; - char dummy_data[FILE_RECORD_SIZE]; + char dummy_data[BACNET_FILE_POSIX_RECORD_SIZE]; const char *pData = NULL; size_t fileSeekRecord = 0; - if (pathname) { + if (filename_path_valid(pathname)) { if (fileStartRecord == 0) { /* open the file as a clean slate when starting at 0 */ pFile = fopen(pathname, "wb"); @@ -238,11 +241,11 @@ bool bacfile_posix_read_record_data( bool status = false; FILE *pFile = NULL; uint32_t i = 0; - char dummy_data[FILE_RECORD_SIZE] = { 0 }; + char dummy_data[BACNET_FILE_POSIX_RECORD_SIZE] = { 0 }; const char *pData = NULL; size_t fileSeekRecord = 0; - if (pathname) { + if (filename_path_valid(pathname)) { pFile = fopen(pathname, "rb"); if (pFile) { fileSeekRecord = fileStartRecord + fileIndexRecord; diff --git a/src/bacnet/basic/object/bacfile.c b/src/bacnet/basic/object/bacfile.c index 85870bd2..4e4aae22 100644 --- a/src/bacnet/basic/object/bacfile.c +++ b/src/bacnet/basic/object/bacfile.c @@ -29,9 +29,6 @@ #include "bacnet/basic/sys/keylist.h" #include "bacnet/basic/tsm/tsm.h" -#ifndef FILE_RECORD_SIZE -#define FILE_RECORD_SIZE MAX_OCTET_STRING_BYTES -#endif struct object_data { char *Object_Name; char *Pathname; diff --git a/src/bacnet/basic/sys/filename.c b/src/bacnet/basic/sys/filename.c index bc5ff941..72a161d4 100644 --- a/src/bacnet/basic/sys/filename.c +++ b/src/bacnet/basic/sys/filename.c @@ -1,14 +1,25 @@ /** * @file - * @brief Function for filename manipulation + * @brief Function for filename and path manipulation and validation * @author Steve Karg * @date 2007 * @copyright SPDX-License-Identifier: GPL-2.0-or-later WITH GCC-exception-2.0 */ #include #include +#include "bacnet/basic/sys/debug.h" #include "bacnet/basic/sys/filename.h" +/* restrict file paths */ +#ifndef BACNET_FILE_PATH_RESTRICTED +#define BACNET_FILE_PATH_RESTRICTED 1 +#endif + +/** + * @brief Remove path from filename + * @param filename_in - input filename with path + * @return filename without path + */ const char *filename_remove_path(const char *filename_in) { const char *filename_out = filename_in; @@ -30,3 +41,57 @@ const char *filename_remove_path(const char *filename_in) return filename_out; } + +/** + * @brief Validate if pathname is valid by checking for patterns + * such as relative paths and absolute paths which are prohibited. + * @param pathname Path to validate + * @return true if valid, false if not + */ +bool filename_path_valid(const char *pathname) +{ + int path_len; + + if (!pathname) { + return false; + } + if (pathname[0] == 0) { + return false; + } +#if BACNET_FILE_PATH_RESTRICTED + /* check for relative directory patterns */ + if (strstr(pathname, "..") != NULL) { + debug_printf_stderr("Relative paths are prohibited: %s\n", pathname); + return false; + } + /* check for absolute paths */ + if (pathname[0] == '/') { + debug_printf_stderr("Absolute paths are prohibited: %s\n", pathname); + return false; + } + /* check for Windows drive letters (should be relative paths only) */ + path_len = strlen(pathname); + if (path_len >= 2 && pathname[1] == ':') { + debug_printf_stderr( + "Windows drive letters are prohibited: %s\n", pathname); + return false; + } + + /* check for consecutive path separators */ + if (strstr(pathname, "//") != NULL || strstr(pathname, "\\\\") != NULL) { + debug_printf_stderr( + "Consecutive path separators are prohibited: %s\n", pathname); + return false; + } + + /* check for path components that are just dots */ + if (strstr(pathname, "/./") != NULL || strstr(pathname, "\\./") != NULL || + strstr(pathname, "/.\\") != NULL || strstr(pathname, "\\.\\") != NULL) { + debug_printf_stderr( + "Current directory references are prohibited: %s\n", pathname); + return false; + } +#endif + + return true; +} diff --git a/src/bacnet/basic/sys/filename.h b/src/bacnet/basic/sys/filename.h index a2cf348e..ba50fc3e 100644 --- a/src/bacnet/basic/sys/filename.h +++ b/src/bacnet/basic/sys/filename.h @@ -9,6 +9,10 @@ #define BACNET_SYS_FILENAME_H /* BACnet Stack defines - first */ #include "bacnet/bacdef.h" +/* standard includes */ +#include +#include +#include #ifdef __cplusplus extern "C" { @@ -16,6 +20,8 @@ extern "C" { BACNET_STACK_EXPORT const char *filename_remove_path(const char *filename_in); +BACNET_STACK_EXPORT +bool filename_path_valid(const char *pathname); #ifdef __cplusplus } diff --git a/test/bacnet/basic/object/netport/CMakeLists.txt b/test/bacnet/basic/object/netport/CMakeLists.txt index b6019f57..c9384292 100644 --- a/test/bacnet/basic/object/netport/CMakeLists.txt +++ b/test/bacnet/basic/object/netport/CMakeLists.txt @@ -85,6 +85,7 @@ add_executable(${PROJECT_NAME} ${SRC_DIR}/bacnet/basic/sys/debug.c ${SRC_DIR}/bacnet/basic/sys/bigend.c ${SRC_DIR}/bacnet/basic/sys/days.c + ${SRC_DIR}/bacnet/basic/sys/filename.c ${SRC_DIR}/bacnet/basic/sys/keylist.c ${SRC_DIR}/bacnet/datalink/bvlc.c ${SRC_DIR}/bacnet/datalink/bvlc6.c diff --git a/test/bacnet/basic/sys/filename/CMakeLists.txt b/test/bacnet/basic/sys/filename/CMakeLists.txt index 39bb2acd..884c45b8 100644 --- a/test/bacnet/basic/sys/filename/CMakeLists.txt +++ b/test/bacnet/basic/sys/filename/CMakeLists.txt @@ -34,6 +34,7 @@ add_executable(${PROJECT_NAME} # File(s) under test ${SRC_DIR}/bacnet/basic/sys/filename.c # Support files and stubs (pathname alphabetical) + ${SRC_DIR}/bacnet/basic/sys/debug.c # Test and test library files ./src/main.c ${ZTST_DIR}/ztest_mock.c diff --git a/test/bacnet/basic/sys/filename/src/main.c b/test/bacnet/basic/sys/filename/src/main.c index 75c3680e..e2dfd593 100644 --- a/test/bacnet/basic/sys/filename/src/main.c +++ b/test/bacnet/basic/sys/filename/src/main.c @@ -27,6 +27,7 @@ static void testFilename(void) const char *data3 = "c:\\Program Files\\Christopher\\run.exe"; const char *data4 = "//Mary/data/run"; const char *data5 = "bin\\run"; + const char *data6 = "run.exe"; const char *filename = NULL; filename = filename_remove_path(data1); @@ -39,9 +40,53 @@ static void testFilename(void) zassert_equal(strcmp("run", filename), 0, NULL); filename = filename_remove_path(data5); zassert_equal(strcmp("run", filename), 0, NULL); + filename = filename_remove_path(data6); + zassert_equal(strcmp("run.exe", filename), 0, NULL); return; } + +#if defined(CONFIG_ZTEST_NEW_API) +ZTEST(filename_tests, testFilenameValid) +#else +static void testFilenameValid(void) +#endif +{ + const char *data0 = ""; + const char *data1 = "c:\\Joshua\\run"; + const char *data2 = "/home/Anna/run"; + const char *data3 = "c:\\Program Files\\Christopher\\run.exe"; + const char *data4 = "//Mary/data/run"; + const char *data5 = "bin\\\\run"; + const char *data6 = "bin/./run"; + const char *data7 = "bin/../run"; + const char *data_valid = "certs/mycert.pem"; + bool valid = false; + + valid = filename_path_valid(NULL); + zassert_false(valid, NULL); + valid = filename_path_valid(data0); + zassert_false(valid, NULL); + valid = filename_path_valid(data1); + zassert_false(valid, NULL); + valid = filename_path_valid(data2); + zassert_false(valid, NULL); + valid = filename_path_valid(data3); + zassert_false(valid, NULL); + valid = filename_path_valid(data4); + zassert_false(valid, NULL); + valid = filename_path_valid(data5); + zassert_false(valid, NULL); + valid = filename_path_valid(data6); + zassert_false(valid, NULL); + valid = filename_path_valid(data7); + zassert_false(valid, NULL); + valid = filename_path_valid(data_valid); + zassert_true(valid, NULL); + + return; +} + /** * @} */ @@ -51,7 +96,9 @@ ZTEST_SUITE(filename_tests, NULL, NULL, NULL, NULL, NULL); #else void test_main(void) { - ztest_test_suite(filename_tests, ztest_unit_test(testFilename)); + ztest_test_suite( + filename_tests, ztest_unit_test(testFilename), + ztest_unit_test(testFilenameValid)); ztest_run_test_suite(filename_tests); } diff --git a/test/bacnet/datalink/bsc-datalink/CMakeLists.txt b/test/bacnet/datalink/bsc-datalink/CMakeLists.txt index e5d4aaca..77e5825d 100644 --- a/test/bacnet/datalink/bsc-datalink/CMakeLists.txt +++ b/test/bacnet/datalink/bsc-datalink/CMakeLists.txt @@ -167,6 +167,7 @@ target_sources(${PROJECT_NAME} PRIVATE ${SRC_DIR}/bacnet/basic/sys/days.c ${SRC_DIR}/bacnet/basic/sys/debug.c ${SRC_DIR}/bacnet/basic/sys/fifo.c + ${SRC_DIR}/bacnet/basic/sys/filename.c ${SRC_DIR}/bacnet/basic/sys/keylist.c ${SRC_DIR}/bacnet/basic/sys/mstimer.c ${SRC_DIR}/bacnet/access_rule.c