flawfinder warnings in embedding/activex

VERIFIED FIXED

Status

Core Graveyard
Embedding: ActiveX Wrapper
VERIFIED FIXED
16 years ago
6 years ago

People

(Reporter: Stephen P. Morse, Assigned: Adam Lock)

Tracking

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.95 KB, patch
Chak Nanga
: review+
Heikki Toivonen (remove -bugzilla when emailing directly)
: superreview+
jesup
: approval+
Details | Diff | Splinter Review
(Reporter)

Description

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

flawfinder found 7 warnings in embedding/activex code (1746-1752). 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.

1746) embedding/browser/activex/src/control/MozillaBrowser.cpp:615 [4] (buffer) 
strcpy: does not check for buffer overflows. Consider using strncpy or strlcpy.

1747) embedding/browser/activex/src/plugin/LegacyPlugin.cpp:167 [4] (buffer) 
sprintf: does not check for buffer overflows. Use snprintf or vsnprintf.

1748) embedding/browser/activex/src/plugin/LegacyPlugin.cpp:171 [4] (buffer) 
sprintf: does not check for buffer overflows. Use snprintf or vsnprintf.

1749) embedding/browser/activex/src/pluginhostctrl/nsPluginHostCtrl.cpp:687 [2] 
(buffer) sprintf: does not check for buffer overflows. Use snprintf or 
vsnprintf. Risk is low because the source has a constant maximum length.

1750) embedding/browser/activex/src/pluginhostctrl/nsPluginHostCtrl.cpp:689 [2] 
(buffer) sprintf: does not check for buffer overflows. Use snprintf or 
vsnprintf. Risk is low because the source has a constant maximum length.

1751) embedding/browser/activex/src/pluginhostctrl/pluginsdk_include/jni.h:1742 
[4] (format) vfprintf: if format strings can be influenced by an attacker, they 
can be exploited. Use a constant for the format specification.

1752) embedding/browser/activex/src/tlb2xpt/TypeDesc.cpp:126 [2] (buffer) 
sprintf: does not check for buffer overflows. Use snprintf or vsnprintf. Risk is 
low because the source has a constant maximum length.
(Reporter)

Updated

16 years ago
Blocks: 148251
(Assignee)

Comment 1

16 years ago
1. Looks like a bug
2. Looks like a bug. There is string length checking in place, but it's possible
an overflow could be still triggered. This will be fixed independently since the
plugin is being actively developed at present.
3. Same as 2
4. Not a bug, buffer only receives an int value and at 50 bytes is too big to be
overflowed.
5. Same as 4
6. File copied from the 4.x NPAPI sdk, not my code
7. Not a bug. Utility is not built and is merely a command-line development tool.

So 1 is a bug that I'll leave this bug open to fix, 2 & 3 are too but will be
fixed in my next ongoing checkin. The others are not bugs.
(Assignee)

Comment 2

16 years ago
Created attachment 102609 [details] [diff] [review]
Patch for issue 1
(Assignee)

Comment 3

16 years ago
Can I have an r= on this simple patch? I replaced strcpy with strncpy and blank
out the last value in the buffer with \0 irrespective.
(Reporter)

Comment 4

16 years ago
4 more flawfinder bugs in activex (4291-4294)

4291) embedding/browser/activex/src/control/MozillaBrowser.cpp:615 [4] (buffer)
strcpy: does not check for buffer overflows. Consider using strncpy or strlcpy.

4292) embedding/browser/activex/src/plugin/LegacyPlugin.cpp:167 [4] (buffer)
sprintf: does not check for buffer overflows. Use snprintf or vsnprintf.

4293) embedding/browser/activex/src/plugin/LegacyPlugin.cpp:171 [4] (buffer)
sprintf: does not check for buffer overflows. Use snprintf or vsnprintf.

4294) embedding/browser/activex/src/pluginhostctrl/pluginsdk_include/jni.h:1742
[4] (format) vfprintf: if format strings can be influenced by an attacker, they
can be exploited. Use a constant for the format specification.

Comment on attachment 102609 [details] [diff] [review]
Patch for issue 1

sr=heikki, but consider if you want to make this function dynamically allocate
enough space and/or fail if we can't copy enough. Depends on how important
stuff we are doing here, i.e. does it matter if we truncate silently.
Attachment #102609 - Flags: superreview+
(Assignee)

Comment 6

16 years ago
The buffer is meant to hold a clsid. If it's bigger than the buffer then it's
definitely not a clsid and can be safely ignored.

Updated

16 years ago
QA Contact: mdunn → ashishbhatt

Comment 7

16 years ago
Comment on attachment 102609 [details] [diff] [review]
Patch for issue 1

r=chak
Attachment #102609 - Flags: review+
(Assignee)

Comment 8

16 years ago
Oops, I mixed up my comments earlier, it has nothing to do with clsids.
Basically the patch is still limited in length by maximum filename length
supported by Win32. So if the title of document exceeds that length, the rest
can be truncated and thrown away since it can't be saved anyway.
Comment on attachment 102609 [details] [diff] [review]
Patch for issue 1

a=rjesup@wgate.com
Attachment #102609 - Flags: approval+
(Assignee)

Comment 10

16 years ago
Fix for issues 1, 2 & 3 are checked in. I'll mark this fixed since the others
are false positives.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 11

16 years ago
Marking as verified for 1, 2 & 3
Status: RESOLVED → VERIFIED
Component: Embedding: ActiveX Wrapper → Embedding: ActiveX Wrapper
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.