Bug 34955 - Veracode is flagging "Very High" security flaws related to potential Buffer Overflows in Mono
Summary: Veracode is flagging "Very High" security flaws related to potential Buffer O...
Status: RESOLVED ANSWERED
Alias: None
Product: Runtime
Classification: Mono
Component: General ()
Version: 4.0.0
Hardware: PC Mac OS
: --- major
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2015-10-16 13:40 UTC by David Hathaway
Modified: 2015-10-16 16:13 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Veracode scan log (3.91 KB, application/octet-stream)
2015-10-16 13:40 UTC, David Hathaway
Details
Build Information (1.24 KB, application/octet-stream)
2015-10-16 14:09 UTC, David Hathaway
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED ANSWERED

Description David Hathaway 2015-10-16 13:40:30 UTC
Created attachment 13381 [details]
Veracode scan log

Vericode is flagging potential buffer overflows in Mono code.  Attached log file contains full details, but the affected files are:

https://github.com/mono/mono/blob/master/mono/utils/mono-context.c
https://github.com/mono/mono/blob/master/mono/metadata/icall.c

This is blocking customer from being able publish app.  Near term workaround be explanation of why this may not actually affect the app, while we implement a long term fix to the runtime.
Comment 1 David Hathaway 2015-10-16 14:09:45 UTC
Created attachment 13382 [details]
Build Information
Comment 2 Rodrigo Kumpera 2015-10-16 16:13:31 UTC
Hi David,

There are 3 issues in the report.

The first and the last point to the same piece of code, so it's a duplicate.

In both cases looks like VeriCode got its size calculations wrong.

In the mono-context.c case, this is the relevant line of code: https://github.com/mono/mono/blob/mono-4.0.0-branch/mono/utils/mono-context.c#L337

memcpy (&mctx->fregs, UCONTEXT_REG_VFPREGS (my_uc), sizeof (double) * 16)

It says "..space allocated to the destination buffer (17 bytes)". The destination buffer in this case is mctx->fregs.

mctx is of type MonoContext which is declared like this for arm:

typedef struct {
	mgreg_t pc;
	mgreg_t regs [16];
	double fregs [16];
	mgreg_t cpsr;
} MonoContext;

As we can see it's size is sizeof (double) * 16. The same used in the memcpy call. So this first bug is a false positive.

The second report is for this piece of code:
https://github.com/mono/mono/blob/mono-4.0.0-branch/mono/metadata/icall.c#L6237

gchar buf [256];
MonoString *result;

if (gethostname (buf, sizeof (buf)) == 0)
	result = mono_string_new (mono_domain_get (), buf);
else
	result = NULL;

return result;

It says: "The specified size of 5308672 bytes is larger than the space allocated to the destination buffer (512 bytes)".

Let's see what the sizes are. First, the destination buffer is 'buf', which is 256 bytes long and not 512.

Second, the size of the destination buffer passed to gethostname is sizeof(buf), which will return 256, which is the right value and would not trigger a buffer overflow as claimed. Another false positive.

I hope this is enough information to answer you customer's concerns.