in client_test, print performance counters to a separate file #8107
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
examples/client_test.cpp
Dismissed
|
|
||
| switch (argv[i][1]) | ||
| { | ||
| case 'O': g_stats_log_file = std::fopen(arg, "w+"); continue; |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 18 days ago
In general, to fix uncontrolled path usage, the program should validate or constrain user-supplied file paths before passing them to file APIs. Common approaches include: (1) restricting paths to a specific base directory and verifying that the normalized absolute path still lies within that directory; (2) ensuring that inputs meant to be simple names (not full paths) do not contain separators or ".."; or (3) using an allowlist of acceptable patterns. Which approach to choose depends on the option’s intended semantics.
Here, the -O (stats log file) and -f (log file) options are intended as user-specified output file locations. To avoid changing functionality while mitigating risk, the least intrusive, still meaningful safeguard is to reject obviously dangerous path patterns: absolute paths and any path containing "..". That still allows relative paths such as log.txt or logs/output.log, but blocks direct attempts to target system files or traverse out of the working directory tree. We can implement a small helper is_safe_log_path(char const*) in this file that: (a) rejects empty strings; (b) rejects absolute paths (/ on POSIX, drive + : or leading \\ on Windows); (c) rejects any occurrence of ".." as a full component (e.g., /../, ../, ..\). If an invalid path is detected for -O or -f, the program prints an error to stderr and exits with a non-zero code instead of calling fopen.
Concretely, in examples/client_test.cpp:
- Add a static helper function in the anonymous namespace (near other utility functions) implementing
is_safe_log_path(char const* path)that performs the checks described above. To keep within the provided includes, we will use<cstring>(already included) and basic character comparisons; we do not need new headers. - In the command-line parsing switch where
case 'O'andcase 'f'are handled (around lines 1548–1553), before callingstd::fopen, callis_safe_log_path(arg). If it returnsfalse, print a clear error message (e.g.,"invalid log file path: %s\n") and return1. Otherwise, proceed with the existingstd::fopencalls unchanged. - No other options are modified: paths used via
make_absolute_path(arg)or reading filters remain as they were, to avoid altering semantics beyond the scope of the specific reported issue.
No new imports are required; we only add a helper function and small checks around the existing sink calls.
| @@ -119,8 +119,41 @@ | ||
| using std::chrono::duration_cast; | ||
| using std::stoi; | ||
|
|
||
| // return true if the given path is considered safe to use for log files | ||
| // This is a conservative check that rejects absolute paths and any use of ".." | ||
| // as a path component, to avoid simple path traversal and overwriting of | ||
| // sensitive system files when running with elevated privileges. | ||
| bool is_safe_log_path(char const* path) | ||
| { | ||
| if (path == nullptr || path[0] == '\0') return false; | ||
|
|
||
| #ifdef _WIN32 | ||
| // Reject UNC paths and drive-letter absolute paths (e.g. C:\...) | ||
| if ((path[0] == '\\' && path[1] == '\\') | ||
| || (std::isalpha(static_cast<unsigned char>(path[0])) && path[1] == ':' && (path[2] == '\\' || path[2] == '/'))) | ||
| return false; | ||
| #else | ||
| // Reject absolute paths on POSIX | ||
| if (path[0] == '/') return false; | ||
| #endif | ||
|
|
||
| // Reject any ".." path component | ||
| for (char const* p = path; *p != '\0'; ++p) | ||
| { | ||
| if (*p == '.' | ||
| && p[1] == '.' | ||
| && (p == path || p[-1] == '/' || p[-1] == '\\') | ||
| && (p[2] == '\0' || p[2] == '/' || p[2] == '\\')) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| #ifdef _WIN32 | ||
|
|
||
| bool sleep_and_input(int* c, lt::time_duration const sleep) | ||
| { | ||
| for (int i = 0; i < 2; ++i) | ||
| @@ -1547,8 +1578,22 @@ | ||
|
|
||
| switch (argv[i][1]) | ||
| { | ||
| case 'O': g_stats_log_file = std::fopen(arg, "w+"); continue; | ||
| case 'f': g_log_file = std::fopen(arg, "w+"); break; | ||
| case 'O': | ||
| if (!is_safe_log_path(arg)) | ||
| { | ||
| std::fprintf(stderr, "invalid stats log file path: %s\n", arg); | ||
| return 1; | ||
| } | ||
| g_stats_log_file = std::fopen(arg, "w+"); | ||
| continue; | ||
| case 'f': | ||
| if (!is_safe_log_path(arg)) | ||
| { | ||
| std::fprintf(stderr, "invalid log file path: %s\n", arg); | ||
| return 1; | ||
| } | ||
| g_log_file = std::fopen(arg, "w+"); | ||
| break; | ||
| case 's': save_path = make_absolute_path(arg); break; | ||
| case 'U': torrent_upload_limit = atoi(arg) * 1000; break; | ||
| case 'D': torrent_download_limit = atoi(arg) * 1000; break; |
There was a problem hiding this comment.
what this comment is missing is that the user (that provides the path) is trusted. The program is running on the user's machine. This is like suggesting programs like mv and cp to sanitize the paths passed to them.
No description provided.