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

[Bruno Haible]

Hi Kamil,

> 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.

Ah, thanks for explaining. Now I agree: base64_encode produces the
warning because of the (x << n) | (y >> m) expression patterns that
resemble a byte swap. It would do so also for any other program that
contains a base64_encode invocation with untrusted input as argument.

> > Does it need to be done in the source code at all?
> 
> Yes, in case of gnulib this is the only sensible option.
> ...
> 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.

So, I propose to bite the bullet, but at least put a reasonable comment.


2019-05-09  Kamil Dudka  <address@hidden>
            Bruno Haible  <address@hidden>

        base64: Avoid false positive warning from Coverity.
        * lib/base64.c (base64_encode): Add special comment for Coverity.

diff --git a/lib/base64.c b/lib/base64.c
index f3f7298..80428bb 100644
--- a/lib/base64.c
+++ b/lib/base64.c
@@ -84,6 +84,11 @@ base64_encode_fast (const char *restrict in, size_t inlen, 
char *restrict out)
    If OUTLEN is less than BASE64_LENGTH(INLEN), write as many bytes as
    possible.  If OUTLEN is larger than BASE64_LENGTH(INLEN), also zero
    terminate the output buffer. */
+/* Tell Coverity that this function works fine when called with IN
+   pointing to untrusted input.  By default, Coverity, seeing the value
+   shift expressions below, thinks that it is dangerous to call this
+   function with untrusted input.
+   coverity[-tainted_data_sink: arg-0]  */
 void
 base64_encode (const char *restrict in, size_t inlen,
                char *restrict out, size_t outlen)