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

[Andrew Cagney]

On Sat, 17 Oct 2020 at 05:51, Antony Antony <antony at phenome.org> wrote:
>
> I think fix f9fada7234b is worth a closer look.

The source of the change is an IETF discussion around eid5247.  I'm
not sure if that is public.

The upshot is that while these numbers all seem to be drawn from a
common table, they're not.  Instead there are three independent tables
vis:

extern enum_names ikev2_proposal_protocol_id_names; /* 1=IKE SA, 2=AH, 3=ESP */
extern enum_names ikev2_delete_protocol_id_names; /* 1=IKE SA, 2=AH, 3=ESP */
extern enum_names ikev2_notify_protocol_id_names; /* NONE=0, 2=AH,
3=ESP; NOT IKE! */

as to why I didn't include IKE in the ikev2_notify_protocol_id_names
list?  Because it wasn't listed in the table.  The first two tables,
for instance, will reject NONE.

The requirement:

"If the SPI field is empty, this field MUST be sent as zero and
 MUST be ignored on receipt."

is more interesting.  The code needs to conditionally reject the ID
based on the SPI size.  Adding IKE to the notification table isn't
sufficient.  Given this I most likely either:
- missed the requirement, or
- assumed our code was handling this edge correctly
turns out it wasn't.

> Yesterday, when Tuomo noticed this issue and when I fixed it, the issue
> appeared a simple bug.
> On a closer look I think 14e07ddcf2f5 was very delibrate to block


>
> "Notify with  Protocol ID IKE(1)"
>
> Now we know that out in the wild Cisco would send Protocol ID IKE(1).
> Commits f9fada7234b and 83ea2c2f2 allow this.
> I think we should accept it, be more relaxed in what we accept.
>
> I am wondering why we decided to block Protocol ID IKE(1).
> Andrew would you please share your thoughts?
>
> Extra details:
> pluto send with "Notify with Protocol (0)".
> Wireshark packet disector call
> isakmp.notify.protoid Protocol ID: RESERVED (0)
>
> Pluto call it IKEv2_SEC_PROTO_NONE and according to the comment it is also
> delibrate decision to deviate from IANA registry
>
>  * The value '0' is a little odd.  While IANA lists it as Reserved, a
>  * notify payload must use that value for notifications that do not
>  * include an SPI.  Hence 'NONE' is used.
>
> So far I noticed two comments hinting IKE should not be allowed. I found the
> second one this morning. I think there is one more in the ocde about syncing
> Notify Protocol IDs between IKEv1 and IKEv2. I am going to fix the second
> one.
>
> -extern enum_names ikev2_notify_protocol_id_names;      /* NONE=0, 2=AH, 3=ESP; NOT IKE
> +extern enum_names ikev2_notify_protocol_id_names;      /* NONE=0, 1=IKE, 2=AH, 3=ESP;
>
> Before I commit this fix, I have a question.
> Should we re-visit my fixes or even revert them?
>
> iPhone send Protocol ID: RESERVED (0). So far Cisco is the only outliever we
> know of.
>
> regards,
> -antony
>
> On Fri, Oct 16, 2020 at 02:36:20PM +0000, Antony Antony wrote:
> > New commits:
> > commit f9fada7234b69d069d00d22163229bfe071ef70e
> > Author: Antony Antony <antony at phenome.org>
> > Date:   Fri Oct 16 14:21:43 2020 +0200
> >
> >     ikev2: allow Protocol ID IKE in Notify
> >
> >     Cisco send Protocol ID IKE(1) in notifications in IKEv2 IKE_INIT.
> >
> >     Commit 14e07ddcf2f5 would not allow "1" and drop the message.
> >
> >     RFC7296 #section-3.10 is a bit vauge.
> >     "If the SPI field is empty, this field MUST be sent as zero and
> >      MUST be ignored on receipt."
> >
> >     In this case SPI is empty. May be Cisco should not send "1"?
> >     For now lets allow "1".
> >
> >     Pluto log message
> >
> >     | ***parse IKEv2 Vendor ID Payload:
> >     |    next payload type: ISAKMP_NEXT_v2N (0x29)
> >
> >     | Now let's proceed with payload (ISAKMP_NEXT_v2N)
> >     Protocol ID of IKEv2 Notify Payload has an unknown value: 1 (0x1)
> >     "westnet-eastnet-ipv4-psk-ikev2" #1: malformed payload in packet
> >     | processing: STOP state #0 (in process_md() at demux.c:275)
> >
> >     Wireshark disect.
> >
> >        Payload: Vendor ID (43) : Cisco Copyright
> >             Next payload: Notify (41)
> >
> >         Payload: Notify (41) - NAT_DETECTION_SOURCE_IP
> >             Next payload: Notify (41)
> >             0... .... = Critical Bit: Not Critical
> >             .000 0000 = Reserved: 0x00
> >             Payload length: 28
> >             Protocol ID: IKE (1)
> >
> >     ->>> above is what pluto refused
> >
> >             SPI Size: 0
> >             Notify Message Type: NAT_DETECTION_SOURCE_IP (16388)
> >             Notification DATA: 158a0910b35701b6831b2f3ff90993cd57b993ff
> >         Payload: Notify (41) - NAT_DETECTION_DESTINATION_IP
> >
> >     Fixes: 14e07ddcf2f5 ("constants: organize Security Protocol ID name tables inline with IETF")
> >