[CORE] Add path security check for model serialization #31820
Conversation
| @@ -1224,21 +1224,34 @@ void ngfunction_2_ir(pugi::xml_node& netXml, | |||
| } | |||
| } | |||
|
|
|||
| const std::filesystem::path check_path_safety(const std::filesystem::path& path) { | |||
There was a problem hiding this comment.
Re-use update current utils functions:
Use ov::util::sanitize_path which should remove traversal part from path.
There are also utils like in std like canonical, weakly_canonical
src/core/src/pass/serialize.cpp
Outdated
|
|
||
| return check_path_safety(path); |
There was a problem hiding this comment.
| return check_path_safety(path); | |
| OPENVINO_ASSERT(!std::filesystem::is_symlink(path), "Path must not be symbolic link: " , path); | |
| return ov::util::sanitize(path); |
src/core/src/pass/serialize.cpp
Outdated
| } | ||
|
|
||
| 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(path); |
There was a problem hiding this comment.
The xml path should be validated and bin path can be created without check
src/core/src/pass/serialize.cpp
Outdated
| } else { | ||
| return bin_path; | ||
| return check_path_safety(bin_path); |
There was a problem hiding this comment.
Check if not symlink and use sanitize path to remove traversal part.
Update doxy for class to mention that traversal part from path will be removed and symlinks are not allowed
github-actions
bot
added
category: transformations
| string path = "a/b/../../../../tensor.data"; | ||
| EXPECT_STREQ("a/b/tensor.data", ov::util::prevent_path_traversal(path).string().c_str()); |
There was a problem hiding this comment.
Why .. is skipped over (ignored)? Shouldn't it end up as ../../tensor.data?
There was a problem hiding this comment.
To prevent traversal into parent directories
There was a problem hiding this comment.
Proposed solution changes the path in fact. Is it what user expects (is aware of)?
Example path from another test a/b/../tensor.data doesn't leave working directory and should end up as a/tensor.data.
Generally I don't think OV should bother where a user wants its files to be stored in.
There was a problem hiding this comment.
@jszczepa Could you please share the motivation for preventing path traversal
Details:
Tickets: