[Swan-dev] making coverity happy

[D. Hugh Redelmeier]

| From: Paul Wouters <paul at nohats.ca>

| On Wed, 6 Sep 2017, D. Hugh Redelmeier wrote:
| 
| > I just noticed this code in 0f69bb7d2d33c64739e83388e8a9e4d914a8971a
| > 
| > +                               struct payload_digest *p;
| > +                               for (p = md->chain[ISAKMP_NEXT_v2N]; p !=
| > NULL; p = p->next) {
| > +                                       if (p->payload.v2n.isan_type ==
| > v2N_SIGNATURE_HASH_ALGORITHMS)
| > +                                               break;
| > +                               }
| > +                               passert(p != NULL); /* Make Coverity happy
| > */
| > +                               if
| > (!negotiate_hash_algo_from_notification(p,st))
| >                                        return STF_FATAL;
| > 
| > The assertion may have made Coverity happy but it makes me nervous.
| > Based on purely local analysis (I haven't actually tried to understand
| > the code).
| > 
| > Is it OK for p to be NULL at this point?
| 
| It can never be NULL. A previous loop runs over md, storing the fact
| that there is a v2N_SIGNATURE_HASH_ALGORITHMS notify in seen_ntfy_hash.
| The code quoted above is wrapped in a if (seen_ntfy_hash) { } block.

Yes.  In the interest of clarity:

- the FOR should have an empty test

- the passert should be at the start of the FOR's body.

This eliminates useless redundancy.

| When we loop over it at first, we haven't commited to a state yet, so
| we cannot store it yet in st_seen_hashnotify, hence the local variable.

Right.  But it isn't a win (as I point out later).

| > - if not, the FOR loop condition is scary.
| 
| Well, I thought about making it a while(1) loop, since we KNOW the
| payload is there. It would make presumably make coverity happy, but
| that was something I found too scary to do :)

If is naturally a FOR.  With an empty test.

| > And another thing: are multiple v2N_SIGNATURE_HASH_ALGORITHMS
| > notification payloads legal?  If so, what should be done with them?
| > If not, what should be done with them?
| 
| It's not legal. There should only be one. If there are more, I'm happy
| with undefined behaviour (even if it now means "pick the first one")

Back in the good old days, I always diagnosed this kind of thing.
This seems to have been appreciated by implementors of other IKE
daemons.

| > ================
| > 
| > Along the same lines, could
| > 
| > 		if (seen_ntfy_frag)
| > 			st->st_seen_fragvid = TRUE;
| > 
| > be simplified to this?
| > 
| > 		st->st_seen_fragvid = seen_ntfy_frag;
| 
| Did you mis-paste st_seen_fragvid for st_seen_hashnotify ?

No.  I was looking at code just above "if (seen_ntfy_hash) {".

| But yes, we could do:
| 
| 		st->st_seen_hashnotify = seen_ntfy_frag;

No, you cannot.  This incorrectly handles the case where
IMPAIR_IGNORE_HASH_NOTIFY_REQUEST is selected.

| > ================
| > 
| > Perhaps, at an inconsequential cost in CPU time, the seen_ntfy_hash
| > variable could be eliminated, resulting in simpler code.  The loop is
| > moved up two levels in the control structure: one IF is eliminated and
| > one is moved inside the loop.
| > 
| > 		for (struct payload_digest *p = md->chain[ISAKMP_NEXT_v2N]; p
| > != NULL; p = p->next) {
| > 			if (p->payload.v2n.isan_type ==
| > v2N_SIGNATURE_HASH_ALGORITHMS) {
| > 				if (DBGP(IMPAIR_IGNORE_HASH_NOTIFY_REQUEST)) {
| > 					libreswan_log("Impair: Ignoring the
| > Signature hash notify in IKE_SA_INIT Request");
| > 				} else {
| > 					st->st_seen_hashnotify = TRUE;
| > 					if
| > (!negotiate_hash_algo_from_notification(p,st))
| > 						return STF_FATAL;
| > 				}
| > 				break;	/* ??? what about multiple payloads */
| > 			}
| > 		}
| > 
| > (The declaration and setting of the local variable seen_ntfy_frag can
| > then be deleted.)

This parenthetical comment is wrong.  I meant seen_ntfy_hash.

| In one of the two cases, we have not commited to creating a state yet,
| which is why the use of local variables happens. We are looping over
| the notify's to check for a DCOOKIE, and we might send a reply without
| creating a state (preventing ddos)

The code I changed is directly after a new state is created.  That is
the only code that consumes the value of the local variable
seen_ntfy_hash.

Interestingly enough, the only consumer of seen_ntfy_frag is here too.
So setting the st field could be done directly too, in the same loop,
if only it did not break upon finding v2N_SIGNATURE_HASH_ALGORITHMS.

What would happens if negotiate_hash_algo_from_notification were
called multiple times (due to multiple v2N_SIGNATURE_HASH_ALGORITHMS
payloads)?  Would the behaviour be benign?