walkero
/
flawfinder
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

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.
This commit is contained in:
David A. Wheeler 2014-09-01 15:14:55 -04:00
parent 6031b31f8c
commit f6814c97c1
3 changed files with 43 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
correct-results.html
View File

@ -11,7 +11,7 @@
Here are the security scan results from
<a href="http://www.dwheeler.com/flawfinder">Flawfinder version 1.32</a>,
(C) 2001-2014 <a href="http://www.dwheeler.com">David A. Wheeler</a>.
Number of rules (primarily dangerous function names) in C/C++ ruleset: 188
Number of rules (primarily dangerous function names) in C/C++ ruleset: 209
<p>
Examining test.c <br>
Examining test2.c <br>
@ -28,17 +28,17 @@ Examining test2.c <br>
</pre>
<li>test.c:56: <b> [5] </b> (buffer) <i> strncat:
Easily used incorrectly (e.g., incorrectly computing the correct maximum
size to add) (<a
size to add) [MS-banned] (<a
href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
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. </i>
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. </i>
<pre>
strncat(d,s,sizeof(d)); /* Misuse - this should be flagged as riskier. */
</pre>
<li>test.c:57: <b> [5] </b> (buffer) <i> _tcsncat:
Easily used incorrectly (e.g., incorrectly computing the correct maximum
size to add) (<a
size to add) [MS-banned] (<a
href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
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 <br>
<li>test.c:17: <b> [4] </b> (buffer) <i> strcpy:
Does not check for buffer overflows when copying to destination [MS-banned]
(<a href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
Consider using strcpy_s, strncpy, or strlcpy (warning, strncpy is easily
Consider using snprintf, strcpy_s, or strlcpy (warning: strncpy easily
misused). </i>
<pre>
strcpy(b, a);
@ -147,7 +147,8 @@ Examining test2.c <br>
_mbscpy(d,s); /* like strcpy, this doesn't check for buffer overflow */
</pre>
<li>test.c:52: <b> [4] </b> (buffer) <i> lstrcat:
Does not check for buffer overflows when concatenating to destination (<a
Does not check for buffer overflows when concatenating to destination
[MS-banned] (<a
href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>). </i>
<pre>
lstrcat(d,s);
@ -181,7 +182,7 @@ Examining test2.c <br>
<li>test.c:16: <b> [2] </b> (buffer) <i> strcpy:
Does not check for buffer overflows when copying to destination [MS-banned]
(<a href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
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. </i>
<pre>
strcpy(a, gettext("Hello there")); // Did this work?
@ -238,7 +239,7 @@ Examining test2.c <br>
<li>test.c:15: <b> [1] </b> (buffer) <i> strcpy:
Does not check for buffer overflows when copying to destination [MS-banned]
(<a href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
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. </i>
<pre>
strcpy(a, "\n"); // Did this work?
@ -274,9 +275,9 @@ Examining test2.c <br>
</pre>
<li>test.c:55: <b> [1] </b> (buffer) <i> strncat:
Easily used incorrectly (e.g., incorrectly computing the correct maximum
size to add) (<a
size to add) [MS-banned] (<a
href="http://cwe.mitre.org/data/definitions/120.html">CWE-120</a>).
Consider strcat_s, strlcat, or automatically resizing strings. </i>
Consider strcat_s, strlcat, snprintf, or automatically resizing strings. </i>
<pre>
strncat(d,s,10);
</pre>

33
correct-results.txt
View File

@ -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).

31
flawfinder
View File

@ -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)",