[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paul Eggert
Subject: Re: Coverity false positives triggered by gnulib's implementation of base64
Date: Thu, 9 May 2019 12:14:40 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

On 5/7/19 7:22 AM, Kamil Dudka wrote:
> It triggered the following false positives in the cryptsetup project:
> 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.
> 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.

Can you explain in more detail what these diagnostic messages mean, and
why this is Gnulib's "fault"? For example, how do you know that the
reports are false positives and not true positives? That sort of thing.
Normally I'm a fan of static analysis, but it can go too far sometimes....

> gnulib's base64_encode() as seen by Coverity Analysis represents (3)
> because its implementation uses byte swaps.
base64_encode does not actually swap bytes. For example, it wouldn't be
reasonable for base64.c to include byteswap.h in order to clarify
base64_encode, because the byteswap.h's byteswapping operations wouldn't
help. Also, even if base64_encode were swapping bytes, byte-swapping by
itself is not a problem and I don't see why Heartbleed is relevant here,
as the resulting subscripting operations are obviously in range. So I
would like to to know exactly why Coverity infers that this particular
byte-manipulation is problematic.

For example, does the problem go away if you replace 'b64c[to_uchar
(in[0]) >> 2]' with 'b64c[(to_uchar (in[0]) >> 2) & 0x3f]'? If so, it
would indicate that Coverity merely needs to improve their range analysis.

More generally, does it help if you make simple changes to base64.c that
might clarify the code a bit and in addition pacify Coverity? If so,
that might be preferable. Something like the attached untested patch,
say. The idea is to somehow convince Coverity that the code is OK, while
not making it worse (and maybe making it better).

> /* coverity[-tainted_data_sink: arg-0] */
Just adding a line like that would be too cryptic. We can't expect the
reader to know the ins and outs of a proprietary 3rd-party tool.
Instead, the comment would have to explain what's going on and why the
comment is needed, well enough for the Gnulib audience.

However, I'm hoping that we don't need the line at all, because it
sounds like Coverity might be buggy here.

Attachment: base64.patch
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]