From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 209.51.188.0/24 X-Spam-Status: No, score=-3.3 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 746EE1F45F for ; Thu, 9 May 2019 19:15:25 +0000 (UTC) Received: from localhost ([127.0.0.1]:59734 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hOoVm-0002Bf-N0 for normalperson@yhbt.net; Thu, 09 May 2019 15:15:22 -0400 Received: from eggs.gnu.org ([209.51.188.92]:51659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hOoVB-0001td-Rt for bug-gnulib@gnu.org; Thu, 09 May 2019 15:14:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hOoVA-000525-NE for bug-gnulib@gnu.org; Thu, 09 May 2019 15:14:45 -0400 Received: from zimbra.cs.ucla.edu ([131.179.128.68]:53890) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hOoVA-000516-Dl for bug-gnulib@gnu.org; Thu, 09 May 2019 15:14:44 -0400 Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 8C4D41619A9; Thu, 9 May 2019 12:14:42 -0700 (PDT) Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id 4mszWFwXGBnG; Thu, 9 May 2019 12:14:41 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 54B2B161990; Thu, 9 May 2019 12:14:41 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id HaGlfQwlgc0Y; Thu, 9 May 2019 12:14:41 -0700 (PDT) Received: from Penguin.CS.UCLA.EDU (Penguin.CS.UCLA.EDU [131.179.64.200]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 340B516198A; Thu, 9 May 2019 12:14:41 -0700 (PDT) Subject: Re: Coverity false positives triggered by gnulib's implementation of base64 To: Kamil Dudka References: <2475310.f135Qeco9B@kdudka-nb> From: Paul Eggert Openpgp: preference=signencrypt Autocrypt: addr=eggert@cs.ucla.edu; prefer-encrypt=mutual; keydata= xsFNBEyAcmQBEADAAyH2xoTu7ppG5D3a8FMZEon74dCvc4+q1XA2J2tBy2pwaTqfhpxxdGA9 Jj50UJ3PD4bSUEgN8tLZ0san47l5XTAFLi2456ciSl5m8sKaHlGdt9XmAAtmXqeZVIYX/UFS 96fDzf4xhEmm/y7LbYEPQdUdxu47xA5KhTYp5bltF3WYDz1Ygd7gx07Auwp7iw7eNvnoDTAl KAl8KYDZzbDNCQGEbpY3efZIvPdeI+FWQN4W+kghy+P6au6PrIIhYraeua7XDdb2LS1en3Ss mE3QjqfRqI/A2ue8JMwsvXe/WK38Ezs6x74iTaqI3AFH6ilAhDqpMnd/msSESNFt76DiO1ZK QMr9amVPknjfPmJISqdhgB1DlEdw34sROf6V8mZw0xfqT6PKE46LcFefzs0kbg4GORf8vjG2 Sf1tk5eU8MBiyN/bZ03bKNjNYMpODDQQwuP84kYLkX2wBxxMAhBxwbDVZudzxDZJ1C2VXujC OJVxq2kljBM9ETYuUGqd75AW2LXrLw6+MuIsHFAYAgRr7+KcwDgBAfwhPBYX34nSSiHlmLC+ KaHLeCLF5ZI2vKm3HEeCTtlOg7xZEONgwzL+fdKo+D6SoC8RRxJKs8a3sVfI4t6CnrQzvJbB n6gxdgCu5i29J1QCYrCYvql2UyFPAK+do99/1jOXT4m2836j1wARAQABzSBQYXVsIEVnZ2Vy dCA8ZWdnZXJ0QGNzLnVjbGEuZWR1PsLBfgQTAQIAKAUCTIByZAIbAwUJEswDAAYLCQgHAwIG FQgCCQoLBBYCAwECHgECF4AACgkQ7ZfpDmKqfjRRGw/+Ij03dhYfYl/gXVRiuzV1gGrbHk+t nfrI/C7fAeoFzQ5tVgVinShaPkZo0HTPf18x6IDEdAiO8Mqo1yp0CtHmzGMCJ50o4Grgfjlr 6g/+vtEOKbhleszN2XpJvpwM2QgGvn/laTLUu8PH9aRWTs7qJJZKKKAb4sxYc92FehPu6FOD 0dDiyhlDAq4lOV2mdBpzQbiojoZzQLMQwjpgCTK2572eK9EOEQySUThXrSIz6ASenp4NYTFH s9tuJQvXk9gZDdPSl3bp+47dGxlxEWLpBIM7zIONw4ks4azgT8nvDZxA5IZHtvqBlJLBObYY 0Le61Wp0y3TlBDh2qdK8eYL426W4scEMSuig5gb8OAtQiBW6k2sGUxxeiv8ovWu8YAZgKJfu oWI+uRnMEddruY8JsoM54KaKvZikkKs2bg1ndtLVzHpJ6qFZC7QVjeHUh6/BmgvdjWPZYFTt N+KA9CWX3GQKKgN3uu988yznD7LnB98T4EUH1HA/GnfBqMV1gpzTvPc4qVQinCmIkEFp83zl +G5fCjJJ3W7ivzCnYo4KhKLpFUm97okTKR2LW3xZzEW4cLSWO387MTK3CzDOx5qe6s4a91Zu ZM/j/TQdTLDaqNn83kA4Hq48UHXYxcIh+Nd8k/3w6lFuoK0wrOFiywjLx+0ur5jmmbecBGHc 1xdhAFHOwU0ETIByZAEQAKaF678T9wyH4wjTrV1Pz3cDEoSnV/0ZUrOT37p1dcGyj/IXq1x6 70HRVahAmk0sZpYc25PF9D5GPYHFWlNjuPU96rDndXB3hedmBRhLdC4bAXjI4DV+bmdVe+q/ IMnlZRaVlm9EiMCVAR6w13sReu7qXkW9r3RwY2AzXskp/tAe4BRKr1Zmbvi2nbnQ6epEC42r Rbx0B1EhjbIQZ5JHGk24iPT7LdBgnNmos5wYjzwNlkMQD5T0Ydzhk7J+UxwA5m46mOhRDC2r FV/A0gm5TLy8DXjv/Esc4gYnYai6SQqnUEVh5LuV8YCJBnijs+Tiw71x1icmn6xGI45EugJO gec+rLypYgpVp4x0HI5T88qBRYCkxH3Kg8Qo+EWNA9A4LRQ9DX8njona0gf0s03tocK8kBN6 6UoqqPtHBnc4eMgBymCflK12eKfd2YYxnyg9cZazWA5VslvTxpm76hbg5oiAEH/Vg/8MxHyA nPhfrgwyPrmJEcVBafdspJnYQxBYNco2LFPIhlOvWh8r4at+s+M3Lb26oUTczlgdW1Sf3SDA 77BMRnF0FQyE+7AzV79MBN4ykiqaezQxtaF1Fy/tvkhffSo8u+dwG0EgJh+te38gTcISVr0G IPplLz6YhjrbHrPRF1CN5UuL9DBGjxuN35RLNVEfta6RUFlR6NctTjvrABEBAAHCwWUEGAEC AA8FAkyAcmQCGwwFCRLMAwAACgkQ7ZfpDmKqfjSrHA/+KzAKvTxRhA9MWNLxIyJ7S5uJ16gs T3oCjZrBKGEhKMOGX4O0GA6VOEryO7QRCCYah3oxSG38IAnNeiwJXgU9Bzkk85UGbPEd7HGF /VSeHCQwWou6jqUDTSDvn9YhNTdG0KXPM74aC+xr2Zow1O2mhXihgWKD0Dw+0LYPnUOsQ0KO FxHXXYHmRrS1OZPU59BLvc+TRhIhafSHKLwbXK+6ckkxBx6h8z5ccpG0Qs4bFhdFYnFrEieD LoGmnE2YLhdV6swJ9VNCS6pLiEohT3fm7aXm15tZOIyzMZhHRSAPblXxQ0ZSWjq8oRrcYNFx c4W1URpAkBCOYJoXvQfD5L3lqAl8TCqDUzYxhH/tJhbDdHrqHH767jaDaTB1+Talp/2AMKwc XNOdiklGxbmHVG6YGl6g8Lrbsu9NZEI4yLlHzuikthJWgz+3vZhVGyNlt+HNIoF6CjDL2omu 5cEq4RDHM44QqPk6l7O0pUvN1mT4B+S1b08RKpqm/ff015E37HNV/piIvJlxGAYz8PSfuGCB 1thMYqlmgdhd9/BabGFbGGYHA6U4/T5zqU+f6xHy1SsAQZ1MSKlLwekBIT+4/cLRGqCHjnV0 q5H/T6a7t5mPkbzSrOLSo4puj+IToNjYyYIDBWzhlA19avOa+rvUjmHtD3sFN7cXWtkGoi8b uNcby4U= Organization: UCLA Computer Science Department Message-ID: <1324b17b-a3a8-e1fe-f781-f360343b89a1@cs.ucla.edu> 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 MIME-Version: 1.0 In-Reply-To: <2475310.f135Qeco9B@kdudka-nb> Content-Type: multipart/mixed; boundary="------------B0F89A40120A7A9E90127A14" Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 131.179.128.68 X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: bug-gnulib@gnu.org Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: "bug-gnulib" This is a multi-part message in MIME format. --------------B0F89A40120A7A9E90127A14 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 5/7/19 7:22 AM, Kamil Dudka wrote: > 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. 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. --------------B0F89A40120A7A9E90127A14 Content-Type: text/x-patch; name="base64.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="base64.patch" diff --git a/lib/base64.c b/lib/base64.c index f3f7298aa..786b0f418 100644 --- a/lib/base64.c +++ b/lib/base64.c @@ -64,11 +64,11 @@ static const char b64c[64] = /* Base64 encode IN array of size INLEN into OUT array. OUT needs to be of length >= BASE64_LENGTH(INLEN), and INLEN needs to be - a multiple of 3. */ + a positive multiple of 3. */ static void base64_encode_fast (const char *restrict in, size_t inlen, char *restrict out) { - while (inlen) + do { *out++ = b64c[to_uchar (in[0]) >> 2]; *out++ = b64c[((to_uchar (in[0]) << 4) + (to_uchar (in[1]) >> 4)) & 0x3f]; @@ -78,6 +78,7 @@ base64_encode_fast (const char *restrict in, size_t inlen, char *restrict out) inlen -= 3; in += 3; } + while (inlen); } /* Base64 encode IN array of size INLEN into OUT array of size OUTLEN. @@ -88,6 +89,9 @@ void base64_encode (const char *restrict in, size_t inlen, char *restrict out, size_t outlen) { + if (!outlen) + return; + /* Note this outlen constraint can be enforced at compile time. I.E. that the output buffer is exactly large enough to hold the encoded inlen bytes. The inlen constraints (of corresponding @@ -101,16 +105,17 @@ base64_encode (const char *restrict in, size_t inlen, return; } - while (inlen && outlen) + if (inlen) + while (true) { *out++ = b64c[to_uchar (in[0]) >> 2]; if (!--outlen) - break; + return; *out++ = b64c[((to_uchar (in[0]) << 4) + (--inlen ? to_uchar (in[1]) >> 4 : 0)) & 0x3f]; if (!--outlen) - break; + return; *out++ = (inlen ? b64c[((to_uchar (in[1]) << 2) @@ -118,18 +123,17 @@ base64_encode (const char *restrict in, size_t inlen, & 0x3f] : '='); if (!--outlen) - break; + return; *out++ = inlen ? b64c[to_uchar (in[2]) & 0x3f] : '='; if (!--outlen) + return; + if (inlen <= 1) break; - if (inlen) - inlen--; - if (inlen) - in += 3; + inlen--; + in += 3; } - if (outlen) - *out = '\0'; + *out = '\0'; } /* Allocate a buffer and store zero terminated base64 encoded data --------------B0F89A40120A7A9E90127A14--