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 <[email protected]> wrote: > > > On Tue, 4 May 2021 at 01:15, D. Hugh Redelmeier <[email protected]> wrote: > >> | From: [email protected] >> >> | >> ________________________________________________________________________________________________________ >> | *** 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 >> [email protected] >> https://lists.libreswan.org/mailman/listinfo/swan-dev >> >
_______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
