Open hjmjohnson opened 2 months ago
Removing this feature is likely to just push it into the application code, for the applications that already use it. And it is a non-issue for most applications. Leaving it in, makes it easy for applications to expose this security vulnerability to the end-user. But how much of an issue is this? I think this issue should be a low priority.
This issue only applies to m_SeriesFormat
, right? As far as I can see, that's the only case where the user is allowed to specify the formatting by a printf
-like string.
The least we could do is add a warning/note to m_SeriesFormat
and its setter.
In the future, we could maybe replace it with a C++20 std::format based implementation.
Removing this feature is likely to just push it into the application code, for the applications that already use it.
Depends, in user's application code, it might be a compile-time constant.
This issue only applies to m_SeriesFormat, right? As far as I can see, that's the only case where the user is allowed to specify the formatting by a printf-like string.
Yes, only one I see too. And the only warnings here too: https://open.cdash.org/viewBuildError.php?type=1&buildid=9579695
In the future, we could maybe replace it with a C++20 std::format based implementation.
How would that help?
In the future, we could maybe replace it with a C++20 std::format based implementation.
How would that help?
Correction: I should say std::vformat
, not std::format
! std::vformat
checks that the passed parameters match with the format string. It prevents hackers from accessing arbitrary memory through the format string.
snprintf(fileName, IOCommon::ITK_MAXPATHLEN + 1, m_SeriesFormat.c_str(), fileNumber);
Could then (when using C++20) be replaced with something like:
std::string fileName = std::vformat(m_SeriesFormatString, fileNumber);
Ah that looks promising. I guess it could even be done today, wrapped in #if __cplusplus > xyz
and falling back to snprintf
in the #else
.
Ah that looks promising. I guess it could even be done today, wrapped in
#if __cplusplus > xyz
and falling back tosnprintf
in the#else
.
Well, ideally yes. But the format of std::format
/std::vformat
is different from the old printf/snprintf "%d"
format. So it would be an API change 🤷 .
2 cents: For SimpleITK we did not expose these methods and pushed the generation or globing to scripting language. Other languages seem more expressive, and less error prone for these types of operations.
I guess that will be easier from C++ too, when C++20 is available.
Description
Allowing users to specify formatting strings at runtime is a well known exploitable code security vulnerability.
We currently suppress these warnings, but it would be better to re-write the codebase to avoid the security vulnerability all together.
Steps to Reproduce
Expected behavior
No warning suppression and no security vulnerability.
Actual behavior
When ITK_GCC_SUPPRESS_Wformat_nonliteral supression are disabled, warnings are issued.
Reproducibility
New compilers, and requesting -Wformat-nonliteral
Versions
Since the earliest versions of ITK to at least 2024-04-29
Additional Information
https://github.com/InsightSoftwareConsortium/ITK/pull/4616#discussion_r1583057823