Validate the path during ConfigFile upload
The IBM management console usecase - ConfigFile upload was allowing
to create or modify any file at the BMC when the path url is given
as below.
PUT https://${bmc}/ibm/v1/Host/ConfigFiles/../../../../../<any file under root dir> --data-binary "junk data"
This commit adds validation to the "path" variable after the "ConfigFiles/"
in the url - so that only the ConfigFiles are created or modified.
The filename validation includes:
Restrict the maximum filename length to 20 characters
Restrict the allowed charaters to [A-Za-z0-9-]
The minimum size of the file allowed is 100 bytes
The maximum size of the file allowed is 500KB
Maximum total size of the ConfigFile directory at BMC file system allowed is 10MB
Tested by:
1. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/../../../../../etc/p2 --data-binary "some data"
Bad Request
2. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/../../../etc/p2 --data-binary "some data"
Bad Request
3. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/../etc/p2 --data-binary "some data"
Bad Request
4. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/etc/p2 --data-binary "some data"
{
"Description": "Error while creating the file"
}
5. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/mydir/p2 --data-binary "some data"
{
"Description": "Error while creating the file"
}
6. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/ --data-binary "some data"
Not Found
7. PUT https://${bmc}/ibm/v1/Host/ConfigFiles --data-binary "some data"
Method Not Allowed
8. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2/../p2 --data-binary "some data"
Bad Request
9. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2/p2 --data-binary "some data"
{
"Description": "Error while creating the file"
}
10. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2/../../../p2 --data-binary "some data"
Bad Request
11. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/./../../p2 --data-binary "some data"
Bad Request
12. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/. --data-binary "some data"
Bad Request
13. PUT https://${bmc}/ibm/v1/Host/../ConfigFiles/p2 --data-binary "some data"
Not Found
14. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2 --data-binary "some data"
{
"Description": "File Created"
}
15. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2 --data-binary "some data"
{
"Description": "File Updated"
}
16. PUT https://${bmc}/ibm/v1/Host/ConfigFiles/p2.ext --data-binary "some data"
{
"Description": "File Created"
}
17. Tested sending filename greater than 20 charaters
Bad Request
18. Tested sending filename with special charaters
Bad Request
19. Tested sending filesize less than 100bytes
Bad request
20. Tested sending filesize greater than 500KB
Bad request
21. Tested uploading the file when the directory size is nearly full
Bad request
22. Added unit test for isValidConfigFileName
Signed-off-by: Sunitha Harish <sunharis@in.ibm.com>
Change-Id: I838d39d5765ddc8701f7e5c533a93eebde021cbf
diff --git a/include/ibm/management_console_rest.hpp b/include/ibm/management_console_rest.hpp
index 3612b4f..0ce0d5b 100644
--- a/include/ibm/management_console_rest.hpp
+++ b/include/ibm/management_console_rest.hpp
@@ -15,7 +15,6 @@
#include <filesystem>
#include <fstream>
-#include <regex>
using SType = std::string;
using SegmentFlags = std::vector<std::pair<std::string, uint32_t>>;
@@ -34,6 +33,10 @@
constexpr const char* contentNotAcceptableMsg = "Content Not Acceptable";
constexpr const char* internalServerError = "Internal Server Error";
+constexpr size_t maxSaveareaDirSize =
+ 10000000; // Allow save area dir size to be max 10MB
+constexpr size_t minSaveareaFileSize =
+ 100; // Allow save area file size of minimum 100B
constexpr size_t maxSaveareaFileSize =
500000; // Allow save area file size upto 500KB
constexpr size_t maxBroadcastMsgSize =
@@ -80,6 +83,7 @@
inline void handleFilePut(const crow::Request& req, crow::Response& res,
const std::string& fileID)
{
+ std::error_code ec;
// Check the content-type of the request
std::string_view contentType = req.getHeaderValue("content-type");
if (boost::starts_with(contentType, "multipart/form-data"))
@@ -101,27 +105,126 @@
res.jsonValue["Description"] = resourceNotFoundMsg;
return;
}
- // Create the file
+
std::ofstream file;
std::filesystem::path loc("/var/lib/obmc/bmc-console-mgmt/save-area");
- loc /= fileID;
+ // Get the current size of the savearea directory
+ std::filesystem::recursive_directory_iterator iter(loc, ec);
+ if (ec)
+ {
+ res.result(boost::beast::http::status::internal_server_error);
+ res.jsonValue["Description"] = internalServerError;
+ BMCWEB_LOG_DEBUG << "handleIbmPut: Failed to prepare save-area "
+ "directory iterator. ec : "
+ << ec;
+ return;
+ }
+ std::uintmax_t saveAreaDirSize = 0;
+ for (auto& it : iter)
+ {
+ if (!std::filesystem::is_directory(it, ec))
+ {
+ if (ec)
+ {
+ res.result(boost::beast::http::status::internal_server_error);
+ res.jsonValue["Description"] = internalServerError;
+ BMCWEB_LOG_DEBUG << "handleIbmPut: Failed to find save-area "
+ "directory . ec : "
+ << ec;
+ return;
+ }
+ std::uintmax_t fileSize = std::filesystem::file_size(it, ec);
+ if (ec)
+ {
+ res.result(boost::beast::http::status::internal_server_error);
+ res.jsonValue["Description"] = internalServerError;
+ BMCWEB_LOG_DEBUG << "handleIbmPut: Failed to find save-area "
+ "file size inside the directory . ec : "
+ << ec;
+ return;
+ }
+ saveAreaDirSize += fileSize;
+ }
+ }
+ BMCWEB_LOG_DEBUG << "saveAreaDirSize: " << saveAreaDirSize;
+
+ // Get the file size getting uploaded
const std::string& data = req.body;
- BMCWEB_LOG_DEBUG << "data capaticty : " << data.capacity();
- if (data.capacity() > maxSaveareaFileSize)
+ BMCWEB_LOG_DEBUG << "data length: " << data.length();
+
+ if (data.length() < minSaveareaFileSize)
+ {
+ res.result(boost::beast::http::status::bad_request);
+ res.jsonValue["Description"] =
+ "File size is less than minimum allowed size[100B]";
+ return;
+ }
+ if (data.length() > maxSaveareaFileSize)
{
res.result(boost::beast::http::status::bad_request);
res.jsonValue["Description"] =
"File size exceeds maximum allowed size[500KB]";
return;
}
+
+ // Form the file path
+ loc /= fileID;
BMCWEB_LOG_DEBUG << "Writing to the file: " << loc;
- bool fileExists = false;
- if (std::filesystem::exists(loc))
+ // Check if the same file exists in the directory
+ bool fileExists = std::filesystem::exists(loc, ec);
+ if (ec)
{
- fileExists = true;
+ res.result(boost::beast::http::status::internal_server_error);
+ res.jsonValue["Description"] = internalServerError;
+ BMCWEB_LOG_DEBUG << "handleIbmPut: Failed to find if file exists. ec : "
+ << ec;
+ return;
}
+
+ std::uintmax_t newSizeToWrite = 0;
+ if (fileExists)
+ {
+ // File exists. Get the current file size
+ std::uintmax_t currentFileSize = std::filesystem::file_size(loc, ec);
+ if (ec)
+ {
+ res.result(boost::beast::http::status::internal_server_error);
+ res.jsonValue["Description"] = internalServerError;
+ BMCWEB_LOG_DEBUG << "handleIbmPut: Failed to find file size. ec : "
+ << ec;
+ return;
+ }
+ // Calculate the difference in the file size.
+ // If the data.length is greater than the existing file size, then
+ // calculate the difference. Else consider the delta size as zero -
+ // because there is no increase in the total directory size.
+ // We need to add the diff only if the incoming data is larger than the
+ // existing filesize
+ if (data.length() > currentFileSize)
+ {
+ newSizeToWrite = data.length() - currentFileSize;
+ }
+ BMCWEB_LOG_DEBUG << "newSizeToWrite: " << newSizeToWrite;
+ }
+ else
+ {
+ // This is a new file upload
+ newSizeToWrite = data.length();
+ }
+
+ // Calculate the total dir size before writing the new file
+ BMCWEB_LOG_DEBUG << "total new size: " << saveAreaDirSize + newSizeToWrite;
+
+ if ((saveAreaDirSize + newSizeToWrite) > maxSaveareaDirSize)
+ {
+ res.result(boost::beast::http::status::bad_request);
+ res.jsonValue["Description"] = "File size does not fit in the savearea "
+ "directory maximum allowed size[10MB]";
+ return;
+ }
+
file.open(loc, std::ofstream::out);
if (file.fail())
{
@@ -562,6 +665,40 @@
res.end();
}
+inline bool isValidConfigFileName(const std::string& fileName,
+ crow::Response& res)
+{
+ if (fileName.empty())
+ {
+ BMCWEB_LOG_ERROR << "Empty filename";
+ res.jsonValue["Description"] = "Empty file path in the url";
+ return false;
+ }
+
+ // ConfigFile name is allowed to take upper and lowercase letters,
+ // numbers and hyphen
+ std::size_t found = fileName.find_first_not_of(
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-");
+ if (found != std::string::npos)
+ {
+ BMCWEB_LOG_ERROR << "Unsupported character in filename: " << fileName;
+ res.jsonValue["Description"] = "Unsupported character in filename";
+ return false;
+ }
+
+ // Check the filename length
+ if (fileName.length() > 20)
+ {
+ BMCWEB_LOG_ERROR << "Name must be maximum 20 characters. "
+ "Input filename length is: "
+ << fileName.length();
+ res.jsonValue["Description"] = "Filename must be maximum 20 characters";
+ return false;
+ }
+
+ return true;
+}
+
inline void requestRoutes(App& app)
{
@@ -599,12 +736,24 @@
deleteConfigFiles(res);
});
- BMCWEB_ROUTE(app, "/ibm/v1/Host/ConfigFiles/<path>")
+ BMCWEB_ROUTE(app, "/ibm/v1/Host/ConfigFiles/<str>")
.privileges({"ConfigureComponents", "ConfigureManager"})
- .methods(boost::beast::http::verb::put, boost::beast::http::verb::get,
- boost::beast::http::verb::delete_)(
- [](const crow::Request& req, crow::Response& res,
- const std::string& path) { handleFileUrl(req, res, path); });
+ .methods(
+ boost::beast::http::verb::put, boost::beast::http::verb::get,
+ boost::beast::http::verb::delete_)([](const crow::Request& req,
+ crow::Response& res,
+ const std::string& fileName) {
+ std::shared_ptr<bmcweb::AsyncResp> asyncResp =
+ std::make_shared<bmcweb::AsyncResp>(res);
+ BMCWEB_LOG_DEBUG << "ConfigFile : " << fileName;
+ // Validate the incoming fileName
+ if (!isValidConfigFileName(fileName, res))
+ {
+ asyncResp->res.result(boost::beast::http::status::bad_request);
+ return;
+ }
+ handleFileUrl(req, res, fileName);
+ });
BMCWEB_ROUTE(app, "/ibm/v1/HMC/LockService")
.privileges({"ConfigureComponents", "ConfigureManager"})
diff --git a/meson.build b/meson.build
index 7a16e91..5eaa63c 100644
--- a/meson.build
+++ b/meson.build
@@ -330,6 +330,7 @@
srcfiles_unittest = ['include/ut/dbus_utility_test.cpp',
'redfish-core/ut/privileges_test.cpp',
'redfish-core/ut/lock_test.cpp',
+ 'redfish-core/ut/configfile_test.cpp',
'http/ut/utility_test.cpp']
# Gather the Configuration data
@@ -389,7 +390,7 @@
include_directories : incdir,
install_dir: bindir,
dependencies: [
- boost, boost_url, gtest,openssl,gmock,nlohmann_json,sdbusplus
+ boost, boost_url, gtest,openssl,gmock,nlohmann_json,sdbusplus,pam
]))
endforeach
endif
diff --git a/redfish-core/ut/configfile_test.cpp b/redfish-core/ut/configfile_test.cpp
new file mode 100644
index 0000000..9f6bc13
--- /dev/null
+++ b/redfish-core/ut/configfile_test.cpp
@@ -0,0 +1,71 @@
+#include "ibm/management_console_rest.hpp"
+#include "nlohmann/json.hpp"
+
+#include <string>
+
+#include "gmock/gmock.h"
+
+namespace crow
+{
+namespace ibm_mc
+{
+
+TEST(ConfigFileTest, FileNameValidChar)
+{
+ crow::Response res;
+
+ const std::string fileName = "GoodConfigFile";
+ EXPECT_TRUE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameInvalidChar)
+{
+ crow::Response res;
+
+ const std::string fileName = "Bad@file";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameInvalidPath1)
+{
+ crow::Response res;
+
+ const std::string fileName = "/../../../../../etc/badpath";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameInvalidPath2)
+{
+ crow::Response res;
+
+ const std::string fileName = "/../../etc/badpath";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameInvalidPath3)
+{
+ crow::Response res;
+
+ const std::string fileName = "/mydir/configFile";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameNull)
+{
+ crow::Response res;
+
+ const std::string fileName = "";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameSlash)
+{
+ crow::Response res;
+
+ const std::string fileName = "/";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+TEST(ConfigFileTest, FileNameMorethan20Char)
+{
+ crow::Response res;
+
+ const std::string fileName = "BadfileBadfileBadfile";
+ EXPECT_FALSE(isValidConfigFileName(fileName, res));
+}
+
+} // namespace ibm_mc
+} // namespace crow