Re: Coverity false positives triggered by gnulib's implementation of base64

[Kamil Dudka]

Hi Bruno,

On Wednesday, May 8, 2019 10:15:37 AM CEST Bruno Haible wrote:
> Hi Kamil,
> 
> > Coverity Analysis 2019.03 incorrectly marks the input argument
> > of base64_encode(), and conseuqnetly base64_encode_alloc(), as
> > tainted_data_sink because it sees byte-level operations on the input.
> > 
> > It triggered the following false positives in the cryptsetup project:
> > 
> > Error: TAINTED_SCALAR:
> > lib/luks2/luks2_digest_pbkdf2.c:117: tainted_data_argument: Calling
> > function "crypt_random_get" taints argument "salt".
> > lib/luks2/luks2_digest_pbkdf2.c:157: tainted_data: Passing tainted
> > variable "salt" to a tainted sink.
> > 
> > Error: TAINTED_SCALAR:
> > lib/luks2/luks2_keyslot_luks2.c:445: tainted_data_argument: Calling
> > function "crypt_random_get" taints argument "salt".
> > lib/luks2/luks2_keyslot_luks2.c:448: tainted_data: Passing tainted
> > variable "salt" to a tainted sink.
> > 
> > 
> > ... but it can affect other gnulib-based projects, too.  Would it be
> > possible to apply the following one-line patch on gnulib source code
> > to suppress this class of false positives in gnulib-based projects?
> > 
> > https://gitlab.com/cryptsetup/cryptsetup/commit/75b2610e
> 
> When I read the description of "tainted data" [1] and of the
> recommendations how to deal with such warnings [2], it is clear that
> the warning/error is about the global data flow. Therefore it seems
> inappropriate to me to put annotations about the global data flow
> into gnulib (which is shared among multiple packages).

Yes, the false positives will not appear while analyzing gnulib's code
in isolation, if this is what you mean.  On the other hand, gnulib is
intended to be bundled with other projects, where these false positives
will likely trigger.

You can find a brief definition of taint analysis for example here:

https://www.owasp.org/index.php/Static_Code_Analysis#Taint_Analysis

There are 3 important state-transitions in the data-flow analysis:

(1) obtaining data from untrusted source
(2) sanitizing the data (checking bounds etc.)
(3) unsafe use of untrusted data

gnulib's base64_encode() as seen by Coverity Analysis represents (3)
because its implementation uses byte swaps.  This is a heuristic that
is not always correct, so false positives may happen.  If you ask why
byte swaps are checked, I believe it was inspired by Heartbleed:

https://www.synopsys.com/blogs/software-security/detecting-heartbleed-with-static-analysis/

The inline annotation that I proposed as a patch gives Coverity a hint
that gnulib's implementation of base64_encode() can safely process data
from untrusted sources.  The annotation is specific to the implementation
of the function, not to users of the function.  Thanks to the annotation,
the false positives will be suppressed in all projects that bundle gnulib.

> Therefore, what I would suggest is that you create an inline function
> that merely invokes base64_encode_alloc, use it in line 157 of [3],
> and put your annotation on it.

There is no need for such obfuscation in cryptsetup since they have the
inline annotation already added in their own copy of the base64 module.
I was just trying to improve the developer experience of gnulib itself.

> Does it need to be done in the source code at all?

Yes, in case of gnulib this is the only sensible option.

> Some Coverity tools
> have a UI that allows the developers to mark the findings as "false
> positive" or as "handled" without touching the source code, and these
> marks persist across modifications of the source code. Don't you have
> this possibility for this Coverity tool?

Yes, various tools exist to waive false positives.  The problem is that
instances of these tools do not share data with each other in the universe.
Consequently, developers have to repeatedly review these false positives
and waive them in each single instance of these tools.  And even worse with
gnulib because these false positives are usually not matched across different
project that bundle gnulib, even if you have a single instance of the waiving
tool in your organisation.

Kamil

> Bruno
> 
> [1] https://community.synopsys.com/s/article/Tainted-data-in-Coverity
> [2]
> https://stackoverflow.com/questions/24772247/how-to-handle-coverity-error-t
> ainted-scalar-in-fread [3]
> https://fossies.org/linux/cryptsetup/lib/luks2/luks2_digest_pbkdf2.c