fix: CWE-134 format string vulnerability and compiler flags audit #98
Reference in New Issue
Block a user
No description provided.
Delete Branch "fix/cwe-134-format-string-audit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Compiler flags hardening, CWE-134 format string audit, and bug fixes discovered by new warnings.
Compiler flags (configure.ac)
-Wformat,-Wformat-nonliteral,-Wformat-security-Wextra,-Wundef,-Wfloat-equal,-Wredundant-decls-Wnull-dereference,-Wpointer-arith,-Wimplicit-function-declaration-fstack-protector-strong,-fno-common,-D_FORTIFY_SOURCE=2-Wlogical-op,-Wduplicated-cond,-Wduplicated-branches,-Wstringop-overflow,-Warray-bounds=2,-Walloc-zero-Wno-unused-parameter,-Wno-missing-field-initializers,-Wno-sign-compare,-Wno-cast-function-typeFormat annotations
Added
G_GNUC_PRINTFto variadic functions inui.h,log.h,http_common.h— enables compile-time format string validation.Security fix
iq.c: User-controllederror_messagefrom XMPP stanza passed directly as format string tocons_show_error()— now wrapped with"%s".Ran terminal command: wc -l check-cwe134.sh && head -30 check-cwe134.sh
CWE-134 script (check-cwe134.sh)
Replaced the old regex-based heuristic scanner with a two-stage approach:
cons_show,win_println,log_*, etc.) haveG_GNUC_PRINTF/__attribute__((format))annotations. Without the annotation,gcc -Wformat-nonliteralsilently ignores format misuse in those functions.gcc -Wformat=2 -Werror(now enabled in configure.ac) to catch actual format string violations at build time.This eliminates false positives from the old grep-based patterns and catches real issues the old script missed (e.g. the
iq.csecurity fix above was found this way).Bug fixes found by new warnings
chatlog.cresourcepartinstead ofNULLasfrom_jid-Wduplicated-branchesrosterwin.c-Wduplicated-branchesomemo.celseduplicatedifbody-Wduplicated-branchesconsole.c> 0instead of!= NULL-Wextrastanza.cpri_str/idle_strbuffers too small forINT_MIN(10 → 12 bytes)-Wformat-truncationFormat mismatch fixes
chatwin.cJid*passed to%sinstead ofchar*connection.c%xforlongflags →%lx%dforsize_t→%zu;gpointer→(char*)castcmd_defs.c%dforg_list_length()(guint) →%uiq.cfrom_jid->barejid→from_jid->fulljid(matching intent)console.c,mucwin.c,privwin.c,account.c,omemo.c,presence.cgpointer→(char*)casts for%sCleanup
form.c, database.c,muc.c/h,common.c,xmpp.hconstqualifiers addedvcard.c,files.cui.h,connection.h,omemo.hclang-formatalignmentapi.clog_warning("%s", "msg %s", var)— extra wrapper caused args to be ignoredintvalargument fromcons_show()test_common.cCloses #85
fix: CWE-134 format string vulnerability auditto WIP: fix: CWE-134 format string vulnerability audit60e8b17228to4ef3bc8e46WIP: fix: CWE-134 format string vulnerability auditto WIP: fix: CWE-134 format string vulnerability and compiler flags auditWIP: fix: CWE-134 format string vulnerability and compiler flags auditto fix: CWE-134 format string vulnerability and compiler flags auditReally nice change overall. I have some question marks in certain places + wording of commit message (
(INT_MIN = -2147483648 needs 12 bytes including NUL)(instead ofNULL-terminator) slightly confused me. Nonetheless, great job. Just fix the minor issues that were pointed out and it's ready to merge.@@ -36,0 +75,4 @@# Find variadic declarations with a const char* parameter followed by ...)# that do NOT have a format attribute on the preceding lineNEW_ISSUES=$(grep -B1 -rn --include="*.h" \'const char\s*\*.*,\s*\.\.\.)' "$DIR" 2>/dev/null \What about
gchar?added
@@ -984,3 +984,3 @@if (itemcount == 0 && prefs_get_boolean(PREF_ROSTER_COUNT_ZERO)) {g_string_append_printf(header, " (%d)", itemcount);} else {} else if (itemcount > 0) {It doesn't make sense to do it this way. Condition is still duplicated.
Something like this would be more concise.
corrected
@@ -1029,3 +1029,3 @@if (itemcount == 0 && prefs_get_boolean(PREF_ROSTER_COUNT_ZERO)) {g_string_append_printf(header, " (%d)", itemcount);} else {} else if (itemcount > 0) {same as above
corrected
@@ -529,3 +523,1 @@for (device = xmpp_stanza_get_children(list); device != NULL; device = xmpp_stanza_get_next(device)) {if (g_strcmp0(xmpp_stanza_get_name(device), "device") != 0) {continue;/* New scope to keep declarations after goto out above */Again comment style issue. Also I do not see a point in this scope
Refactored
@@ -516,3 +516,3 @@GSList* currb = b;while (curra) {while (curra && currb) {Condition above
if (g_slist_length(a) != g_slist_length(b)) {should normally prevent this scenario. It would be strange if one list is going to finish earlier than another one.original code invoke error: potential null pointer dereference [-Werror=null-dereference]
int bval = GPOINTER_TO_INT(currb->data);
I guess we can call is a defensive programming.
d103b32025to872b2bfeacsuggested squash commit:
fix: CWE-134 format string audit and compiler hardening
Security:
in ui.h, log.h and http_common.h
Compiler flags (configure.ac):
-Wimplicit-function-declaration, -Wundef, -Wfloat-equal,
-Wredundant-decls, -Walloc-zero
-Wduplicated-cond, -Wduplicated-branches, -Wstringop-overflow,
-Warray-bounds=2
-Wno-missing-field-initializers, -Wno-sign-compare,
-Wno-cast-function-type
Bug fixes found by new warnings:
unnecessary scope block and goto, use early return
(INT_MIN = -2147483648 needs 12 bytes including NUL)
Format mismatch fixes:
gpointer → (char*) casts for %s
Const-correctness and cleanup:
connection_set_priority (connection.h),
omemo_devicelist_configure_and_request (omemo.h)
Tooling (check-cwe134.sh):
872b2bfeactod081993c90