-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[CORE] Add path security check for model serialization #31820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
34bb674
25cd94d
de02de2
66208c4
da5954b
0a5dd54
1eb7e26
f506dd3
6724b9a
d7d23c5
a9b7d76
ef73d0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,12 @@ std::string path_to_string(const Path& path) { | |
| /// \return A sanitized path | ||
| std::string sanitize_path(const std::string& path); | ||
|
|
||
| /// \brief Check the safety of a file path by expecting no parent directory references ("..") and no symbolic links | ||
| /// references. | ||
| /// \param path A path to file | ||
| /// \return A validated path to file | ||
| const std::filesystem::path check_path_safety(const std::filesystem::path& path); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add test for this function ? |
||
|
|
||
| /// \brief Returns the name with extension for a given path | ||
| /// \param path The path to the output file | ||
| std::string get_file_name(const std::string& path); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |||||
| #include <fstream> | ||||||
| #include <sstream> | ||||||
|
|
||||||
| #include "openvino/core/except.hpp" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is not part of util library |
||||||
| #include "openvino/util/common_util.hpp" | ||||||
|
|
||||||
| #ifdef _WIN32 | ||||||
|
|
@@ -256,6 +257,19 @@ std::string ov::util::sanitize_path(const std::string& path) { | |||||
| return (start == std::string::npos) ? "" : sanitized_path.substr(start); | ||||||
| } | ||||||
|
|
||||||
| const std::filesystem::path ov::util::check_path_safety(const std::filesystem::path& path) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this function simple like: bool is_path_traversal(const std::filesystem::path& path){
return std::find(path.begin(), path.end(), "..") != path.end();
} |
||||||
| const bool contains_dotdot = std::any_of(path.begin(), path.end(), [](const auto& part) { | ||||||
| return part == ".."; | ||||||
| }); | ||||||
|
|
||||||
| OPENVINO_ASSERT(contains_dotdot == false, | ||||||
| "Invalid file path: path contains parent directory reference '..': ", | ||||||
| path); | ||||||
|
|
||||||
| OPENVINO_ASSERT(std::filesystem::is_symlink(path) == false, "Path must not refer to a symbolic link: ", path); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return path; | ||||||
| } | ||||||
|
|
||||||
| void ov::util::convert_path_win_style(std::string& path) { | ||||||
| std::replace(path.begin(), path.end(), '/', '\\'); | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ namespace pass { | |
| * @brief Serialize transformation converts ov::Model into IR files | ||
| * @attention | ||
| * - dynamic shapes are not supported | ||
| * - any parent directory traversal (e.g. ".." in path) will be removed from output file paths | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be updated |
||
| * - symbolic links are not allowed as output file paths | ||
| * \ingroup ov_pass_cpp_api | ||
| */ | ||
| class OPENVINO_API Serialize : public ov::pass::ModelPass { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1226,19 +1226,19 @@ void ngfunction_2_ir(pugi::xml_node& netXml, | |
|
|
||
| const std::filesystem::path valid_xml_path(const std::filesystem::path& path) { | ||
| OPENVINO_ASSERT(path.extension() == ".xml", | ||
| "Path for xml file doesn't contains file name with 'xml' extension: \"", | ||
| path, | ||
| "\""); | ||
| return path; | ||
| "Path for xml file doesn't contains file name with 'xml' extension: ", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider change this helper to: void validate_ir_path(const std::filesystem::path& path, std::filesystem::path&& ext){
OPENVINO_ASSERT(path.extension() == ext, "Path ", path, " doesn't contains file name with ", ext, " extension");
OPENVINO_ASSERT(!is_path_traversal(path), "Path has traversal component: ", path);
OPENVINO_ASSERT(!std::filesystem::is_symlink(path), "Path is symlink ", path);
}And use for xml, bin file validation, and additional check can be added here if required. |
||
| path); | ||
|
|
||
| return ov::util::check_path_safety(std::filesystem::absolute(path)); | ||
| } | ||
|
|
||
| std::filesystem::path provide_bin_path(const std::filesystem::path& xml_path, const std::filesystem::path& bin_path) { | ||
| if (bin_path.empty()) { | ||
| auto path = xml_path; | ||
| path.replace_extension(".bin"); | ||
| return path; | ||
| return ov::util::check_path_safety(std::filesystem::absolute(path)); | ||
| } else { | ||
| return bin_path; | ||
| return ov::util::check_path_safety(std::filesystem::absolute(bin_path)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1297,11 +1297,11 @@ bool pass::Serialize::run_on_model(const std::shared_ptr<ov::Model>& model) { | |
| ov::util::create_directory_recursive(m_xmlPath); | ||
|
|
||
| std::ofstream bin_file(m_binPath, std::ios::binary); | ||
| OPENVINO_ASSERT(bin_file, "Can't open bin file: \"", m_binPath, "\""); | ||
| OPENVINO_ASSERT(bin_file, "Can't open bin file: ", m_binPath); | ||
|
|
||
| // create xml file | ||
| std::ofstream xml_file(m_xmlPath); | ||
| OPENVINO_ASSERT(xml_file, "Can't open xml file: \"", m_xmlPath, "\""); | ||
| OPENVINO_ASSERT(xml_file, "Can't open xml file: ", m_xmlPath); | ||
|
|
||
| try { | ||
| serializeFunc(xml_file, bin_file, model, m_version); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should current dir
.be checked, also?