nih-at
Address potential buffer overrun issues
Changes:
- Use more secure functions (e.g. *_s functions) for Microsoft CRT implementations that support secure libraries.
- Fixed possible buffer overrun issues by ensuring that allocation’s inputs don’t suffer from integer overflow or truncation (or just add a cast when the target variable cannot be truncated).
Could you please explain what these secure functions are? Are they only available on Windows?
Microsoft is now requiring '/sdl' flag for all internal products and the '/sdl' flag on the Microsoft toolchain requires the *_s functions. https://docs.microsoft.com/en-us/cpp/build/reference/sdl-enable-additional-security-checks?view=msvc-170
The *_s functions are just versions of the original functions with security enhancements. https://docs.microsoft.com/en-us/cpp/c-runtime-library/security-features-in-the-crt?view=msvc-170
I see, so this is worth supporting.
I see three possibilities:
- Keep using the legacy functions in libzip code and map them to their _s counterparts via defines. This kind of defeats the point, since they don't have the relevant information for buffer sizes.
- Use the _s functions in libzip and add a compatibility layer for platforms without SDL. a. Map them via defines. b. Implement them, including their additional checks.
- Create a more generic abstraction to use in libzip and implement that in terms of the _s or legacy functions, depending on availability. This should also handle similar enhancements on other platforms.
The main libzip source should use one set of functions constantly and remain readable, with a minimum of #ifdefs.
To decide which is best, I would need to know which functions have a _s counterpart, and what the additional arguments are. Do you have a list of all affected functions and their _s counterparts?
I do not have a complete list but here are the main ones that showed up for us as we looked through this repo with links to documentation for their _s counterparts: memcpy, strcpy, sprintf, strerror, and _snwprintf
Since these secure functions are part of ISO C 11, we decided to use them and add compatibility defines for systems without them.
Could you please check if we still use functions that are forbidden?
We also need localtime_s and strcpy_s. We also do not allow fdopen and some of the values need to be cast as size_t. Thank you!
We no longer use localtime or strcpy.
You can disable the use of fdopen by running cmake with -DENABLE_FDOPEN=OFF.
I've seen you have replaced fdopen with _tfdopen, is that allowed? Is _fdopen also allowed, because that's what we're using on Windows (see compat.h line 95).
We should now handle situations cleanly where size_t is less than 64 bits. If you see any other places where that might cause a problem, please let us know.