[Swan-dev] Coverity Scan finds issue with programs/pluto/keys.c

Andrew Cagney andrew.cagney at gmail.com
Tue May 4 13:34:16 UTC 2021


Here's a more up-to-date comment:

 * Try all keys from PUBKEY_DB.
 *
 * Return true when searching should stop (not when it succeeded);
 * return false when searching can continue.
 *
 *   Returns  FATAL_DIAG  KEY     tried_cnt
 *    false     NULL     NULL        0      no key, try again
 *    false     NULL     NULL       >0      no key worked, try again
 *    true    <valid>   <valid>     N/A     fatal error caused by KEY
 *    true      NULL    <valid>     N/A     KEY worked

On Tue, 4 May 2021 at 09:18, Andrew Cagney <andrew.cagney at gmail.com> wrote:

>
>
> On Tue, 4 May 2021 at 01:15, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
>> | From: scan-admin at coverity.com
>>
>> |
>> ________________________________________________________________________________________________________
>> | *** CID 1504578:    (FORWARD_NULL)
>> | /programs/pluto/keys.c: 529 in authsig_and_log_using_pubkey()
>> | 523                   stop = try_all_keys("preloaded", pluto_pubkeys,
>> &s);
>> | 524           }
>> | 525
>> | 526           if (s.fatal_diag != NULL) {
>> | 527                   LLOG_JAMBUF(RC_LOG_SERIOUS, ike->sa.st_logger,
>> buf) {
>> | 528                           jam(buf, "authentication aborted: problem
>> with '");
>> | >>>     CID 1504578:    (FORWARD_NULL)
>> | >>>     Passing null pointer "&s.key->id" to "jam_id", which
>> dereferences it.
>> | 529                           jam_id(buf, &s.key->id,
>> jam_sanitized_bytes);
>> | 530                           jam(buf, "': ");
>> | 531                           jam_diag(buf, s.fatal_diag);
>> | 532                           pfree_diag(&s.fatal_diag);
>> | 533                   }
>> | 534                   return STF_FATAL;
>> | /programs/pluto/keys.c: 529 in authsig_and_log_using_pubkey()
>> | 523                   stop = try_all_keys("preloaded", pluto_pubkeys,
>> &s);
>> | 524           }
>> | 525
>> | 526           if (s.fatal_diag != NULL) {
>> | 527                   LLOG_JAMBUF(RC_LOG_SERIOUS, ike->sa.st_logger,
>> buf) {
>> | 528                           jam(buf, "authentication aborted: problem
>> with '");
>> | >>>     CID 1504578:    (FORWARD_NULL)
>> | >>>     Passing null pointer "&s.key->id" to "jam_id", which
>> dereferences it.
>> | 529                           jam_id(buf, &s.key->id,
>> jam_sanitized_bytes);
>> | 530                           jam(buf, "': ");
>> | 531                           jam_diag(buf, s.fatal_diag);
>> | 532                           pfree_diag(&s.fatal_diag);
>> | 533                   }
>> | 534                   return STF_FATAL;
>>
>> I'm suspicious about the case where both calls to try_all_keys return
>> true.  I'll call this "double stop".
>>
>> According to the comment on try_all_keys, not much is specified about
>> the results when false is returned.
>>
>> Perhaps something like this would fix the (possibly non-existent)
>> problem.  Clearly the message and the message generation should be
>> improved.
>>
>> It isn't clear to me that this actually fixes the problem that
>> Coverity Scan saw.
>>
>
> I suspect Coverity is confused:
>
> /*
>  * Try all keys from PUBKEY_DB.
>  *
>  * Return true when searching should stop (not when it succeeded);
>  * return false when searching can continue.
>  *
>  *   Returns  FATAL_DIAG  KEY     tried_cnt
>  *    false                                 can try again
>  *    true    <valid>   <valid>     N/A     fatal error caused by KEY
>  *    true      NULL     NULL        0      no key found
>  *    true      NULL     NULL       >0      no key worked
>  *    true      NULL    <valid>     N/A     KEY worked
>  */
>
> FATAIL_DIAG => KEY
>
> BTW, the below would defeat the code listing all the keys that were tried.
>
> diff --git a/programs/pluto/keys.c b/programs/pluto/keys.c
>> index 78aa0ec8bd..013fb47885 100644
>> --- a/programs/pluto/keys.c
>> +++ b/programs/pluto/keys.c
>> @@ -521,6 +521,12 @@ stf_status authsig_and_log_using_pubkey(struct
>> ike_sa *ike,
>>         bool stop = try_all_keys("peer",
>> ike->sa.st_remote_certs.pubkey_db, &s);
>>         if (!stop) {
>>                 stop = try_all_keys("preloaded", pluto_pubkeys, &s);
>> +               if (!stop) {
>> +                       LLOG_JAMBUF(RC_LOG_SERIOUS, ike->sa.st_logger,
>> buf) {
>> +                               jam(buf, "authentication aborted: could
>> not find public keyskeys");
>> +                       }
>> +                       return STF_FATAL;
>> +               }
>>         }
>>
>>         if (s.fatal_diag != NULL) {
>> _______________________________________________
>> Swan-dev mailing list
>> Swan-dev at lists.libreswan.org
>> https://lists.libreswan.org/mailman/listinfo/swan-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20210504/372f517e/attachment-0001.html>


More information about the Swan-dev mailing list