Closed Bug 173837 Opened 22 years ago Closed 22 years ago

flawfinder warnings in content

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: morse, Assigned: jst)

References

Details

Attachments

(1 file, 1 obsolete file)

patch to nsCSSParser (diff -u)
3.83 KB, patch
Details | Diff | Splinter Review 
Heikki ran flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 
branch.

flawfinder found 46 warnings in content code (3344-3389). 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.

3344) content/base/src/nsDocumentViewer.cpp:216 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3345) content/base/src/nsDocumentViewer.cpp:217 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3346) content/base/src/nsDocumentViewer.cpp:218 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3347) content/base/src/nsDocumentViewer.cpp:219 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3348) content/base/src/nsDocumentViewer.cpp:220 [4] (format) fprintf: if format 
strings can be influenced by an attacker, they can be exploited. Use a constant 
for the format specification.

3349) content/base/src/nsDocumentViewer.cpp:1892 [4] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy.

3350) content/base/src/nsDocumentViewer.cpp:1896 [1] (buffer) strcat: does not 
check for buffer overflows. Consider using strncat or strlcat. Risk is low 
because the source is a constant character.

3351) content/base/src/nsDocumentViewer.cpp:1899 [4] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy.

3352) content/base/src/nsDocumentViewer.cpp:1900 [2] (buffer) strcat: does not 
check for buffer overflows. Consider using strncat or strlcat. Risk is low 
because the source is a constant string.

3353) content/base/src/nsDocumentViewer.cpp:1914 [4] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy.

3354) content/base/src/nsDocumentViewer.cpp:1915 [1] (buffer) strcat: does not 
check for buffer overflows. Consider using strncat or strlcat. Risk is low 
because the source is a constant character.

3355) content/base/src/nsDocumentViewer.cpp:1916 [4] (buffer) strcat: does not 
check for buffer overflows. Consider using strncat or strlcat.

3356) content/base/src/nsDocumentViewer.cpp:2069 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3357) content/base/src/nsDocumentViewer.cpp:3644 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3358) content/html/content/src/nsHTMLInputElement.cpp:2138 [2] (buffer) sprintf: 
does not check for buffer overflows. Use snprintf or vsnprintf. Risk is low 
because the source has a constant maximum length.

3359) content/html/content/src/nsHTMLInputElement.cpp:2140 [2] (buffer) sprintf: 
does not check for buffer overflows. Use snprintf or vsnprintf. Risk is low 
because the source has a constant maximum length.

3360) content/html/style/src/nsCSSParser.cpp:2509 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3361) content/html/style/src/nsCSSParser.cpp:2516 [2] (buffer) sprintf: does not 
check for buffer overflows. Use snprintf or vsnprintf. Risk is low because the 
source has a constant maximum length.

3362) content/svg/content/src/nsSVGLength.cpp:286 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3363) content/svg/content/src/nsSVGPathSeg.cpp:210 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3364) content/svg/content/src/nsSVGPathSeg.cpp:319 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3365) content/svg/content/src/nsSVGPathSeg.cpp:427 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3366) content/svg/content/src/nsSVGPathSeg.cpp:536 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3367) content/svg/content/src/nsSVGPathSeg.cpp:651 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3368) content/svg/content/src/nsSVGPathSeg.cpp:823 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3369) content/svg/content/src/nsSVGPathSeg.cpp:991 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3370) content/svg/content/src/nsSVGPathSeg.cpp:1132 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3371) content/svg/content/src/nsSVGPathSeg.cpp:1279 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3372) content/svg/content/src/nsSVGPathSeg.cpp:1469 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3373) content/svg/content/src/nsSVGPathSeg.cpp:1648 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3374) content/svg/content/src/nsSVGPathSeg.cpp:1741 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3375) content/svg/content/src/nsSVGPathSeg.cpp:1834 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3376) content/svg/content/src/nsSVGPathSeg.cpp:1927 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3377) content/svg/content/src/nsSVGPathSeg.cpp:2024 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3378) content/svg/content/src/nsSVGPathSeg.cpp:2164 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3379) content/svg/content/src/nsSVGPathSeg.cpp:2301 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3380) content/svg/content/src/nsSVGPathSeg.cpp:2409 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3381) content/svg/content/src/nsSVGPointList.cpp:230 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3382) content/svg/content/src/nsSVGTransform.cpp:151 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3383) content/svg/content/src/nsSVGTransform.cpp:153 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3384) content/svg/content/src/nsSVGTransform.cpp:159 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3385) content/svg/content/src/nsSVGTransform.cpp:161 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3386) content/svg/content/src/nsSVGTransform.cpp:170 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3387) content/svg/content/src/nsSVGTransform.cpp:172 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3388) content/svg/content/src/nsSVGTransform.cpp:179 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.

3389) content/svg/content/src/nsSVGTransform.cpp:186 [4] (format) snprintf: if 
format strings can be influenced by an attacker, they can be exploited. Use a 
constant for the format specification.
Blocks: 148251
Alex, Bradley: could you check the warnings under SVG?
nsDocumentViewer.cpp: there are no longer fprintf calls on the trunk, and on the
1.0 branch they are debug code.


The line in question is:

nsTextFormatter::snprintf(buf, sizeof(buf)/sizeof(PRUnichar),
NS_LITERAL_STRING("%g").get(), (double)mValueInSpecifiedUnits);

The format argument is constant, but I guess flawfinder can't really know that.

I didn't check all of the SVG stuff, though, just a couple.
nsDocumentViewer.cpp: there are no longer strcpy, strcat or sprintf calls.

nsHTMLInputElement.cpp: we print into a fixed buffer of size 20 which is enough
to hold a 32-bit integer (%d).


The stuff in nsCSSParser.cpp looks suspicuous. See at least lines 2576, 2583 on
the trunk (2509 and 2516 on 1.0 branch). We have a fixed buffer of size 10 and
use snprintf to write 32-bit int, and float into it. But 32-bit int can be 10
chars long, so where does the null terminator go? Can't remember how the float
works off the top of my head, but that is probably also unsafe. We then convert
the buffer into nsAutoString. Cc John Keiser who indicated he knows a better way
to do this.

http://lxr.mozilla.org/mozilla/source/content/html/style/src/nsCSSParser.cpp#2576

The struct is nsCSSToken, mInteger is PRInt32 and mNumber is float:
http://lxr.mozilla.org/mozilla/source/content/html/style/src/nsCSSScanner.h#96
37 more flawfinder warnings

4031) content/base/src/nsDocumentViewer.cpp:217 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4032) content/base/src/nsDocumentViewer.cpp:218 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4033) content/base/src/nsDocumentViewer.cpp:219 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4034) content/base/src/nsDocumentViewer.cpp:220 [4] (format) fprintf: if format
strings can be influenced by an attacker, they can be exploited. Use a constant
for the format specification.

4035) content/base/src/nsDocumentViewer.cpp:1892 [4] (buffer) strcpy: does not
check for buffer overflows. Consider using strncpy or strlcpy.

4036) content/base/src/nsDocumentViewer.cpp:1899 [4] (buffer) strcpy: does not
check for buffer overflows. Consider using strncpy or strlcpy.

4037) content/base/src/nsDocumentViewer.cpp:1914 [4] (buffer) strcpy: does not
check for buffer overflows. Consider using strncpy or strlcpy.

4038) content/base/src/nsDocumentViewer.cpp:1916 [4] (buffer) strcat: does not
check for buffer overflows. Consider using strncat or strlcat.

4039) content/svg/content/src/nsSVGLength.cpp:286 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4040) content/svg/content/src/nsSVGPathSeg.cpp:210 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4041) content/svg/content/src/nsSVGPathSeg.cpp:319 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4042) content/svg/content/src/nsSVGPathSeg.cpp:427 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4043) content/svg/content/src/nsSVGPathSeg.cpp:536 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4044) content/svg/content/src/nsSVGPathSeg.cpp:651 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4045) content/svg/content/src/nsSVGPathSeg.cpp:823 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4046) content/svg/content/src/nsSVGPathSeg.cpp:991 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4047) content/svg/content/src/nsSVGPathSeg.cpp:1132 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4048) content/svg/content/src/nsSVGPathSeg.cpp:1279 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4049) content/svg/content/src/nsSVGPathSeg.cpp:1469 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4050) content/svg/content/src/nsSVGPathSeg.cpp:1648 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4051) content/svg/content/src/nsSVGPathSeg.cpp:1741 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4052) content/svg/content/src/nsSVGPathSeg.cpp:1834 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4053) content/svg/content/src/nsSVGPathSeg.cpp:1927 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4054) content/svg/content/src/nsSVGPathSeg.cpp:2024 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4055) content/svg/content/src/nsSVGPathSeg.cpp:2164 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4056) content/svg/content/src/nsSVGPathSeg.cpp:2301 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4057) content/svg/content/src/nsSVGPathSeg.cpp:2409 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4058) content/svg/content/src/nsSVGPointList.cpp:230 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4059) content/svg/content/src/nsSVGTransform.cpp:151 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4060) content/svg/content/src/nsSVGTransform.cpp:153 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4061) content/svg/content/src/nsSVGTransform.cpp:159 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4062) content/svg/content/src/nsSVGTransform.cpp:161 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4063) content/svg/content/src/nsSVGTransform.cpp:170 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4064) content/svg/content/src/nsSVGTransform.cpp:172 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4065) content/svg/content/src/nsSVGTransform.cpp:179 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.

4066) content/svg/content/src/nsSVGTransform.cpp:186 [4] (format) snprintf: if
format strings can be influenced by an attacker, they can be exploited. Use a
constant for the format specification.


This increases the size of the buffer from 10 to 20 (why not?  It's just a
little stack space) and changes it from sprintf to snprintf.  I also untabified
the 40-line section of code that it was in (which was the only tabbed section
in the area and rewrapped an overly long comment in the tabbed section, since
I'd be taking blame for it anyway.
Attached patch patch to nsCSSParser (diff -uw) (obsolete) — — Splinter Review 
Same as above, but diff -uw instead of diff -w.
Attachment #102697 - Attachment description: patch to nsCSSParser (diff -uw) → patch to nsCSSParser (diff -u)
Comment on attachment 102698 [details] [diff] [review]
patch to nsCSSParser (diff -uw)

r/sr=bzbarsky
Attachment #102698 - Flags: superreview+
Comment on attachment 102698 [details] [diff] [review]
patch to nsCSSParser (diff -uw)

r=heikki

But isn't there are way to make this more efficient as jkeiser implied (or
maybe I misunderstood), meaning better than copy-to-C-buffer-then-nsString?
Attachment #102698 - Flags: review+
Comment on attachment 102698 [details] [diff] [review]
patch to nsCSSParser (diff -uw)

a=rjesup@wgate.com
Attachment #102698 - Flags: approval+
Comment on attachment 102698 [details] [diff] [review]
patch to nsCSSParser (diff -uw)

It's worth noting that snprintf is not portable across all our platforms. 
(Some of them ifdef it in stdio.h.  Look on myotonic for an example.)  So this
is checked in, but using PR_snprintf.  And the testcases on bug 170614 (which I
used to test this earlier as well) still seem the same.
Attachment #102698 - Attachment is obsolete: true
you know, there is also nsTextFormatter::ssprintf that has nsAString& output -
you could print directly into "str" and avoid the extra conversion.
All of the SVG stuff is safe from security perspective, but it could be improved.

As Alec suggest, if you have code that sprintfs to a buffer and then puts the
buffer into a string object you could achieve that with a single call. SVG had
lots of these.

Also there is stuff like (nsSVGLength::SetValueAsString()):

301   // XXX how am I supposed to do this ??? 
302   // char* str  = aValue.ToNewCString();
303   char* str;
304   {
305     nsAutoString temp(aValueAsString);
306     str = ToNewCString(temp);
307   }

I believe you could do

  char* str = ToNewCString(aValue);

In any case, you will need to check if the allocation succeeded.

In nsSVGPathSeg.cpp we have a bunch of the format warnings. They are safe,
but... I am not sure how many characters a |float| can take when printed with
|%g| format; my naive experiments seem to indicate 10, at least on my system. If
it can take more, we may end up truncating, especially in
nsSVGPathSegCurvetoQuadraticSmoothRel::GetValueString() where we have a buffer
of 24 and do "t%g,%g".

Closing as fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: