String manipulation security: Remove snprintf with user defined formatting options. about itk HOT 8 OPEN

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.

from itk.

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.

from itk.

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?

from itk.

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.

Looking at

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);

from itk.

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.

from itk.

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.

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 🤷 .

from itk.

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.

from itk.

I guess that will be easier from C++ too, when C++20 is available.

from itk.