Harden NUT work with strings where dynamic formatting strings are used #2460
+1,044
−1,032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…roduce snprintf_dynamic() and related methods [networkupstools#2450] Mitigate the inherent insecurity of dynamically constructed formatting strings vs. a fixed vararg list with its amounts and types of variables printed by this or that method and pre-compiled in the program. * minimize_formatting_string() with caller-specified buffer; * minimize_formatting_string_staticbuf() for one-shot use-cases; * validate_formatting_string() to compare a dynamic and expected formatting strings; * vsnprintf_dynamic(), vsnprintfcat_dynamic() for practical applications (with fixed va_list argument); * snprintf_dynamic(), snprintfcat_dynamic(), mkstr_dynamic() for practical applications (with ... variadic arguments); * added vsnprintfcat() with fixed va_list argument, for good measure. Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…rsions for hardened dynamic format string support [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…rdened *_dynamic() string methods [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…llapse known different formats for same data type into same char to ease sanity-check comparisons [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
….c: introduce verbosity option to validate_formatting_string() and minimize_formatting_string() [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…valid use-case for vsnprintf() [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
… an unknown model [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…ch to snprintf_dynamic() instead of hushing potential flaws with macros [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…ad of hushing potential flaws with macros [networkupstools#2450] Found by pragmas to clean up, with :; git grep -En 'Wformat-(sec|nonlit)' Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…operly cast the value, and harden with snprintf_dynamic() [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…t the odd conversion, and harden with snprintf_dynamic() [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
….battery.start" might vary by applicable formatting strings [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…witching to snprintf_dynamic() instead of hushing potential flaws with macros [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…ent platforms Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…may produce invalid printf-style strings and not complain (garbage in = garbage out) [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…lerate dynamic formats that are sub-strings and beginnings of reference (wasteful but survivable) [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
jimklimov
added
enhancement
need testing
|
✅ Build nut 2.8.2.1776-master completed (commit 72fc10ba29 by @jimklimov) |
…rmat-extra-args" [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…t-extra-args" in pragmas to quiesce "bogus-looking" test cases [networkupstools#2450] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…x_blazer-common.c: define macros for minimize_formatting_string() and validate_formatting_string() verbosity argument values [networkupstools#2450] Custom builds that do not want to require setting driver/tool debug levels can re-define their NUT_DYNAMICFORMATTING_DEBUG_LEVEL to e.g. 0 and see any formatting discrepancies instantly. Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
|
✅ Build nut 2.8.2.1785-master completed (commit 2cb2457138 by @jimklimov) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #2450
Builds upon the idea suggested in the issue above.
Impacts quite a lot of drivers and other code, so I initially plan on mile-stoning for a later release and asking the community about testing it in practice (after this seems okay with all systems of the NUT CI farm).
One way this can mis-fire is if some mapping tables (potentially) use different formatting strings like
%dand%ldfor some value stored in along-- one of the sources deals with that by checking if the dynamic formatting string normalized to this or that and casts accordingly. Maybe this can be generalized. Test code is prepared for TDD-style development, if it comes to that. (Was already helpful for some scenarios)Some locations were not modified, where varargs are used (at least currently) with fixed formatting strings, and static compilers do not smell anything fishy about this. A few cases can be currently simplified, where error messages etc. are thrown with only the "fixed" formatting string and no varargs. Not sure it should be done though (less flexibility later).
CC @arnaudquette-eaton :
snmp-ups(primarily around daisy-chain support) is one of the impacted code-bases, testing with some ePDUs would be beneficial.CC @ericclappier-eaton : this may impact or benefit your work too, pinging just in case :)