[Swan-dev] [Swan-commit] ikev2: allow Protocol ID IKE in Notify

Paul Wouters paul at nohats.ca
Sat Oct 17 15:14:16 UTC 2020


On Sat, 17 Oct 2020, Antony Antony wrote:

> I think fix f9fada7234b is worth a closer look.
>
> Yesterday, when Tuomo noticed this issue and when I fixed it, the issue
> appeared a simple bug.

Thanks for finding and fixing it so quickly!

> On a closer look I think 14e07ddcf2f5 was very delibrate to block
>
> "Notify with  Protocol ID IKE(1)"

>From the commit message, I assume Andrew noticed that these numbers were
not really one Registry entry, but different Registries that just happen
to be (basically) the same. So he split them. I think that is fine, but
clearly not all implementations treat these as different ones as can be
seen by Cisco.....

> Now we know that out in the wild Cisco would send Protocol ID IKE(1).
> Commits f9fada7234b and 83ea2c2f2 allow this.

It does, but it is technically not RFC compliant still. If we receive it
set to say 13, the RFC still tells us we MUST ignore it if the SPI
fields are 0. Currently, we don't do that. In theory, this could become
a problem if a new child sa protocol would appear, like ESPv5 or IPFTS
or something that would be a new protocol in addition to AH or ESP.
Altough it is extremely unlikely, it would mean libreswan would break
when that happens.

> I am wondering why we decided to block Protocol ID IKE(1).
> Andrew would you please share your thoughts?

I think it was unintentional. Andrew just read the RFC and saw that
only 0, 2 and 3 were valid entries and he made the enum for that?

> Should we re-visit my fixes or even revert them?

I think the current fixes are fine. I think it would be harder to allow
_any_ number in this field as we are supposed to do. I'm okay with
fixing that issue later - eg after we do a quick 4.1 release now.

My preference would be to do a 4.1 release right now, so it is ready
for Monday morning, so we avoid people possibly trying out 4.0 and
seeing failures.

Paul


More information about the Swan-dev mailing list