Skip to content

Commit 2bd60c8

Browse files
committed
Fix CodeQL violations of type 'cpp/path-injection in lib/pal
1 parent 0747bc7 commit 2bd60c8

File tree

3 files changed

+132
-0
lines changed

3 files changed

+132
-0
lines changed

lib/pal/PAL.cpp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
#include <Objbase.h>
4848
#pragma comment(lib, "Ole32.Lib") /* CoCreateGuid */
4949
#include <oacr.h>
50+
#include <windows.h>
5051
#endif
5152

5253
#ifdef ANDROID
@@ -113,8 +114,28 @@ namespace PAL_NS_BEGIN {
113114
return result;
114115
}
115116

117+
// Check if the path exists
118+
#if defined(_WIN32) || defined(_WIN64)
119+
DWORD fileAttr = GetFileAttributesA(traceFolderPath.c_str());
120+
bool pathExists = (fileAttr != INVALID_FILE_ATTRIBUTES && (fileAttr & FILE_ATTRIBUTE_DIRECTORY));
121+
#else
122+
bool pathExists = (access(traceFolderPath.c_str(), F_OK) != -1);
123+
#endif
124+
// Check if the path contains ".."
125+
bool containsParentDirectory = (traceFolderPath.find("..") != std::string::npos);
126+
127+
if (!pathExists || containsParentDirectory)
128+
{
129+
assert(false && "Invalid trace folder path.");
130+
return false;
131+
}
132+
116133
debugLogMutex.lock();
117134
debugLogPath = traceFolderPath;
135+
if (debugLogPath.back() != '/' && debugLogPath.back() != '\\')
136+
{
137+
debugLogPath += "/";
138+
}
118139
debugLogPath += "mat-debug-";
119140
debugLogPath += std::to_string(MAT::GetCurrentProcessId());
120141
debugLogPath += ".log";
@@ -131,6 +152,16 @@ namespace PAL_NS_BEGIN {
131152
return result;
132153
}
133154

155+
const std::unique_ptr<std::fstream>& getDebugLogStream() noexcept
156+
{
157+
return debugLogStream;
158+
}
159+
160+
const std::string& getDebugLogPath() noexcept
161+
{
162+
return debugLogPath;
163+
}
164+
134165
void log_done()
135166
{
136167
debugLogMutex.lock();

lib/pal/PAL.hpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,16 @@ namespace PAL_NS_BEGIN
239239
return GetPAL().RegisterIkeyWithWindowsTelemetry(ikeyin, storageSize, uploadQuotaSize);
240240
}
241241

242+
#ifdef HAVE_MAT_LOGGING
243+
namespace detail
244+
{
245+
bool log_init(bool isTraceEnabled, const std::string& traceFolderPath);
246+
const std::unique_ptr<std::fstream>& getDebugLogStream() noexcept;
247+
const std::string& getDebugLogPath() noexcept;
248+
void log_done();
249+
}
250+
#endif
251+
242252
} PAL_NS_END
243253

244254
#endif

tests/unittests/PalTests.cpp

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,21 @@
77
#include "pal/PseudoRandomGenerator.hpp"
88
#include "Version.hpp"
99

10+
#ifdef HAVE_MAT_LOGGING
11+
#include "pal/PAL.hpp"
12+
#include <gtest/gtest.h>
13+
#include <fstream>
14+
#include <memory>
15+
#include <string>
16+
#include <cstdio>
17+
#if defined(_WIN32) || defined(_WIN64)
18+
#include <windows.h>
19+
#else
20+
#include <unistd.h>
21+
#endif
22+
using namespace PAL::detail;
23+
#endif
24+
1025
using namespace testing;
1126

1227
class PalTests : public Test {};
@@ -127,3 +142,79 @@ TEST_F(PalTests, SdkVersion)
127142

128143
EXPECT_THAT(PAL::getSdkVersion(), Eq(v));
129144
}
145+
146+
#ifdef HAVE_MAT_LOGGING
147+
class LogInitTest : public Test
148+
{
149+
protected:
150+
const std::string validPath = "valid/path/";
151+
152+
void SetUp() override
153+
{
154+
// Create the valid path directory
155+
#if defined(_WIN32) || defined(_WIN64)
156+
CreateDirectoryA(validPath, NULL);
157+
#else
158+
mkdir(validPath.c_str(), 0777);
159+
#endif
160+
}
161+
162+
void TearDown() override
163+
{
164+
PAL::detail::log_done();
165+
if (!PAL::detail::getDebugLogPath().empty())
166+
{
167+
std::remove(PAL::detail::getDebugLogPath().c_str());
168+
}
169+
170+
// Remove the valid path directory
171+
#if defined(_WIN32) || defined(_WIN64)
172+
RemoveDirectoryA(validPath);
173+
#else
174+
rmdir(validPath.c_str());
175+
#endif
176+
}
177+
};
178+
179+
TEST_F(LogInitTest, LogInitDisabled)
180+
{
181+
EXPECT_FALSE(log_init(false, validPath));
182+
}
183+
184+
TEST_F(LogInitTest, LogInitValidPath)
185+
{
186+
EXPECT_TRUE(PAL::detail::log_init(true, validPath));
187+
EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open());
188+
}
189+
190+
TEST_F(LogInitTest, LogInitParentDirectoryInvalidPath)
191+
{
192+
EXPECT_FALSE(PAL::detail::log_init(true, "invalid/../path/"));
193+
}
194+
195+
TEST_F(LogInitTest, LogInitPathDoesNotExist)
196+
{
197+
EXPECT_FALSE(PAL::detail::log_init(true, "nonexistent/path/"));
198+
}
199+
200+
TEST_F(LogInitTest, LogInitAlreadyInitialized)
201+
{
202+
EXPECT_TRUE(PAL::detail::log_init(true, validPath));
203+
EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open());
204+
EXPECT_TRUE(PAL::detail::log_init(true, validPath)); // Should return true as it's already initialized
205+
}
206+
207+
TEST_F(LogInitTest, LogInitPathWithoutTrailingSlash)
208+
{
209+
std::string pathWithoutSlash = "valid/path";
210+
EXPECT_TRUE(PAL::detail::log_init(true, pathWithoutSlash));
211+
EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open());
212+
}
213+
214+
TEST_F(LogInitTest, LogInitPathWithoutTrailingBackslash)
215+
{
216+
std::string pathWithoutBackslash = "valid\\path";
217+
EXPECT_TRUE(PAL::detail::log_init(true, pathWithoutBackslash));
218+
EXPECT_TRUE(PAL::detail::getDebugLogStream()->is_open());
219+
}
220+
#endif

0 commit comments

Comments
 (0)