[Swan-dev] making coverity happy

[Paul Wouters]

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.

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.

> - 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 :)

> 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")

> This structure makes more sense to me:
>
> +                               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 (!negotiate_hash_algo_from_notification(p,st))
> +                                                       return STF_FATAL;
> +                                               break;   /* are multiple legal ??? */
> +                                       }
> +                               }

That makes sense, yes.


> ================
>
> There's another slight puzzle with this code
>
> 		if (seen_ntfy_hash) {
> 			if (!DBGP(IMPAIR_IGNORE_HASH_NOTIFY_REQUEST)) {
> 				st->st_seen_hashnotify = TRUE;
> 				...
> 			} else {
> 				st->st_seen_hashnotify = FALSE;
> 				...
> 			}
> 		}
>
> What is st->st_seen_hashnotify set to when !seen_ntfy_hash?
>
> Perhaps it defaults to FALSE.  If so, the last assignment to it is
> redundant and thus confusing (at least to me).  If not, it feels like
> a bug.

It's a redundant call. I'm fine with removing it.

> ================
>
> 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 ?
But yes, we could do:

 		st->st_seen_hashnotify = seen_ntfy_frag;


> ================
>
> 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.)

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)

Paul