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?
- if so, the assertion is scary
- if not, the FOR loop condition is scary.
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?
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
??? */
+ }
+ }
================
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.
================
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;
================
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.)
_______________________________________________
Swan-dev mailing list
[email protected]
https://lists.libreswan.org/mailman/listinfo/swan-dev
