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

[Andrew Cagney]

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/89937c4e/attachment.html>