From f6814c97c105ba258a8b1765f28e4d323a0f7f52 Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Mon, 1 Sep 2014 15:14:55 -0400 Subject: [PATCH] Reduce risk level to 0 of snprintf with constant format string - snprintf is a useful *countermeasure* for buffer overflows, and unlike some alternatives it is standard and *widely* available. (strlcpy/strlcat are useful but not standard and not widely available; snprintf_s is standard but not widely available). Historically we warned about snprintf because old systems didn't implement it correctly, but at this point these old systems are more historical than anything else. Instead, let's specifically *mention* snprintf as a recommended potential solution for buffer overflows. --- correct-results.html | 25 +++++++++++++------------ correct-results.txt | 33 +++++++++++++++++---------------- flawfinder | 31 +++++++++++++------------------ 3 files changed, 43 insertions(+), 46 deletions(-) diff --git a/correct-results.html b/correct-results.html index 1f9bfb3..adebb31 100644 --- a/correct-results.html +++ b/correct-results.html @@ -11,7 +11,7 @@ Here are the security scan results from Flawfinder version 1.32, (C) 2001-2014 David A. Wheeler. -Number of rules (primarily dangerous function names) in C/C++ ruleset: 188 +Number of rules (primarily dangerous function names) in C/C++ ruleset: 209

Examining test.c
Examining test2.c
@@ -28,17 +28,17 @@ Examining test2.c

  • test.c:56: [5] (buffer) strncat: Easily used incorrectly (e.g., incorrectly computing the correct maximum - size to add) (CWE-120). - Consider strcat_s, strlcat, or automatically resizing strings. Risk is - high; the length parameter appears to be a constant, instead of computing - the number of characters left. + Consider strcat_s, strlcat, snprintf, or automatically resizing strings. + Risk is high; the length parameter appears to be a constant, instead of + computing the number of characters left. 
       strncat(d,s,sizeof(d)); /* Misuse - this should be flagged as riskier. */
  • test.c:57: [5] (buffer) _tcsncat: Easily used incorrectly (e.g., incorrectly computing the correct maximum - size to add) (CWE-120). Consider strcat_s, strlcat, or automatically resizing strings. Risk is high; the length parameter appears to be a constant, instead of computing @@ -79,7 +79,7 @@ Examining test2.c
  • test.c:17: [4] (buffer) strcpy: Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120). - Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily + Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused). 
      strcpy(b, a);
@@ -147,7 +147,8 @@ Examining test2.c 
    
   _mbscpy(d,s); /* like strcpy, this doesn't check for buffer overflow */
  • test.c:52: [4] (buffer) lstrcat: - Does not check for buffer overflows when concatenating to destination (CWE-120). 
       lstrcat(d,s);
@@ -181,7 +182,7 @@ Examining test2.c 
    
 
  • test.c:16:   [2]  (buffer)  strcpy:
   Does not check for buffer overflows when copying to destination [MS-banned]
   (CWE-120).
-  Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily
+  Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily
   misused). Risk is low because the source is a constant string. 
 
      strcpy(a, gettext("Hello there")); // Did this work?
@@ -238,7 +239,7 @@ Examining test2.c 
    
 
  • test.c:15:   [1]  (buffer)  strcpy:
   Does not check for buffer overflows when copying to destination [MS-banned]
   (CWE-120).
-  Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily
+  Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily
   misused). Risk is low because the source is a constant character. 
 
      strcpy(a, "\n"); // Did this work?
@@ -274,9 +275,9 @@ Examining test2.c 
    
 
    
 
  • test.c:55:   [1]  (buffer)  strncat:
   Easily used incorrectly (e.g., incorrectly computing the correct maximum
-  size to add) (CWE-120).
-  Consider strcat_s, strlcat, or automatically resizing strings. 
+  Consider strcat_s, strlcat, snprintf, or automatically resizing strings. 
 
       strncat(d,s,10);
 
    
diff --git a/correct-results.txt b/correct-results.txt
index 40d3574..b8bcb20 100644
--- a/correct-results.txt
+++ b/correct-results.txt
@@ -1,5 +1,5 @@
 Flawfinder version 1.32, (C) 2001-2014 David A. Wheeler.
-Number of rules (primarily dangerous function names) in C/C++ ruleset: 188
+Number of rules (primarily dangerous function names) in C/C++ ruleset: 209
 Examining test.c
 Examining test2.c
 
@@ -9,14 +9,15 @@ test.c:32:  [5] (buffer) gets:
   Does not check for buffer overflows (CWE-120, CWE-20). Use fgets() instead.
 test.c:56:  [5] (buffer) strncat:
   Easily used incorrectly (e.g., incorrectly computing the correct maximum
-  size to add) (CWE-120). Consider strcat_s, strlcat, or automatically
-  resizing strings. Risk is high; the length parameter appears to be a
-  constant, instead of computing the number of characters left.
+  size to add) [MS-banned] (CWE-120). Consider strcat_s, strlcat, snprintf,
+  or automatically resizing strings. Risk is high; the length parameter
+  appears to be a constant, instead of computing the number of characters
+  left.
 test.c:57:  [5] (buffer) _tcsncat:
   Easily used incorrectly (e.g., incorrectly computing the correct maximum
-  size to add) (CWE-120). Consider strcat_s, strlcat, or automatically
-  resizing strings. Risk is high; the length parameter appears to be a
-  constant, instead of computing the number of characters left.
+  size to add) [MS-banned] (CWE-120). Consider strcat_s, strlcat, or
+  automatically resizing strings. Risk is high; the length parameter appears
+  to be a constant, instead of computing the number of characters left.
 test.c:60:  [5] (buffer) MultiByteToWideChar:
   Requires maximum length in CHARACTERS, not bytes (CWE-120). Risk is high,
   it appears that the size is given as bytes, but the function requires size
@@ -33,8 +34,8 @@ test.c:73:  [5] (misc) SetSecurityDescriptorDacl:
   Access), which would even forbid administrator access (CWE-732).
 test.c:17:  [4] (buffer) strcpy:
   Does not check for buffer overflows when copying to destination [MS-banned]
-  (CWE-120). Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy
-  is easily misused).
+  (CWE-120). Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy
+  easily misused).
 test.c:20:  [4] (buffer) sprintf:
   Does not check for buffer overflows (CWE-120). Use sprintf_s, snprintf, or
   vsnprintf.
@@ -63,7 +64,7 @@ test.c:49:  [4] (buffer) _mbscpy:
   of the buffer.
 test.c:52:  [4] (buffer) lstrcat:
   Does not check for buffer overflows when concatenating to destination
-  (CWE-120).
+  [MS-banned] (CWE-120).
 test.c:75:  [3] (shell) CreateProcess:
   This causes a new process to execute and is difficult to use safely
   (CWE-78). Specify the application path in the first argument, NOT as part
@@ -80,8 +81,8 @@ test.c:91:  [3] (buffer) getopt_long:
   of all string inputs.
 test.c:16:  [2] (buffer) strcpy:
   Does not check for buffer overflows when copying to destination [MS-banned]
-  (CWE-120). Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy
-  is easily misused). Risk is low because the source is a constant string.
+  (CWE-120). Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy
+  easily misused). Risk is low because the source is a constant string.
 test.c:19:  [2] (buffer) sprintf:
   Does not check for buffer overflows (CWE-120). Use sprintf_s, snprintf, or
   vsnprintf. Risk is low because the source has a constant maximum length.
@@ -108,8 +109,8 @@ test.c:97:  [2] (misc) fopen:
   contents? (CWE-362).
 test.c:15:  [1] (buffer) strcpy:
   Does not check for buffer overflows when copying to destination [MS-banned]
-  (CWE-120). Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy
-  is easily misused). Risk is low because the source is a constant character.
+  (CWE-120). Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy
+  easily misused). Risk is low because the source is a constant character.
 test.c:18:  [1] (buffer) sprintf:
   Does not check for buffer overflows (CWE-120). Use sprintf_s, snprintf, or
   vsnprintf. Risk is low because the source is a constant character.
@@ -125,8 +126,8 @@ test.c:54:  [1] (buffer) _tcsncpy:
   pointers [MS-banned] (CWE-120).
 test.c:55:  [1] (buffer) strncat:
   Easily used incorrectly (e.g., incorrectly computing the correct maximum
-  size to add) (CWE-120). Consider strcat_s, strlcat, or automatically
-  resizing strings.
+  size to add) [MS-banned] (CWE-120). Consider strcat_s, strlcat, snprintf,
+  or automatically resizing strings.
 test.c:58:  [1] (buffer) strlen:
   Does not handle strings that are not \0-terminated; if given one it may
   perform an over-read (it could cause a crash if unprotected) (CWE-126).
diff --git a/flawfinder b/flawfinder
index 405db48..c8decca 100755
--- a/flawfinder
+++ b/flawfinder
@@ -597,18 +597,13 @@ def c_printf(hit):
     source = strip_i18n(hit.parameters[format_position])
     if c_constant_string(source):
       # Parameter is constant, so there's no risk of format string problems.
-      if hit.name == "snprintf" or hit.name == "vsnprintf":
-        hit.level = 1
-        hit.warning = \
-          "On some very old systems, snprintf is incorrectly implemented " \
-          "and permits buffer overflows; there are also incompatible " \
-          "standard definitions of it"
-        hit.suggestion = "Check it during installation, or use something else"
-        hit.category = "port"
-      else:
-        # We'll pass it on, just in case it's needed, but at level 0 risk.
-        hit.level = 0
-        hit.note = "Constant format string, so not considered very risky (there's some residual risk, especially in a loop)."
+      # At one time we warned that very old systems sometimes incorrectly
+      # allow buffer overflows on snprintf/vsnprintf, but those systems
+      # are now very old, and snprintf is an important potential tool for
+      # countering buffer overflows.
+      # We'll pass it on, just in case it's needed, but at level 0 risk.
+      hit.level = 0
+      hit.note = "Constant format string, so not considered risky."
   add_warning(hit)
 
 
@@ -740,7 +735,7 @@ c_ruleset = {
   "strcpy" :
      (c_buffer, 4,
       "Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120)",
-      "Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily misused)",
+      "Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused)",
       "buffer", "", {}),
   "strcpyA|strcpyW|StrCpy|StrCpyA|lstrcpyA|lstrcpyW|_tccpy|_mbccpy|_ftcscpy|_mbsncpy|StrCpyN|StrCpyNA|StrCpyNW|StrNCpy|strcpynA|StrNCpyA|StrNCpyW|lstrcpynA|lstrcpynW" :
       # We need more info on these functions; I got their names from the
@@ -748,7 +743,7 @@ c_ruleset = {
       # instead of "c_buffer".
      (normal, 4,
       "Does not check for buffer overflows when copying to destination [MS-banned] (CWE-120)",
-      "Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily misused)",
+      "Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily misused)",
       "buffer", "", {}),
   "lstrcpy|wcscpy|_tcscpy|_mbscpy" :
      (c_buffer, 4,
@@ -763,7 +758,7 @@ c_ruleset = {
   "strcat" :
      (c_buffer, 4,
       "Does not check for buffer overflows when concatenating to destination [MS-banned] (CWE-120)",
-      "Consider using strcat_s, strncat, or strlcat (warning, strncat is easily misused)",
+      "Consider using strcat_s, strncat, strlcat, or snprintf (warning: strncat is easily misused)",
       "buffer", "", {}),
   "lstrcat|wcscat|_tcscat|_mbscat" :
      (c_buffer, 4,
@@ -798,7 +793,7 @@ c_ruleset = {
          # FIXING security problems, and raising it to a
          # higher risk level would cause many false positives.
       "Easily used incorrectly (e.g., incorrectly computing the correct maximum size to add) [MS-banned] (CWE-120)",
-      "Consider strcat_s, strlcat, or automatically resizing strings",
+      "Consider strcat_s, strlcat, snprintf, or automatically resizing strings",
       "buffer", "", {}),
   "lstrcatn|wcsncat|_tcsncat|_mbsnbcat" :
      (c_strncat,
@@ -874,8 +869,8 @@ c_ruleset = {
 
   "strlen|wcslen|_tcslen|_mbslen" :
      (normal,
-      1, # Often this isn't really a risk, and even when, it usually at worst causes
-      # program crash (and nothing worse).
+      1, # Often this isn't really a risk, and even when it is, at worst it
+         # often causes a program crash (and nothing worse).
       "Does not handle strings that are not \\0-terminated; " +
       "if given one it may perform an over-read (it could cause a crash " +
          "if unprotected) (CWE-126)",