173850 - flawfinder warnings in layout

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Stephen P. Morse]

Heikki ran flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 
branch.

flawfinder found 122 warnings in layout code (3560-3681). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition to checking the branch, also check the trunk.

3560) layout/html/base/src/nsFrame.cpp:4567 [4] (buffer) strcpy: does not check 
for buffer overflows. Consider using strncpy or strlcpy.

3561) layout/html/base/src/nsFrame.cpp:4568 [4] (buffer) strcpy: does not check 
for buffer overflows. Consider using strncpy or strlcpy.

3562) layout/html/base/src/nsFrame.cpp:4966 [2] (buffer) strcpy: does not check 
for buffer overflows. Consider using strncpy or strlcpy. Risk is low because the 
source is a constant string.

3563) layout/html/base/src/nsFrame.cpp:4971 [2] (buffer) strcpy: does not check 
for buffer overflows. Consider using strncpy or strlcpy. Risk is low because the 
source is a constant string.

3564) layout/html/base/src/nsFrame.cpp:4974 [2] (buffer) sprintf: does not check 
for buffer overflows. Use snprintf or vsnprintf. Risk is low because the source 
has a constant maximum length.

3565) layout/html/base/src/nsHTMLReflowState.cpp:2184 [3] (buffer) getenv: 
Environment variables are untrustable input if they can be set by an attacker. 
They can have any content and length, and the same variable can be set more than 
once.. Check environment variables carefully before using them.

3566) layout/html/base/src/nsPageFrame.cpp:91 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3567) layout/html/base/src/nsPageFrame.cpp:92 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3568) layout/html/base/src/nsPageFrame.cpp:93 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3569) layout/html/base/src/nsPageFrame.cpp:94 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3570) layout/html/base/src/nsPageFrame.cpp:95 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3571) layout/html/base/src/nsPageFrame.cpp:596 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3572) layout/html/base/src/nsPageFrame.cpp:597 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3573) layout/html/base/src/nsPageFrame.cpp:598 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3574) layout/html/base/src/nsPresShell.cpp:7565 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3575) layout/html/base/src/nsPresShell.cpp:7593 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3576) layout/html/base/src/nsPresShell.cpp:7604 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3577) layout/html/base/src/nsPresShell.cpp:7733 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3578) layout/html/base/src/nsPresShell.cpp:7851 [4] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy.

3579) layout/html/base/src/nsPresShell.cpp:7856 [2] (buffer) strcat: does not 
check for buffer overflows. Consider using strncat or strlcat. Risk is low 
because the source is a constant string.

3580) layout/html/base/src/nsSimplePageSequence.cpp:94 [4] (format) fprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3581) layout/html/base/src/nsSimplePageSequence.cpp:95 [4] (format) fprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3582) layout/html/base/src/nsSimplePageSequence.cpp:96 [4] (format) fprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3583) layout/html/base/src/nsSimplePageSequence.cpp:97 [4] (format) fprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3584) layout/html/base/src/nsSimplePageSequence.cpp:98 [4] (format) fprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3585) layout/html/forms/src/nsComboboxControlFrame.cpp:220 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3586) layout/html/forms/src/nsComboboxControlFrame.cpp:221 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3587) layout/html/forms/src/nsComboboxControlFrame.cpp:222 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3588) layout/html/forms/src/nsComboboxControlFrame.cpp:223 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3589) layout/html/forms/src/nsComboboxControlFrame.cpp:244 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3590) layout/html/forms/src/nsComboboxControlFrame.cpp:245 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3591) layout/html/forms/src/nsComboboxControlFrame.cpp:246 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3592) layout/html/forms/src/nsComboboxControlFrame.cpp:247 [4] (format) printf: 
if format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3593) layout/html/forms/src/nsFormControlFrame.cpp:88 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3594) layout/html/forms/src/nsFormControlFrame.cpp:89 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3595) layout/html/forms/src/nsFormControlFrame.cpp:90 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3596) layout/html/forms/src/nsFormControlFrame.cpp:91 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3597) layout/html/forms/src/nsFormControlFrame.cpp:93 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3598) layout/html/forms/src/nsFormControlFrame.cpp:94 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3599) layout/html/forms/src/nsFormControlFrame.cpp:95 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3600) layout/html/forms/src/nsFormControlFrame.cpp:96 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3601) layout/html/forms/src/nsListControlFrame.cpp:170 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3602) layout/html/forms/src/nsListControlFrame.cpp:171 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3603) layout/html/forms/src/nsListControlFrame.cpp:172 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3604) layout/html/forms/src/nsListControlFrame.cpp:173 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3605) layout/html/forms/src/nsListControlFrame.cpp:194 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3606) layout/html/forms/src/nsListControlFrame.cpp:195 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3607) layout/html/forms/src/nsListControlFrame.cpp:196 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3608) layout/html/forms/src/nsListControlFrame.cpp:197 [4] (format) printf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3609) layout/html/table/src/nsTableFrame.cpp:7223 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3610) layout/html/table/src/nsTableFrame.cpp:7225 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3611) layout/html/table/src/nsTableFrame.cpp:7229 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3612) layout/html/table/src/nsTableFrame.cpp:7231 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3613) layout/html/table/src/nsTableFrame.cpp:7233 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3614) layout/html/table/src/nsTableFrame.cpp:7235 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3615) layout/html/table/src/nsTableFrame.cpp:7251 [2] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy. Risk is low 
because the source is a constant string.

3616) layout/html/table/src/nsTableFrame.cpp:7254 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3617) layout/svg/base/src/nsSVGGenericContainerFrame.cpp:276 [4] (format) 
printf: if format strings can be influenced by an attacker, they can be 
exploited. Use a constant for the format specification.

3618) layout/svg/base/src/nsSVGRenderingContext.cpp:167 [2] (buffer) sprintf: 
does not check for buffer overflows. Use snprintf or vsnprintf. Risk is low 
because the source has a constant maximum length.

Comment 1

[timeless]

general notes: the list is very stale. we're going to ignore getenv because the
user is already dead if that's a problem.

Base
nsFrame.cpp is in // Start Display Reflow which is for debug only

nsPageFrame.cpp
smprintf is user input
strcpy #ifdef DEBUG_PRINTING

nsPresShell.cpp
sprintf is debug and safe unless a pointer becomes >16 characters long
strcpy and strcat are debug

nsSimplePageSequence.cpp
printf is commented out

forms
nsComboboxControlFrame.cpp
printf is debug

nsFormControlFrame.cpp
printf is debug

nsListControlFrame.cpp
printf is debug

table
nsTableFrame.cpp
printf is debug
strcpy is debug

svg
nsSVGGenericContainerFrame.cpp
printf is debug

nsSVGRenderingContext.cpp
printf is debug

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am reopening for now. I don't agree that "smprintf is user input" is good
enough. It takes variable number of arguments. In every use case, does the
format string match what we are trying to insert? Do we allocate enough memory
to hold the string?

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Timeless, please see my comment above.

Comment 4

[timeless]

a bit of clarification: not only is it user input, the implementation of
nsTextFormatter::smprintf doesn't do stupid things. it can be found at
http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsTextFormatter.cpp

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

timeless, you did not answer my questions. When you say it doesn't do stupid
things, do you mean it checks the number of placeholders in the format string
against the number of actual arguments (and types, btw... it can't do that
automatically I think), and does it correctly calculate the length of the buffer
it allocates for the new string? Note that the standard C library functions do
NOT check the number of arguments, and can't check the types either.

Comment 6

[timeless]

sorry, it's not user input, it is localizer input.  There is a localization note
which makes it pretty clear to localizers what to do.  Current policy says that
skins are not allowed to crash mozilla but I think locales are.  If a localizer
messes up then it's a bug in the localization.

Remember that a localization could mislabel "Delete Message" as "Open Message"
and change "Are you sure you want to delete your inbox" to "Yes Get My Mail". 
Localizers must be trustworthy.

However it goes, any bug about that class's behavior belongs to intl/xpcom and
not layout (actually printing).

if you have concerns about that class or its users then please read it or file a
bug in xpcom.  The caller which is what this bug is concerned about has chosen a
class which is designed to do the right thing. It mallocs its own buffer.  It
does not check the number of parameters, and the caller doesn't verify that
there are not more than two parameters.

/layout/html/base/src/nsSimplePageSequence.cpp, line 175 --
SetPageNumberFormat("pagenumber", "%1$d", PR_TRUE);
/layout/html/base/src/nsSimplePageSequence.cpp, line 176 --
SetPageNumberFormat("pageofpages", "%1$d of %2$d", PR_FALSE);
/layout/html/base/src/nsSimplePageSequence.cpp, line 628 -- nsresult rv =
nsFormControlHelper::GetLocalizedString(PRINTING_PROPERTIES,
NS_ConvertUTF8toUCS2(aPropName).get(), pageNumberFormat);
/layout/html/base/src/nsSimplePageSequence.cpp, line 801 --
SetPageNumberFormat("pagenumber", "%1$d", PR_TRUE);
/layout/html/base/src/nsSimplePageSequence.cpp, line 802 --
SetPageNumberFormat("pageofpages", "%1$d of %2$d", PR_FALSE);

Comment 7

[Stephen P. Morse]

Flawfinder reported 38 more warning messages in layout code (4557-4594). 
Reopening.  (See also bug 173558 for more warnings in layout.)

4557) layout/html/base/src/nsFrame.cpp:4567 [4] (buffer) strcpy: does not check
for buffer overflows. Consider using strncpy or strlcpy.

4558) layout/html/base/src/nsFrame.cpp:4568 [4] (buffer) strcpy: does not check
for buffer overflows. Consider using strncpy or strlcpy.

4559) layout/html/base/src/nsPageFrame.cpp:91 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4560) layout/html/base/src/nsPageFrame.cpp:92 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4561) layout/html/base/src/nsPageFrame.cpp:93 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4562) layout/html/base/src/nsPageFrame.cpp:94 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4563) layout/html/base/src/nsPageFrame.cpp:95 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4564) layout/html/base/src/nsPresShell.cpp:7851 [4] (buffer) strcpy: does not
check for buffer overflows. Consider using strncpy or strlcpy.

4565) layout/html/base/src/nsSimplePageSequence.cpp:94 [4] (format) fprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4566) layout/html/base/src/nsSimplePageSequence.cpp:95 [4] (format) fprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4567) layout/html/base/src/nsSimplePageSequence.cpp:96 [4] (format) fprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4568) layout/html/base/src/nsSimplePageSequence.cpp:97 [4] (format) fprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4569) layout/html/base/src/nsSimplePageSequence.cpp:98 [4] (format) fprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4570) layout/html/forms/src/nsComboboxControlFrame.cpp:220 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4571) layout/html/forms/src/nsComboboxControlFrame.cpp:221 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4572) layout/html/forms/src/nsComboboxControlFrame.cpp:222 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4573) layout/html/forms/src/nsComboboxControlFrame.cpp:223 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4574) layout/html/forms/src/nsComboboxControlFrame.cpp:244 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4575) layout/html/forms/src/nsComboboxControlFrame.cpp:245 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4576) layout/html/forms/src/nsComboboxControlFrame.cpp:246 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4577) layout/html/forms/src/nsComboboxControlFrame.cpp:247 [4] (format) printf:
if format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4578) layout/html/forms/src/nsFormControlFrame.cpp:88 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4579) layout/html/forms/src/nsFormControlFrame.cpp:89 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4580) layout/html/forms/src/nsFormControlFrame.cpp:90 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4581) layout/html/forms/src/nsFormControlFrame.cpp:91 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4582) layout/html/forms/src/nsFormControlFrame.cpp:93 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4583) layout/html/forms/src/nsFormControlFrame.cpp:94 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4584) layout/html/forms/src/nsFormControlFrame.cpp:95 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4585) layout/html/forms/src/nsFormControlFrame.cpp:96 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4586) layout/html/forms/src/nsListControlFrame.cpp:170 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4587) layout/html/forms/src/nsListControlFrame.cpp:171 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4588) layout/html/forms/src/nsListControlFrame.cpp:172 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4589) layout/html/forms/src/nsListControlFrame.cpp:173 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4590) layout/html/forms/src/nsListControlFrame.cpp:194 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4591) layout/html/forms/src/nsListControlFrame.cpp:195 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4592) layout/html/forms/src/nsListControlFrame.cpp:196 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4593) layout/html/forms/src/nsListControlFrame.cpp:197 [4] (format) printf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4594) layout/svg/base/src/nsSVGGenericContainerFrame.cpp:276 [4] (format)
printf: if format strings can be influenced by an attacker, they can be
exploited. Use a constant for the format specification.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

If localization can crash us it should be a bug, at least when it comes to
buffer lengths. 

I can't remember if I filed it, but our localization code should check the
arguments and types in the string bundles and check that the actual values match
( think that would be the easiest way for localizations to mess us up att he
moment - wrong number of arguments with wrong types). But it should be its own
bug, and not solved as part of this one.

Hmm, the second batch of flawfinder warnings seems to contain only duplicates.

Marking as INVALID.