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

[Kamil Dudka]

On Friday, May 10, 2019 12:13:32 AM CEST Bruno Haible wrote:
> Hi Paul,
> 
> > Sorry, I'm still not following. Unless the tainted data is used to
> > calculate an array index, there's no problem with Heartbleed and the
> > Coverity heuristic should not diagnose a problem.
> 
> Yes, IF they were only using an algorithm and no heuristic,
> base64_encode would not be flagged as a dangerous consumer of
> untrusted input.
> 
> But their article
> https://www.synopsys.com/blogs/software-security/detecting-heartbleed-with-s
> tatic-analysis/ says:
> 
>   "Program analysis is hard and approximations and trade-offs are
>    absolutely mandatory. We’ve found that the best results come from
>    a combination of advanced algorithms and knowledge of idioms that
>    occur in real-world code."
> 
> So they are combining data flow analysis - in order to determine
> that the argument of base64_alloc is untrusted data - with a
> heuristic - "if a function contains array accesses with indices that
> are computed with ntohs calls, we should flag it as dangerous consumer".
> 
> > the proposed comment would be wrong as it would pacify
> > Coverity without fixing the real bug elsewhere.
> 
> The base64_encode function does not make a dangerous array
> access (only to the 'b64c' array). And its result is a string,
> not an integer, and therefore cannot be used as an array index
> either. Therefore adding this comment cannot silence real bugs.
> 
> But maybe it will be sufficient to mask all b64c arguments
> with '& 0x3f', like you already suggested in the other mail?
> 
> Bruno
> 
> diff --git a/lib/base64.c b/lib/base64.c
> index f3f7298..a00e0f4 100644
> --- a/lib/base64.c
> +++ b/lib/base64.c
> @@ -70,7 +70,7 @@ base64_encode_fast (const char *restrict in, size_t inlen,
> char *restrict out) {
>    while (inlen)
>      {
> -      *out++ = b64c[to_uchar (in[0]) >> 2];
> +      *out++ = b64c[(to_uchar (in[0]) >> 2) & 0x3f];
>        *out++ = b64c[((to_uchar (in[0]) << 4) + (to_uchar (in[1]) >> 4)) &
> 0x3f]; *out++ = b64c[((to_uchar (in[1]) << 2) + (to_uchar (in[2]) >> 6)) &
> 0x3f]; *out++ = b64c[to_uchar (in[2]) & 0x3f];
> @@ -103,7 +103,7 @@ base64_encode (const char *restrict in, size_t inlen,
> 
>    while (inlen && outlen)
>      {
> -      *out++ = b64c[to_uchar (in[0]) >> 2];
> +      *out++ = b64c[(to_uchar (in[0]) >> 2) & 0x3f];
>        if (!--outlen)
>          break;
>        *out++ = b64c[((to_uchar (in[0]) << 4)
> 
> Bruno

Thanks!  This also helps to suppress the false positives on cryptsetup
with Coverity Static Analysis version 2019.03.

Kamil