From 98e8aa8128511875857d901e7909d0eeb6066bde Mon Sep 17 00:00:00 2001 From: Mateusz Bencer Date: Tue, 17 Nov 2020 05:16:39 +0100 Subject: [PATCH] Protect reading external weights from up-dir (#3098) * introduced path sanitizing * added tests, folders structure clean-up * fixed test * fixed up-dir path handling * improved sanitizing for windows * fix Windows test * move sanitize impl --- ...2\346\227\245\346\234\254\350\252\236.prototxt" | 2 +- .../onnx_reader/onnx_reader_external_data.cpp | 2 +- ngraph/core/include/ngraph/file_util.hpp | 5 ++ ngraph/core/src/file_util.cpp | 10 ++- ngraph/frontend/onnx_import/src/core/transform.cpp | 4 +- ngraph/test/file_util.cpp | 28 ++++++ .../{ => external_data}/external_data.prototxt | 2 +- .../external_data_different_paths.prototxt | 4 +- .../external_data_file_not_found.prototxt | 0 .../external_data_optional_fields.prototxt | 2 +- .../external_data_sanitize_test.prototxt | 99 +++++++++++++++++++++ ...data_two_tensors_data_in_the_same_file.prototxt | 4 +- .../external_data_file_in_up_dir.prototxt | 82 +++++++++++++++++ .../external_data/tensors_data}/a/tensor_a.data | Bin .../external_data/tensors_data}/b/tensor_b.data | Bin .../tensors_data}/multiple_tensors.data | Bin .../onnx/external_data/tensors_data}/tensor.data | Bin .../tensors_data}/tensor_optional_fields.data | Bin ngraph/test/onnx/onnx_import_external_data.in.cpp | 54 +++++++++-- 19 files changed, 279 insertions(+), 19 deletions(-) rename "inference-engine/tests/functional/inference_engine/onnx_reader/models/\320\220\320\221\320\222\320\223\320\224\320\225\320\201\320\226\320\227\320\230\320\231/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" => "inference-engine/tests/functional/inference_engine/onnx_reader/models/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" (97%) rename ngraph/test/models/onnx/{ => external_data}/external_data.prototxt (96%) rename ngraph/test/models/onnx/{ => external_data}/external_data_different_paths.prototxt (90%) rename ngraph/test/models/onnx/{ => external_data}/external_data_file_not_found.prototxt (100%) rename ngraph/test/models/onnx/{ => external_data}/external_data_optional_fields.prototxt (95%) create mode 100644 ngraph/test/models/onnx/external_data/external_data_sanitize_test.prototxt rename ngraph/test/models/onnx/{ => external_data}/external_data_two_tensors_data_in_the_same_file.prototxt (91%) create mode 100644 ngraph/test/models/onnx/external_data/inner_scope/external_data_file_in_up_dir.prototxt rename ngraph/test/{files/onnx/external_data => models/onnx/external_data/tensors_data}/a/tensor_a.data (100%) rename ngraph/test/{files/onnx/external_data => models/onnx/external_data/tensors_data}/b/tensor_b.data (100%) rename ngraph/test/{files/onnx/external_data => models/onnx/external_data/tensors_data}/multiple_tensors.data (100%) rename ngraph/test/{files/onnx/external_data => models/onnx/external_data/tensors_data}/tensor.data (100%) rename ngraph/test/{files/onnx/external_data => models/onnx/external_data/tensors_data}/tensor_optional_fields.data (100%) diff --git "a/inference-engine/tests/functional/inference_engine/onnx_reader/models/\320\220\320\221\320\222\320\223\320\224\320\225\320\201\320\226\320\227\320\230\320\231/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" "b/inference-engine/tests/functional/inference_engine/onnx_reader/models/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" similarity index 97% rename from "inference-engine/tests/functional/inference_engine/onnx_reader/models/\320\220\320\221\320\222\320\223\320\224\320\225\320\201\320\226\320\227\320\230\320\231/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" rename to "inference-engine/tests/functional/inference_engine/onnx_reader/models/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" index ab03f7c..a6f5372 100644 --- "a/inference-engine/tests/functional/inference_engine/onnx_reader/models/\320\220\320\221\320\222\320\223\320\224\320\225\320\201\320\226\320\227\320\230\320\231/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" +++ "b/inference-engine/tests/functional/inference_engine/onnx_reader/models/\343\201\262\343\202\211\343\201\214\343\201\252\346\227\245\346\234\254\350\252\236.prototxt" @@ -23,7 +23,7 @@ graph { name: "A" external_data { key: "location", - value: "../data/tensor.data" + value: "data/tensor.data" } data_location: 1 } diff --git a/inference-engine/tests/functional/inference_engine/onnx_reader/onnx_reader_external_data.cpp b/inference-engine/tests/functional/inference_engine/onnx_reader/onnx_reader_external_data.cpp index 0d4c073..1a3cf5b 100644 --- a/inference-engine/tests/functional/inference_engine/onnx_reader/onnx_reader_external_data.cpp +++ b/inference-engine/tests/functional/inference_engine/onnx_reader/onnx_reader_external_data.cpp @@ -78,7 +78,7 @@ TEST(ONNX_Reader_Tests, ImportModelWithExternalDataFromWstringNamedFile) { std::string win_dir_path = ONNX_TEST_MODELS; std::replace(win_dir_path.begin(), win_dir_path.end(), '/', '\\'); const std::wstring unicode_win_dir_path = FileUtils::multiByteCharToWString(win_dir_path.c_str()); - const std::wstring path = unicode_win_dir_path + L"АБВГДЕЁЖЗИЙ\\ひらがな日本語.prototxt"; + const std::wstring path = unicode_win_dir_path + L"ひらがな日本語.prototxt"; auto cnnNetwork = ie.ReadNetwork(path, L""); auto function = cnnNetwork.getFunction(); diff --git a/ngraph/core/include/ngraph/file_util.hpp b/ngraph/core/include/ngraph/file_util.hpp index 8f9bf9c..eb5f03e 100644 --- a/ngraph/core/include/ngraph/file_util.hpp +++ b/ngraph/core/include/ngraph/file_util.hpp @@ -77,5 +77,10 @@ namespace ngraph /// \param str A null-terminated string /// \return A wide-char string NGRAPH_API std::wstring multi_byte_char_to_wstring(const char* str); + + /// \brief Remove path components which would allow traversing up a directory tree. + /// \param path A path to file + /// \return A sanitiazed path + NGRAPH_API std::string sanitize_path(const std::string& path); } } diff --git a/ngraph/core/src/file_util.cpp b/ngraph/core/src/file_util.cpp index 1d18396..15a75f0 100644 --- a/ngraph/core/src/file_util.cpp +++ b/ngraph/core/src/file_util.cpp @@ -255,6 +255,15 @@ void file_util::iterate_files(const string& path, } } +std::string file_util::sanitize_path(const std::string& path) +{ + const auto colon_pos = path.find(":"); + const auto sanitized_path = path.substr(colon_pos == std::string::npos ? 0 : colon_pos + 1); + const std::string to_erase = "/.\\"; + const auto start = sanitized_path.find_first_not_of(to_erase); + return (start == std::string::npos) ? "" : sanitized_path.substr(start); +} + NGRAPH_API void file_util::convert_path_win_style(std::string& path) { std::replace(path.begin(), path.end(), '/', '\\'); @@ -291,5 +300,4 @@ std::wstring file_util::multi_byte_char_to_wstring(const char* str) return result; #endif } - #endif // ENABLE_UNICODE_PATH_SUPPORT diff --git a/ngraph/frontend/onnx_import/src/core/transform.cpp b/ngraph/frontend/onnx_import/src/core/transform.cpp index 094b2dd..0b2fe5d 100644 --- a/ngraph/frontend/onnx_import/src/core/transform.cpp +++ b/ngraph/frontend/onnx_import/src/core/transform.cpp @@ -92,8 +92,10 @@ void ngraph::onnx_import::transform::update_external_data_paths( { const auto external_data_relative_path = initializer_tensor.external_data(location_key_value_index).value(); + const auto santized_external_data_relative_path = + file_util::sanitize_path(external_data_relative_path); auto external_data_full_path = - file_util::path_join(model_dir_path, external_data_relative_path); + file_util::path_join(model_dir_path, santized_external_data_relative_path); #if defined(ENABLE_UNICODE_PATH_SUPPORT) && defined(_WIN32) file_util::convert_path_win_style(external_data_full_path); diff --git a/ngraph/test/file_util.cpp b/ngraph/test/file_util.cpp index ff67616..c486ad9 100644 --- a/ngraph/test/file_util.cpp +++ b/ngraph/test/file_util.cpp @@ -89,3 +89,31 @@ TEST(file_util, path_join) EXPECT_STREQ("/test1/test2", file_util::path_join(s1, s2).c_str()); } } + +TEST(file_util, santize_path) +{ + { + string path = "../../tensor.data"; + EXPECT_STREQ("tensor.data", file_util::sanitize_path(path).c_str()); + } + { + string path = "/../tensor.data"; + EXPECT_STREQ("tensor.data", file_util::sanitize_path(path).c_str()); + } + { + string path = ".."; + EXPECT_STREQ("", file_util::sanitize_path(path).c_str()); + } + { + string path = "workspace/data/tensor.data"; + EXPECT_STREQ("workspace/data/tensor.data", file_util::sanitize_path(path).c_str()); + } + { + string path = "..\\..\\tensor.data"; + EXPECT_STREQ("tensor.data", file_util::sanitize_path(path).c_str()); + } + { + string path = "C:\\workspace\\tensor.data"; + EXPECT_STREQ("workspace\\tensor.data", file_util::sanitize_path(path).c_str()); + } +} diff --git a/ngraph/test/models/onnx/external_data.prototxt b/ngraph/test/models/onnx/external_data/external_data.prototxt similarity index 96% rename from ngraph/test/models/onnx/external_data.prototxt rename to ngraph/test/models/onnx/external_data/external_data.prototxt index 62465aa..9500b33 100644 --- a/ngraph/test/models/onnx/external_data.prototxt +++ b/ngraph/test/models/onnx/external_data/external_data.prototxt @@ -41,7 +41,7 @@ graph { name: "A" external_data { key: "location", - value: "../../files/onnx/external_data/tensor.data" + value: "tensors_data/tensor.data" } data_location: 1 } diff --git a/ngraph/test/models/onnx/external_data_different_paths.prototxt b/ngraph/test/models/onnx/external_data/external_data_different_paths.prototxt similarity index 90% rename from ngraph/test/models/onnx/external_data_different_paths.prototxt rename to ngraph/test/models/onnx/external_data/external_data_different_paths.prototxt index 2213357..82bb359 100644 --- a/ngraph/test/models/onnx/external_data_different_paths.prototxt +++ b/ngraph/test/models/onnx/external_data/external_data_different_paths.prototxt @@ -15,7 +15,7 @@ graph { name: "data_a" external_data { key: "location", - value: "../../files/onnx/external_data/a/tensor_a.data" + value: "tensors_data/a/tensor_a.data" } data_location: 1 } @@ -25,7 +25,7 @@ graph { name: "data_b" external_data { key: "location", - value: "../../files/onnx/external_data/b/tensor_b.data" + value: "tensors_data/b/tensor_b.data" } data_location: 1 } diff --git a/ngraph/test/models/onnx/external_data_file_not_found.prototxt b/ngraph/test/models/onnx/external_data/external_data_file_not_found.prototxt similarity index 100% rename from ngraph/test/models/onnx/external_data_file_not_found.prototxt rename to ngraph/test/models/onnx/external_data/external_data_file_not_found.prototxt diff --git a/ngraph/test/models/onnx/external_data_optional_fields.prototxt b/ngraph/test/models/onnx/external_data/external_data_optional_fields.prototxt similarity index 95% rename from ngraph/test/models/onnx/external_data_optional_fields.prototxt rename to ngraph/test/models/onnx/external_data/external_data_optional_fields.prototxt index 8438d37..2b194cf 100644 --- a/ngraph/test/models/onnx/external_data_optional_fields.prototxt +++ b/ngraph/test/models/onnx/external_data/external_data_optional_fields.prototxt @@ -41,7 +41,7 @@ graph { name: "A" external_data { key: "location", - value: "../../files/onnx/external_data/tensor_optional_fields.data" + value: "tensors_data/tensor_optional_fields.data" } external_data { key: "offset", diff --git a/ngraph/test/models/onnx/external_data/external_data_sanitize_test.prototxt b/ngraph/test/models/onnx/external_data/external_data_sanitize_test.prototxt new file mode 100644 index 0000000..950b900 --- /dev/null +++ b/ngraph/test/models/onnx/external_data/external_data_sanitize_test.prototxt @@ -0,0 +1,99 @@ +ir_version: 3 +producer_name: "nGraph ONNX Importer" +graph { + node { + output: "B" + op_type: "Constant" + attribute { + name: "value" + t { + dims: 2 + dims: 2 + data_type: 1 + float_data: 1 + float_data: 2 + float_data: 3 + float_data: 4 + name: "const_tensor" + } + type: TENSOR + } + } + node { + input: "A" + input: "B" + output: "X" + name: "add_node1" + op_type: "Add" + } + node { + input: "X" + input: "C" + output: "Y" + name: "add_node2" + op_type: "Add" + } + name: "test_graph" + initializer { + dims: 2 + dims: 2 + data_type: 1 + name: "A" + external_data { + key: "location", + value: "./tensors_data/tensor.data" + } + data_location: 1 + } + input { + name: "A" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } + input { + name: "C" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } + output { + name: "Y" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } +} +opset_import { + version: 4 +} diff --git a/ngraph/test/models/onnx/external_data_two_tensors_data_in_the_same_file.prototxt b/ngraph/test/models/onnx/external_data/external_data_two_tensors_data_in_the_same_file.prototxt similarity index 91% rename from ngraph/test/models/onnx/external_data_two_tensors_data_in_the_same_file.prototxt rename to ngraph/test/models/onnx/external_data/external_data_two_tensors_data_in_the_same_file.prototxt index 3f24f37..2b9b362 100644 --- a/ngraph/test/models/onnx/external_data_two_tensors_data_in_the_same_file.prototxt +++ b/ngraph/test/models/onnx/external_data/external_data_two_tensors_data_in_the_same_file.prototxt @@ -15,7 +15,7 @@ graph { name: "data_a" external_data { key: "location", - value: "../../files/onnx/external_data/multiple_tensors.data" + value: "tensors_data/multiple_tensors.data" } external_data { key: "offset", @@ -33,7 +33,7 @@ graph { name: "data_b" external_data { key: "location", - value: "../../files/onnx/external_data/multiple_tensors.data" + value: "tensors_data/multiple_tensors.data" } external_data { key: "offset", diff --git a/ngraph/test/models/onnx/external_data/inner_scope/external_data_file_in_up_dir.prototxt b/ngraph/test/models/onnx/external_data/inner_scope/external_data_file_in_up_dir.prototxt new file mode 100644 index 0000000..aa98b3b --- /dev/null +++ b/ngraph/test/models/onnx/external_data/inner_scope/external_data_file_in_up_dir.prototxt @@ -0,0 +1,82 @@ +ir_version: 3 +producer_name: "nGraph ONNX Importer" +graph { + node { + input: "A" + input: "B" + output: "Y" + name: "add" + op_type: "Add" + } + name: "test_graph" + initializer { + dims: 2 + dims: 2 + data_type: 1 + name: "A" + external_data { + key: "location", + value: "../tensors_data/tensor.data" + } + external_data { + key: "offset", + value: "4096" + } + external_data { + key: "length", + value: "16" + } + data_location: 1 + } + input { + name: "A" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } + input { + name: "B" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } + output { + name: "Y" + type { + tensor_type { + elem_type: 1 + shape { + dim { + dim_value: 2 + } + dim { + dim_value: 2 + } + } + } + } + } +} +opset_import { + version: 4 +} diff --git a/ngraph/test/files/onnx/external_data/a/tensor_a.data b/ngraph/test/models/onnx/external_data/tensors_data/a/tensor_a.data similarity index 100% rename from ngraph/test/files/onnx/external_data/a/tensor_a.data rename to ngraph/test/models/onnx/external_data/tensors_data/a/tensor_a.data diff --git a/ngraph/test/files/onnx/external_data/b/tensor_b.data b/ngraph/test/models/onnx/external_data/tensors_data/b/tensor_b.data similarity index 100% rename from ngraph/test/files/onnx/external_data/b/tensor_b.data rename to ngraph/test/models/onnx/external_data/tensors_data/b/tensor_b.data diff --git a/ngraph/test/files/onnx/external_data/multiple_tensors.data b/ngraph/test/models/onnx/external_data/tensors_data/multiple_tensors.data similarity index 100% rename from ngraph/test/files/onnx/external_data/multiple_tensors.data rename to ngraph/test/models/onnx/external_data/tensors_data/multiple_tensors.data diff --git a/ngraph/test/files/onnx/external_data/tensor.data b/ngraph/test/models/onnx/external_data/tensors_data/tensor.data similarity index 100% rename from ngraph/test/files/onnx/external_data/tensor.data rename to ngraph/test/models/onnx/external_data/tensors_data/tensor.data diff --git a/ngraph/test/files/onnx/external_data/tensor_optional_fields.data b/ngraph/test/models/onnx/external_data/tensors_data/tensor_optional_fields.data similarity index 100% rename from ngraph/test/files/onnx/external_data/tensor_optional_fields.data rename to ngraph/test/models/onnx/external_data/tensors_data/tensor_optional_fields.data diff --git a/ngraph/test/onnx/onnx_import_external_data.in.cpp b/ngraph/test/onnx/onnx_import_external_data.in.cpp index e9217bc..d8e4380 100644 --- a/ngraph/test/onnx/onnx_import_external_data.in.cpp +++ b/ngraph/test/onnx/onnx_import_external_data.in.cpp @@ -36,7 +36,7 @@ using TestEngine = test::ENGINE_CLASS_NAME(${BACKEND_NAME}); NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data) { const auto function = onnx_import::import_onnx_model( - file_util::path_join(SERIALIZED_ZOO, "onnx/external_data.prototxt")); + file_util::path_join(SERIALIZED_ZOO, "onnx/external_data/external_data.prototxt")); auto test_case = test::TestCase(function); test_case.add_input({1.f, 2.f, 3.f, 4.f}); @@ -47,7 +47,8 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data) NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_from_stream) { - std::string path = file_util::path_join(SERIALIZED_ZOO, "onnx/external_data.prototxt"); + std::string path = + file_util::path_join(SERIALIZED_ZOO, "onnx/external_data/external_data.prototxt"); std::ifstream stream{path, std::ios::in | std::ios::binary}; ASSERT_TRUE(stream.is_open()); const auto function = onnx_import::import_onnx_model(stream, path); @@ -63,8 +64,8 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_from_stream) NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_optinal_fields) { - const auto function = onnx_import::import_onnx_model( - file_util::path_join(SERIALIZED_ZOO, "onnx/external_data_optional_fields.prototxt")); + const auto function = onnx_import::import_onnx_model(file_util::path_join( + SERIALIZED_ZOO, "onnx/external_data/external_data_optional_fields.prototxt")); auto test_case = test::TestCase(function); test_case.add_input({1.f, 2.f, 3.f, 4.f}); @@ -75,8 +76,8 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_optinal_fields) NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_in_different_paths) { - auto function = onnx_import::import_onnx_model( - file_util::path_join(SERIALIZED_ZOO, "onnx/external_data_different_paths.prototxt")); + auto function = onnx_import::import_onnx_model(file_util::path_join( + SERIALIZED_ZOO, "onnx/external_data/external_data_different_paths.prototxt")); auto test_case = test::TestCase(function); // first input: {3.f}, second: {1.f, 2.f, 5.f} read from external files @@ -89,7 +90,8 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_in_different_paths) NGRAPH_TEST(${BACKEND_NAME}, onnx_external_two_tensors_data_in_the_same_file) { auto function = onnx_import::import_onnx_model(file_util::path_join( - SERIALIZED_ZOO, "onnx/external_data_two_tensors_data_in_the_same_file.prototxt")); + SERIALIZED_ZOO, + "onnx/external_data/external_data_two_tensors_data_in_the_same_file.prototxt")); auto test_case = test::TestCase(function); // first input: {3, 2, 1}, second: {1, 2, 3} read from external file @@ -103,8 +105,8 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_invalid_external_data_exception) { try { - auto function = onnx_import::import_onnx_model( - file_util::path_join(SERIALIZED_ZOO, "onnx/external_data_file_not_found.prototxt")); + auto function = onnx_import::import_onnx_model(file_util::path_join( + SERIALIZED_ZOO, "onnx/external_data/external_data_file_not_found.prototxt")); FAIL() << "Incorrect path to external data not detected"; } catch (const ngraph_error& error) @@ -119,3 +121,37 @@ NGRAPH_TEST(${BACKEND_NAME}, onnx_external_invalid_external_data_exception) FAIL() << "Importing onnx model failed for unexpected reason"; } } + +NGRAPH_TEST(${BACKEND_NAME}, onnx_external_invalid_up_dir_path) +{ + try + { + auto function = onnx_import::import_onnx_model(file_util::path_join( + SERIALIZED_ZOO, + "onnx/external_data/inner_scope/external_data_file_in_up_dir.prototxt")); + FAIL() << "Incorrect path to external data not detected"; + } + catch (const ngraph_error& error) + { + EXPECT_PRED_FORMAT2(testing::IsSubstring, + std::string("tensor.data, offset: 4096, " + "data_lenght: 16, sha1_digest: 0)"), + error.what()); + } + catch (...) + { + FAIL() << "Importing onnx model failed for unexpected reason"; + } +} + +NGRAPH_TEST(${BACKEND_NAME}, onnx_external_data_sanitize_path) +{ + const auto function = onnx_import::import_onnx_model(file_util::path_join( + SERIALIZED_ZOO, "onnx/external_data/external_data_sanitize_test.prototxt")); + + auto test_case = test::TestCase(function); + test_case.add_input({1.f, 2.f, 3.f, 4.f}); + test_case.add_expected_output(Shape{2, 2}, {3.f, 6.f, 9.f, 12.f}); + + test_case.run(); +} -- 2.7.4