173630 - flawfinder warnings in embedding/activex

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Stephen P. Morse]

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.

Comment 1

[Adam Lock]

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.

Comment 2

[Adam Lock]

Attachment: Patch for issue 1

Comment 3

[Adam Lock]

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.

Comment 4

[Stephen P. Morse]

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 5

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

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.

Comment 6

[Adam Lock]

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.

Comment 7

[Chak Nanga]

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

r=chak

Comment 8

[Adam Lock]

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 9

[Randell Jesup [:jesup] (needinfo me)]

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

a=[e-mail]

Comment 10

[Adam Lock]

Fix for issues 1, 2 & 3 are checked in. I'll mark this fixed since the others
are false positives.

Comment 11

[Ashish Bhatt]

Marking as verified for 1, 2 & 3