Fix security warning generated by std::tmpnam #105987

Sergey Sharybin · 2023-03-22T12:02:08+01:00

Sergey Sharybin commented

2023-03-22 12:02:08 +01:00

Effectively replicate the behavior of the function in the manner
which is used for autosave file.

There might be better a solution which is cross-platform and does
not suffer from the time of check, time of use (TOCTOU) vector of
attack. This seems to be a bigger project to figure out, so until
then silence the warning: it is fine since the directory is only
used to chdir to, so worst case an external attacker can introduce
is a test failure.

Effectively replicate the behavior of the function in the manner which is used for autosave file. There might be better a solution which is cross-platform and does not suffer from the time of check, time of use (TOCTOU) vector of attack. This seems to be a bigger project to figure out, so until then silence the warning: it is fine since the directory is only used to chdir to, so worst case an external attacker can introduce is a test failure.

Sergey Sharybin added the

Module

Core

label 2023-03-22 12:02:08 +01:00

Sergey Sharybin added 1 commit 2023-03-22 12:02:18 +01:00

buildbot/vexp-code-patch-coordinator Build done. Details

dcb7b7c69f Fix security warning generated by std::tmpnam

Effectively replicate the behavior of the function in the manner
which is used for autosave file.

There might be better a solution which is cross-platform and does
not suffer from the time of check, time of use (TOCTOU) vector of
attack. This seems to be a bigger project to figure out, so until
then silence the warning: it is fine since the directory is only
used to chdir to, so worst case an external attacker can introduce
is a test failure.

Sergey Sharybin commented

2023-03-22 12:02:28 +01:00

@blender-bot build

Sergey Sharybin added this to the Core project 2023-03-22 12:02:37 +01:00

Sergey Sharybin requested review from Brecht Van Lommel 2023-03-22 15:00:51 +01:00

Brecht Van Lommel commented

2023-03-22 15:34:56 +01:00

This is the first usage of std::filesystem in Blender. We can use that now with macOS 10.15 as the minimum, however there were also other concerns in #90379.

In particular, the BLI API assumes utf-8 but std::filesystem does not on Windows, so this may fail if the temp directory includes non-ascii characters.

This is the first usage of `std::filesystem` in Blender. We can use that now with macOS 10.15 as the minimum, however there were also other concerns in #90379. In particular, the BLI API assumes utf-8 but `std::filesystem` does not on Windows, so this may fail if the temp directory includes non-ascii characters.

Brecht Van Lommel commented

2023-03-22 15:38:10 +01:00

Maybe it's easier to not use a temporary directory, but use a fixed one like tests/fileops similar to tests/cycles or tests/io_curve_svg.

Maybe it's easier to not use a temporary directory, but use a fixed one like `tests/fileops` similar to `tests/cycles` or `tests/io_curve_svg`.

Sergey Sharybin added 1 commit 2023-03-22 16:57:30 +01:00

9fbcc950f5 Avoid use of system-wide temporary directory

Use temporary directory provided via the command line flags,
which points to a directory within a build folder.

Brecht Van Lommel approved these changes 2023-03-22 17:08:38 +01:00

Sergey Sharybin commented

2023-03-22 17:22:08 +01:00

I am not sure why it is considered to be important how the fs::path stores data internally. It stores the data in the native to the OS manner, which avoids conversion to the wide character format on every file system access (which is required when one uses UTF-8 for the storage).

In the issue you've linked the statement goes by "needs investigation", so not sure how that is escalated to a "concern". In practice there are some tricky aspects of getting UTF-8 path to fs::path, however those are non-intuitive mainly C++20. In C++17 the u8path does the proper thing in a cross-platform manner (at least in own tests). The tricky part is the C++20 which declares u8path deprecated and tells to use path(u8string) constructor signature, and that did not seem to work correctly. The access to the UTF-8 string is something that generic_u8string() (and that is something it seems I've missed in the original patch).

Interestingly, there is no difference in the behavior of std::tmpnam() and fs::temp_directory_path(): they both encode non-ASCII part of a path in some coding scheme, and it does not matter whether generic_string() or generic_u8string() was used.

I've updated the patch so that it avoids use of std::filesystem, but I do not think it solves more problems than introduces new. The comparison with the io_curve_svg I do not find really fair: it uses tests/io_curve_svg to store data which is considered an output. For the temporary files it still uses Python's tempfile.

Adding such explicit temporary directory specification makes it tricky to understand what is to be used and when. Like, why the fileops test has to use this explicit temporary directory, but the PLY test can use the generic Blender temporary directory.

And last but not least, with such change it is required to pass an extra command line argument to the test so that ./bin/tests/blenlib_test --gtest_filter='*change_working_directory' is no longer enough (and keeping tests as easy to run as possible is something I find important).

The proper solution could be to move where_is_temp to BLI. It provides close-to-XDG way of accessing the temp directory, and is not really Blender application specific, so it is not really justified to be in the BKE. Doing so also moves us closer to the more proper and transparent substitute of the std::filepath.

So if there is really requirement of avoiding std::filesystem the proper solution would require to first un-entangle some extra mess in the BKE/BLI. It should happen as a separate review. As for this change, I really do not feel motivated committing this mess, so closing.

I am not sure why it is considered to be important how the `fs::path` stores data internally. It stores the data in the native to the OS manner, which avoids conversion to the wide character format on every file system access (which is required when one uses UTF-8 for the storage). In the issue you've linked the statement goes by "needs investigation", so not sure how that is escalated to a "concern". In practice there are some tricky aspects of getting UTF-8 path to `fs::path`, however those are non-intuitive mainly C++20. In C++17 the `u8path` does the proper thing in a cross-platform manner (at least in own tests). The tricky part is the C++20 which declares `u8path` deprecated and tells to use `path(u8string)` constructor signature, and that did not seem to work correctly. The access to the UTF-8 string is something that `generic_u8string()` (and that is something it seems I've missed in the original patch). Interestingly, there is no difference in the behavior of `std::tmpnam()` and `fs::temp_directory_path()`: they both encode non-ASCII part of a path in some coding scheme, and it does not matter whether `generic_string()` or `generic_u8string()` was used. I've updated the patch so that it avoids use of `std::filesystem`, but I do not think it solves more problems than introduces new. The comparison with the `io_curve_svg` I do not find really fair: it uses `tests/io_curve_svg` to store data which is considered an output. For the temporary files it still uses Python's tempfile. Adding such explicit temporary directory specification makes it tricky to understand what is to be used and when. Like, why the fileops test has to use this explicit temporary directory, but the PLY test can use the generic Blender temporary directory. And last but not least, with such change it is required to pass an extra command line argument to the test so that `./bin/tests/blenlib_test --gtest_filter='*change_working_directory'` is no longer enough (and keeping tests as easy to run as possible is something I find important). The proper solution could be to move `where_is_temp` to BLI. It provides close-to-XDG way of accessing the temp directory, and is not really Blender application specific, so it is not really justified to be in the BKE. Doing so also moves us closer to the more proper and transparent substitute of the `std::filepath`. So if there is really requirement of avoiding `std::filesystem` the proper solution would require to first un-entangle some extra mess in the BKE/BLI. It should happen as a separate review. As for this change, I really do not feel motivated committing this mess, so closing.

Sergey Sharybin closed this pull request

2023-03-22 17:22:09 +01:00

Sergey Sharybin removed this from the Core project 2023-03-23 11:21:05 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix security warning generated by std::tmpnam #105987

Pull request closed