Covierty Integration / Improvement

Covierty Integration / Improvement

[Markus Vervier]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

Dear git developer team,

X41 is processing the current RfP and some questions came up regarding 
the improvement / integration of Coverity Scans for git and the 
estimation of the required work:

- Was there a special purpose for the Coverity integration (e.g. custom 
queries for variant analysis or regression testing?) or did you try to 
integrate it as a best practice / general security hygiene tool?
- Could you tell us more about the amount and types of false positives 
and problems you've faced trying to eliminate them? This will help us to 
understand the expectations / requirements for a successful integration 
of Coverity.
- Could we get access to a sample of the scan results?

Many Thanks

Markus
-- 
Markus Vervier (Managing Director)
X41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen
T: +49 241 9809418-0, Fax: -9
Unternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989
Geschäftsführer: Markus Vervier

[-- Attachment #2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6214 bytes --]

Re: Covierty Integration / Improvement

[Junio C Hamano]

Markus Vervier <markus.vervier@x41-dsec.de> writes:

> - Was there a special purpose for the Coverity integration
>   (e.g. custom queries for variant analysis or regression testing?) or
>  did you try to integrate it as a best practice / general security
> hygiene tool?
> - Could you tell us more about the amount and types of false positives
>   and problems you've faced trying to eliminate them? This will help
>  us to understand the expectations / requirements for a successful
> integration of Coverity.
> - Could we get access to a sample of the scan results?

Not sure what X41 RfP is and how it matters to us, but anyway.

I have old e-mails from the scan-admin@coverity.com but the last one
seems to be from late June 2018, which is ages ago in Git timescale.
I do not recall us paying for such a service so I am guessing that
they had some program that open source projects can enroll, get our
public sources scanned and get the result sent back?

https://scan.coverity.com/projects/git/ (visible without signing in)
seems to match my recollection. They haven't been scanning since
late June 2018.  I wasn't the primary developer who registered us or
who has been reading these reports but if I recall correctly, we
weren't doing anything custom, and fell somewhere between just "we
are curious to see how well Coverity works" and "Yay, a free
offering. We have nothing to lose, other than our time, to sign
ourselves up and if it comes up with useful scan result that would
be good".

Below is a random one back from 2018 timeframe.  Looking at the
places around these three issues in the report in the current
codebase, I think the first two (i.e. calling die() with some
on-stack variable pointing at an allocated piece of memory in heap,
letting exit() to eventually clean up after us) are something we do
not care about, and consider "noise", and the other one (i.e. have
an open fd, leave the function without an intention to kill the
process soon, forgetting to close it) has long been fixed.  I am
reasonably sure that the "fix" happend regardless of the Coverity
report.

----- >8 --------- >8 --------- >8 --------- >8 -----


Hi,

Please find the latest report on new defect(s) introduced to git found with Coverity Scan.

3 new defect(s) introduced to git found with Coverity Scan.


New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)


** CID 1437531:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 900 in clear_midx_file()


________________________________________________________________________________________________________
*** CID 1437531:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 900 in clear_midx_file()
894     void clear_midx_file(const char *object_dir)
895     {
896     	char *midx = get_midx_filename(object_dir);
897     
898     	if (remove_path(midx))
899     		die(_("failed to clear multi-pack-index at %s"), midx);
>>>     CID 1437531:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "midx" going out of scope leaks the storage it points to.

** CID 1437530:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 891 in write_midx_file()


________________________________________________________________________________________________________
*** CID 1437530:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 891 in write_midx_file()
885     	}
886     
887     	FREE_AND_NULL(packs.list);
888     	FREE_AND_NULL(packs.names);
889     	FREE_AND_NULL(entries);
890     	FREE_AND_NULL(pack_perm);
>>>     CID 1437530:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "midx_name" going out of scope leaks the storage it points to.
891     	return 0;
892     }
893     
894     void clear_midx_file(const char *object_dir)
895     {
896     	char *midx = get_midx_filename(object_dir);
897     
898     	if (remove_path(midx))
899     		die(_("failed to clear multi-pack-index at %s"), midx);

** CID 1437529:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 169 in load_multi_pack_index()


________________________________________________________________________________________________________
*** CID 1437529:  Resource leaks  (RESOURCE_LEAK)
/midx.c: 169 in load_multi_pack_index()
163     			      m->pack_names[i - 1],
164     			      m->pack_names[i]);
165     			goto cleanup_fail;
166     		}
167     	}
168     
>>>     CID 1437529:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
169     	return m;
170     
171     cleanup_fail:
172     	FREE_AND_NULL(m);
173     	FREE_AND_NULL(midx_name);
174     	munmap(midx_map, midx_size);


________________________________________________________________________________________________________

Re: Covierty Integration / Improvement

[Theodore Ts'o]

On Sun, Apr 03, 2022 at 02:36:22PM -0700, Junio C Hamano wrote:
> I have old e-mails from the scan-admin@coverity.com but the last one
> seems to be from late June 2018, which is ages ago in Git timescale.
> I do not recall us paying for such a service so I am guessing that
> they had some program that open source projects can enroll, get our
> public sources scanned and get the result sent back?

Yep, that's the way it works.  Someone has to use tools provided by
them to build the open source project and upload the results for them
to analyze.  Coverity predates github, so it's not new-fangled enough
to automatically pull sources from repositories; besides, their paying
customers tend to be using their tool for their proprietary software,
so they haven't had any incentive to create an auto-analyze tool that
pulls from an open source repository.

Some folks at Red Hat do have scripts run out of crontab, that will
monitor git branches on projects that they are interested in and when
they notice that the branch has been updated, they will build and
upload the raw material used by Coverity to their dashboard.  Eric
Sandeen has been doing this for e2fsprogs, and a few other file system
related repo's, and I suspect if someone asked, he would probably be
willing to provide the scripts that he uses.

You do need to be the project admin, or someone authorized by the
project admin, to upload new data for Coverity, or to look at the
analysis of the Coverity results.  I have no idea who the project
admin is for git, but I'm sure if you, as the Git maintainer showed up
and requested to be added as one of the project admin, the open source
ombudsperson (I don't remember the exact title, but they do have
someone who interfaces with OSS projects), would be happy to oblige.

> https://scan.coverity.com/projects/git/ (visible without signing in)
> seems to match my recollection. They haven't been scanning since
> late June 2018.  I wasn't the primary developer who registered us or
> who has been reading these reports but if I recall correctly, we
> weren't doing anything custom, and fell somewhere between just "we
> are curious to see how well Coverity works" and "Yay, a free
> offering. We have nothing to lose, other than our time, to sign
> ourselves up and if it comes up with useful scan result that would
> be good".

My experience with e2fsprogs is that it does have a fair amount of
false positives, but I've been willing to wade through the false
positives, and mark them as such in their web dashboard, because the
early warnings it gives when we've pushed new code that has a
potential security problem is worth it.  But make no mistake, it
definitely requires a certain amount of maintainer time work with the
tool.

Cheers,

						- Ted

Re: Covierty Integration / Improvement

[Ævar Arnfjörð Bjarmason]

COVID19 is spreading via E-Mail now? It's $subject =~
s/Covierty/Coverity/g :)

On Sun, Apr 03 2022, Theodore Ts'o wrote:

> On Sun, Apr 03, 2022 at 02:36:22PM -0700, Junio C Hamano wrote:
>> I have old e-mails from the scan-admin@coverity.com but the last one
>> seems to be from late June 2018, which is ages ago in Git timescale.
>> I do not recall us paying for such a service so I am guessing that
>> they had some program that open source projects can enroll, get our
>> public sources scanned and get the result sent back?
>
> Yep, that's the way it works.  Someone has to use tools provided by
> them to build the open source project and upload the results for them
> to analyze.  Coverity predates github, so it's not new-fangled enough
> to automatically pull sources from repositories; besides, their paying
> customers tend to be using their tool for their proprietary software,
> so they haven't had any incentive to create an auto-analyze tool that
> pulls from an open source repository.
>
> Some folks at Red Hat do have scripts run out of crontab, that will
> monitor git branches on projects that they are interested in and when
> they notice that the branch has been updated, they will build and
> upload the raw material used by Coverity to their dashboard.  Eric
> Sandeen has been doing this for e2fsprogs, and a few other file system
> related repo's, and I suspect if someone asked, he would probably be
> willing to provide the scripts that he uses.
>
> You do need to be the project admin, or someone authorized by the
> project admin, to upload new data for Coverity, or to look at the
> analysis of the Coverity results.  I have no idea who the project
> admin is for git, but I'm sure if you, as the Git maintainer showed up
> and requested to be added as one of the project admin, the open source
> ombudsperson (I don't remember the exact title, but they do have
> someone who interfaces with OSS projects), would be happy to oblige.

Per
https://lore.kernel.org/git/YarO3nkrutmWF7nb@coredump.intra.peff.net/
Jeff ran this from his fork, I'm not sure if that was because he set
something up in the git/git organization, or if by project admin you
mean that any fork of it can set this up on their own.

>> https://scan.coverity.com/projects/git/ (visible without signing in)
>> seems to match my recollection. They haven't been scanning since
>> late June 2018.  I wasn't the primary developer who registered us or
>> who has been reading these reports but if I recall correctly, we
>> weren't doing anything custom, and fell somewhere between just "we
>> are curious to see how well Coverity works" and "Yay, a free
>> offering. We have nothing to lose, other than our time, to sign
>> ourselves up and if it comes up with useful scan result that would
>> be good".
>
> My experience with e2fsprogs is that it does have a fair amount of
> false positives, but I've been willing to wade through the false
> positives, and mark them as such in their web dashboard, because the
> early warnings it gives when we've pushed new code that has a
> potential security problem is worth it.  But make no mistake, it
> definitely requires a certain amount of maintainer time work with the
> tool.

Yes, also per the linked-above output it's quite noise, but there looked
to be some legitimate and hard-to find issues in those reports. It would
be nice to get them running with some regularity on our main branches.

Re: Covierty Integration / Improvement

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2987 bytes --]

Hi Markus,

On Fri, 1 Apr 2022, Markus Vervier wrote:

> Dear git developer team,
>
> X41 is processing the current RfP

would you kindly provide a bit more context? This seems to come right out
of left field. Is "RfP" a "Request for Proposals"? If so, I am not aware
that the git developer team submitted one...

> and some questions came up regarding the
> improvement / integration of Coverity Scans for git and the estimation of the
> required work:
>
> - Was there a special purpose for the Coverity integration (e.g. custom
> queries for variant analysis or regression testing?) or did you try to
> integrate it as a best practice / general security hygiene tool?

There has been talk about integrating Coverity into our regular CI builds,
but nothing actionable has materialized yet.

Git for Windows has such a CI build, but it is currently broken due to a
backwards-incompatible change in Coverity that will require a human to
adjust the CI definition, which has not yet happened due to lack of time.

> - Could you tell us more about the amount and types of false positives and
> problems you've faced trying to eliminate them? This will help us to
> understand the expectations / requirements for a successful integration of
> Coverity.

From the top of my head, I would estimate about 60-70% of the results to
be false positives.

As Junio pointed out, we do not consider memory to be leaked in one-shot
processes where memory is allocated, once, in the equivalent of a `main()`
function. Sure, we could add a slew of `free()` calls right before exiting
the process, but that's kind of pointless.

Another major source of false positives is our string data structure,
which offers a small-ish static, read-only buffer to get started, but
replaces that with something `malloc()`ed/`realloc()`ed as soon as the
string is about to be manipulated. Yet Coverity insists that we're writing
into a read-only buffer, and get out of bounds, which is simply not true.

Similar issues are reported with our `strvec` data structure that has the
same allocation pattern.

Since the false positives outnumber the valid issues reported by Coverity,
we have not been very eager to sift through new reports.

The list of categories of false positives listed above is not exhaustive,
of course, but combined with how cumbersome it is to get access to the
reports (they cannot be viewed anonymously), you get an idea why we do not
pay all that much attention to Coverity.

> - Could we get access to a sample of the scan results?

Sure, if you direct your web browser to
https://scan.coverity.com/projects/git, there is a button "Add me to
project".

Ciao,
Johannes

>
> Many Thanks
>
> Markus
> --
> Markus Vervier (Managing Director)
> X41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen
> T: +49 241 9809418-0, Fax: -9
> Unternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989
> Geschäftsführer: Markus Vervier
>

Re: Covierty Integration / Improvement

[Johannes Schindelin]

Hi Theodore,

On Sun, 3 Apr 2022, Theodore Ts'o wrote:

> You do need to be the project admin, or someone authorized by the
> project admin, to upload new data for Coverity, or to look at the
> analysis of the Coverity results.

To be precise, there are a couple of roles you can have, one being
Maintainer/Owner, another being Contributor/Member, yet another being
Defect Viewer. IIUC contributors can upload new data.

> I have no idea who the project admin is for git, but I'm sure if you, as
> the Git maintainer showed up and requested to be added as one of the
> project admin, the open source ombudsperson (I don't remember the exact
> title, but they do have someone who interfaces with OSS projects), would
> be happy to oblige.

Junio is already a member. In fact, I bumped him up to be admin :-)

Ciao,
Johannes

Re: Covierty Integration / Improvement

[Johannes Schindelin]

Team,

On Wed, 6 Apr 2022, Johannes Schindelin wrote:

> On Fri, 1 Apr 2022, Markus Vervier wrote:
>
> > - Could you tell us more about the amount and types of false positives
> > and problems you've faced trying to eliminate them? This will help us
> > to understand the expectations / requirements for a successful
> > integration of Coverity.
>
> From the top of my head, I would estimate about 60-70% of the results to
> be false positives.
>
> As Junio pointed out, we do not consider memory to be leaked in one-shot
> processes where memory is allocated, once, in the equivalent of a
> `main()` function. Sure, we could add a slew of `free()` calls right
> before exiting the process, but that's kind of pointless.
>
> Another major source of false positives is our string data structure,
> which offers a small-ish static, read-only buffer to get started, but
> replaces that with something `malloc()`ed/`realloc()`ed as soon as the
> string is about to be manipulated. Yet Coverity insists that we're
> writing into a read-only buffer, and get out of bounds, which is simply
> not true.
>
> Similar issues are reported with our `strvec` data structure that has
> the same allocation pattern.
>
> Since the false positives outnumber the valid issues reported by
> Coverity, we have not been very eager to sift through new reports.
>
> The list of categories of false positives listed above is not
> exhaustive, of course, but combined with how cumbersome it is to get
> access to the reports (they cannot be viewed anonymously), you get an
> idea why we do not pay all that much attention to Coverity.

I have fixed Git for Windows' Coverity build and started to sift through
the 154 new defects reported as of v2.36.0-rc0.

Sadly, there is now a new class of overwhelming false positives: Coverity
claims that "strbuf_addstr does not [NUL-]terminate", which is of course
false. Specifically, Coverity explains that:

/strbuf.c
296 void strbuf_add(struct strbuf *sb, const void *data, size_t len)
297 {
298        strbuf_grow(sb, len);
   1. string_copy: Calling memcpy copies a source string data to sb->buf.
   2. string_null_source: The argument sb->buf will not be
      null[sic!]-terminated, because either the source string is not
      null-terminated, or the length of source string data is greater than
      or equal to the size argument len.
299        memcpy(sb->buf + sb->len, data, len);
300        strbuf_setlen(sb, sb->len + len);
301 }

In other words, it misses the fact that `strbuf_setlen()` _does_ set
`sb->buf[sb->len] = '\0'` (I assume that Coverity gets confused by the
`slopbuf` once again).

I stopped after the first 30-40 instances of "String not null terminated"
reports because my time is a bit too expensive to spend on reports like
that. Among the reported issues I looked at, there were two false
positives where Coverity misinterpreted how much space was allocated (and
thought we'd overrun, which we don't), the rest were those NUL-termination
false positives.

Ciao,
Johannes

Re: Covierty Integration / Improvement

[Theodore Ts'o]

On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:
> I have fixed Git for Windows' Coverity build and started to sift through
> the 154 new defects reported as of v2.36.0-rc0.
> 
> Sadly, there is now a new class of overwhelming false positives: Coverity
> claims that "strbuf_addstr does not [NUL-]terminate", which is of course
> false.

It should be possible to suppress this by uploading a Coverity model
file.  See[1] for more details:

[1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model

I've suppressed a similar issue by using the attribute __nonstring,
but I don't think that will work for git, because strbuf->buf really
*is* a NUL-terminated string, where as in ext4 we have some fields
which are designed to be NUL padded, but it is *not* guaranteed to be
NUL-terminated:

#ifndef __nonstring
#ifdef __has_attribute
#if __has_attribute(__nonstring__)
#define __nonstring                    __attribute__((__nonstring__))
#else
#define __nonstring
#endif /* __has_attribute(__nonstring__) */
#else
# define __nonstring
#endif /* __has_attribute */
#endif /* __nonstring */

struct ext2_super_block {
       ...
/*068*/	__u8	s_uuid[16] __nonstring;		/* 128-bit uuid for volume */
/*078*/	__u8	s_volume_name[EXT2_LABEL_LEN] __nonstring;	/* volume name */
       ...
};

(This is needed to suppress warnings by Clang as well.)

Using __nonstring will result in attempts to use s_volume_name in "C"
string context to give a warning, which is why this isn't right for
strbuf->buf.

Cheers,

						- Ted

Re: Covierty Integration / Improvement

[Junio C Hamano]

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:
>> I have fixed Git for Windows' Coverity build and started to sift through
>> the 154 new defects reported as of v2.36.0-rc0.
>> 
>> Sadly, there is now a new class of overwhelming false positives: Coverity
>> claims that "strbuf_addstr does not [NUL-]terminate", which is of course
>> false.
>
> It should be possible to suppress this by uploading a Coverity model
> file.  See[1] for more details:
>
> [1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model
>
> I've suppressed a similar issue by using the attribute __nonstring,
> but I don't think that will work for git, because strbuf->buf really
> *is* a NUL-terminated string, where as in ext4 we have some fields
> which are designed to be NUL padded, but it is *not* guaranteed to be
> NUL-terminated:

That is very much expected from filesystem code, for which strncmp()
and friends were invented for ;-)

> (This is needed to suppress warnings by Clang as well.)
>
> Using __nonstring will result in attempts to use s_volume_name in "C"
> string context to give a warning, which is why this isn't right for
> strbuf->buf.

Indeed.  We do aim to make reading .buf member as NUL-terminated
string safe, so it would make it very inconvenient to warn against
such uses.

Thanks.

Re: Covierty Integration / Improvement

[Markus Vervier]

Hi Johannes,

On 4/6/22 00:17, Johannes Schindelin wrote:
> On Fri, 1 Apr 2022, Markus Vervier wrote:
>> X41 is processing the current RfP
> would you kindly provide a bit more context? This seems to come right out
> of left field. Is "RfP" a "Request for Proposals"? If so, I am not aware
> that the git developer team submitted one...

thank you and everyone else for their comments. To clear up the context:

The OSTIF (https://ostif.org) is organizing a security audit for git
and one of the questions was about Coverity and if the results it gave 
in the past could be verified and/or improved.

Many Thanks

Markus
-- 
Markus Vervier (Managing Director)
X41 D-Sec GmbH, Dennewartstr. 25-27, D-52068 Aachen
T: +49 241 9809418-0, Fax: -9
Unternehmenssitz: Aachen, Amtsgericht Aachen: HRB19989
Geschäftsführer: Markus Vervier

Re: Covierty Integration / Improvement

[Johannes Schindelin]

Hi Theodore,

On Wed, 6 Apr 2022, Theodore Ts'o wrote:

> On Wed, Apr 06, 2022 at 05:08:37PM +0200, Johannes Schindelin wrote:
> > I have fixed Git for Windows' Coverity build and started to sift through
> > the 154 new defects reported as of v2.36.0-rc0.
> >
> > Sadly, there is now a new class of overwhelming false positives: Coverity
> > claims that "strbuf_addstr does not [NUL-]terminate", which is of course
> > false.
>
> It should be possible to suppress this by uploading a Coverity model
> file.  See[1] for more details:
>
> [1] https://community.synopsys.com/s/article/practical-example-of-coverity-function-model

Right, I know about this, my apologies for being too succinct. For the
record, here is how we submit the Coverity builds in Git for Windows:

https://github.com/git-for-windows/build-extra/blob/bea0c37bdc3737843b7808269b077b30e51c67fb/please.sh#L1793-L1925

Unfortunately, you cannot see the model file because that is not provided
per build, but globally for the entire project. Here is the model file we
currently use:

-- snip --
/* modelfile for git */

char strbuf_slopbuf[64];

void *malloc(size_t);
void *calloc(size_t, size_t);
void *realloc(void *, size_t);
void free(void *);

void *xrealloc(void *ptr, size_t size)
{
	void *ret = realloc(ptr, size);
	if (!ret) __coverity_panic__();
	return ret;
}

void *xmalloc(size_t size)
{
	void *mem = malloc(size);
	if (!mem) __coverity_panic__();
	return mem;
}

void xcalloc(size_t num, size_t size)
{
	void *ret = calloc(num, size);
	if (!ret)  __coverity_panic__();
	return ret;
}

void usage(const char *err) {
  __coverity_panic__();
}

void usagef(const char *err, ...) {
  __coverity_panic__();
}

void die(const char *err, ...)  {
  __coverity_panic__();
}

void die_errno(const char *err, ...) {
  __coverity_panic__();
}
-- snap --

I _guess_ we could "help" Coverity by providing some alternative
implementation of `strbuf_add()` that does not use `memcpy()` but instead
a loop that explicitly NUL-terminates the string.

But it feels wrong to do that because that would weaken the analysis
because Coverity would not actually analyze the code that is executed
anymore.

> I've suppressed a similar issue by using the attribute __nonstring,
> but I don't think that will work for git, because strbuf->buf really
> *is* a NUL-terminated string, where as in ext4 we have some fields
> which are designed to be NUL padded, but it is *not* guaranteed to be
> NUL-terminated:
>
> #ifndef __nonstring
> #ifdef __has_attribute
> #if __has_attribute(__nonstring__)
> #define __nonstring                    __attribute__((__nonstring__))
> #else
> #define __nonstring
> #endif /* __has_attribute(__nonstring__) */
> #else
> # define __nonstring
> #endif /* __has_attribute */
> #endif /* __nonstring */
>
> struct ext2_super_block {
>        ...
> /*068*/	__u8	s_uuid[16] __nonstring;		/* 128-bit uuid for volume */
> /*078*/	__u8	s_volume_name[EXT2_LABEL_LEN] __nonstring;	/* volume name */
>        ...
> };
>
> (This is needed to suppress warnings by Clang as well.)
>
> Using __nonstring will result in attempts to use s_volume_name in "C"
> string context to give a warning, which is why this isn't right for
> strbuf->buf.

Correct. I guess we could fool around with the model file until those
false positives are gone, but I have to admit that I cannot justify the
time to work on this.

Ciao,
Johannes

Re: Covierty Integration / Improvement

[Johannes Schindelin]

Hi Markus,

On Thu, 7 Apr 2022, Markus Vervier wrote:

> On 4/6/22 00:17, Johannes Schindelin wrote:
> > On Fri, 1 Apr 2022, Markus Vervier wrote:
> > > X41 is processing the current RfP
> > would you kindly provide a bit more context? This seems to come right out
> > of left field. Is "RfP" a "Request for Proposals"? If so, I am not aware
> > that the git developer team submitted one...
>
> thank you and everyone else for their comments. To clear up the context:
>
> The OSTIF (https://ostif.org) is organizing a security audit for git
> and one of the questions was about Coverity and if the results it gave in the
> past could be verified and/or improved.

Thank you for the context!

If OSTIF can help us get better support from Coverity (as you can see at
https://github.com/git-for-windows/build-extra/commit/23eea104 I could
have wished for a better experience there), I am all for it!

Out of curiosity: are you (or is OSTIF) affiliated with Synopsys somehow?

If not, have you considered if you could help us getting a comprehensive
CodeQL coverage instead? Theoretically, CodeQL should be able to do the
same as Coverity, while allowing us to tweak the analysis in a lot more
powerful ways than Coverity (most notably, it should allow us to reduce
the number of false positives rather dramatically).

It is the number of knobs CodeQL allows that has looked too daunting for
me to give it more than a cursory try [*1*].

Thank you,
Johannes

Footnote *1*: I had played with CodeQL last year but was called away to a
more pressing project, therefore this is woefully incomplete:
https://github.com/git-for-windows/git/compare/main...dscho:codeql

Re: Covierty Integration / Improvement

[Derek Zimmer]

Hello all, (this is a resend, google mail arbitrarily decides to
switch out of plaintext whenever it likes)

Thank you for the conversations about Coverity. After some internal
discussions and negotiating with our security partners, we have
secured some engineers directly from Github who want to work on CodeQL
for Git. They will do the work of getting CodeQL working, do a scan,
and then evaluate how much work getting CodeQL into a usable state for
Git is by looking at the false positive rate and figuring out what can
be muted with rules, and the false negative rate vs Coverity / other
current tests and create some custom tests.

This should give us a good baseline on what is needed to get Git a
solid security scanner for the CI/CD pipeline. We are focusing on
making the results useful and removing nags to save as much developer
time as possible when using it, so that you get the security benefits
without significant drawbacks.

Do we have anyone here that is interested in helping the team set up
CodeQL? I'm sure the engineers will have some questions, especially
regarding the current Coverity mess and what needs to improve in order
to make this new setup more usable.

Derek Zimmer
Executive Director
Open Source Technology Improvement Fund

Derek Zimmer
Executive Director
Open Source Technology Improvement Fund


On Tue, May 10, 2022 at 12:43 PM Derek Zimmer <derek@ostif.org> wrote:
>
> Hello all,
>
> Thank you for the conversations about Coverity. After some internal discussions and negotiating with our security partners, we have secured some engineers directly from Github who want to work on CodeQL for Git. They will do the work of getting CodeQL working, do a scan, and then evaluate how much work getting CodeQL into a usable state for Git is by looking at the false positive rate and figuring out what can be muted with rules, and the false negative rate vs Coverity / other current tests and create some custom tests.
>
> This should give us a good baseline on what is needed to get Git a solid security scanner for the CI/CD pipeline. We are focusing on making the results useful and removing nags to save as much developer time as possible when using it, so that you get the security benefits without significant drawbacks.
>
> Do we have anyone here that is interested in helping the team set up CodeQL? I'm sure the engineers will have some questions, especially regarding the current Coverity mess and what needs to improve in order to make this new setup more usable.
>
> Derek Zimmer
> Executive Director
> Open Source Technology Improvement Fund
>
> On Mon, Apr 11, 2022 at 1:49 PM Derek Zimmer <derek@ostif.org> wrote:
>>
>> Hello all,
>>
>> Answers inline + more context
>>
>> > If OSTIF can help us get better support from Coverity (as you can see at
>> > https://github.com/git-for-windows/build-extra/commit/23eea104 I could
>> > have wished for a better experience there), I am all for it!
>>
>> We may be able to convince them to help based on the volume of work that we do with many open source projects. Not helping one open source project may seem like a small loss to them. Not getting recommended to hundreds of high profile projects because of lacking support is different. It is especially concerning that this particular bug likely affects a huge number of customers.
>>
>> > If not, have you considered if you could help us getting a comprehensive
>> > CodeQL coverage instead? Theoretically, CodeQL should be able to do the
>> > same as Coverity, while allowing us to tweak the analysis in a lot more
>> > powerful ways than Coverity (most notably, it should allow us to reduce
>> > the number of false positives rather dramatically).
>>
>> This is absolutely an option, although we may have to petition Google / OpenSSF / the Linux Foundation for a slight increase in funding, as setting up CodeQL from scratch is a much more laborious task than setting up rules for an existing Coverity setup. We absolutely can do this, but we'd have to split it into a second project with separate funding in order to keep the primary work moving forward while we work out the details.
>>
>> If you ultimately think that setting up CodeQL will yield better results overall for Git, I can get started on finding the resources to get it done immediately. (I have a meeting with the Linux Foundation tomorrow.)
>>
>> If we are going to go with CodeQL as a separate project, we can drop the Coverity work from the current SoW/Proposal and proceed with all of the other action items.
>>
>> Let me know your thoughts everyone on what best suits Git here. It sounds to me like CodeQL is the way to go but if there's a compelling argument for Coverity we can explore that.
>>
>> All the best,
>>
>> Derek Zimmer
>> Executive Director
>> Open Source Technology Improvement Fund
>>
>>
>> On Thu, Apr 7, 2022 at 6:58 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>>>
>>> Hi Markus,
>>>
>>> On Thu, 7 Apr 2022, Markus Vervier wrote:
>>>
>>> > On 4/6/22 00:17, Johannes Schindelin wrote:
>>> > > On Fri, 1 Apr 2022, Markus Vervier wrote:
>>> > > > X41 is processing the current RfP
>>> > > would you kindly provide a bit more context? This seems to come right out
>>> > > of left field. Is "RfP" a "Request for Proposals"? If so, I am not aware
>>> > > that the git developer team submitted one...
>>> >
>>> > thank you and everyone else for their comments. To clear up the context:
>>> >
>>> > The OSTIF (https://ostif.org) is organizing a security audit for git
>>> > and one of the questions was about Coverity and if the results it gave in the
>>> > past could be verified and/or improved.
>>>
>>> Thank you for the context!
>>>
>>> If OSTIF can help us get better support from Coverity (as you can see at
>>> https://github.com/git-for-windows/build-extra/commit/23eea104 I could
>>> have wished for a better experience there), I am all for it!
>>>
>>> Out of curiosity: are you (or is OSTIF) affiliated with Synopsys somehow?
>>>
>>> If not, have you considered if you could help us getting a comprehensive
>>> CodeQL coverage instead? Theoretically, CodeQL should be able to do the
>>> same as Coverity, while allowing us to tweak the analysis in a lot more
>>> powerful ways than Coverity (most notably, it should allow us to reduce
>>> the number of false positives rather dramatically).
>>>
>>> It is the number of knobs CodeQL allows that has looked too daunting for
>>> me to give it more than a cursory try [*1*].
>>>
>>> Thank you,
>>> Johannes
>>>
>>> Footnote *1*: I had played with CodeQL last year but was called away to a
>>> more pressing project, therefore this is woefully incomplete:
>>> https://github.com/git-for-windows/git/compare/main...dscho:codeql