-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[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 9 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 |
|---|---|---|
|
|
@@ -1224,21 +1224,34 @@ void ngfunction_2_ir(pugi::xml_node& netXml, | |
| } | ||
| } | ||
|
|
||
| const std::filesystem::path check_path_safety(const std::filesystem::path& path) { | ||
|
||
| 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); | ||
| return path; | ||
| } | ||
|
|
||
| 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 check_path_safety(ov::util::prevent_path_traversal(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 check_path_safety(ov::util::prevent_path_traversal(path)); | ||
| } else { | ||
| return bin_path; | ||
| return check_path_safety(ov::util::prevent_path_traversal(bin_path)); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1297,11 +1310,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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,6 +111,29 @@ TEST(file_util, sanitize_path) { | |
| string path = "C:\\workspace\\tensor.data"; | ||
| EXPECT_STREQ("workspace\\tensor.data", ov::util::sanitize_path(path).c_str()); | ||
| } | ||
| { | ||
| string path = "a/b/../tensor.data"; | ||
| EXPECT_STREQ("a/b/../tensor.data", ov::util::sanitize_path(path).c_str()); | ||
| } | ||
| } | ||
|
|
||
| TEST(file_util, prevent_path_traversal) { | ||
| { | ||
| string path = "../../tensor.data"; | ||
| EXPECT_STREQ("tensor.data", ov::util::prevent_path_traversal(path).string().c_str()); | ||
| } | ||
| { | ||
| string path = "a/b/../../../../tensor.data"; | ||
| EXPECT_STREQ("a/b/tensor.data", ov::util::prevent_path_traversal(path).string().c_str()); | ||
|
||
| } | ||
| { | ||
| string path = "a/b/../tensor.data"; | ||
| EXPECT_STREQ("a/b/tensor.data", ov::util::prevent_path_traversal(path).string().c_str()); | ||
| } | ||
| { | ||
| string path = "../a/b/../tensor.data"; | ||
| EXPECT_STREQ("a/b/tensor.data", ov::util::prevent_path_traversal(path).string().c_str()); | ||
| } | ||
| } | ||
|
|
||
| using namespace testing; | ||
|
|
||
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.
It should be updated