[Swan-dev] ikev1_close_message()

Paul Wouters paul at nohats.ca
Sun Aug 12 20:33:02 UTC 2018


On Sun, 12 Aug 2018, D. Hugh Redelmeier wrote:

[padding]

> But the code seems to also call this for some attributes (all to do
> with xauth).  This seems quite screwy.  But is it correct code?  Does
> the xauth RFC prescribe it?

No. There is no RFC, only a draft, and it does not mention the word
"pad":

https://tools.ietf.org/html/draft-ietf-ipsec-isakmp-xauth-06

But RFC 2408 section 3.6 does mention padding for attributes:

https://tools.ietf.org/html/rfc2408#section-3.6

 	If the SA Attributes are not aligned on 4-byte boundaries,
 	then subsequent payloads will not be aligned and any padding will
 	be added at the end of the message to make the message 4-octet
 	aligned.

So it seems to me that padding is appropriate.

> At the start of our repo, we had these comments.  I suspect that I
> wrote them.  This function certainly did not apply to attributes, only
> whole ISAKMP messages.  (At the time it was called close_message and
> was in programs/pluto/ipsec_doi.c).
>
> /* The whole message must be a multiple of 4 octets.
>  * I'm not sure where this is spelled out, but look at
>  * rfc2408 3.6 Transform Payload.
>  * Note: it talks about 4 BYTE boundaries!
>  */
>
> I feel that these comments should be preserved as an explanation of
> our thinking.

Or replace it with the actual RFC text quoted above.

> If xauth attributes need to be padded, does that need to be
> suppressed?  Should that suppression be tied to the suppression of
> ISAKMP message padding?  Some how I doubt this.

No, I don't think so. But I also feel that it is probably best not to
change this in IKEv1 at this point. It might cause new problems if
anyone has decided this mistake should be accepted on their end. Since
IKEv1 at commercial vendors is pretty much on lockdown for the code,
no vendor would update their code to match our update of this handling.

That said, this commit hints that Cisco does not pad, so in that way,
there is a huge chance that not padding xauth attributes will work fine
because otherwise those vendors would fail Cisco interop:

commit 47e956bd75f170b5c8299e76df4b9aa55d1334c2
Author: Lubomir Rintel <lkundrak at v3.sk>
Date:   Wed Oct 28 12:42:13 2015 +0100

     XAUTH: Don't attempt to read attributes when there's just padding

     Libreswan, unlike cisco, likes to add padding when transform payload attributes
     don't line up to 4-octet boundaries while it doesn't seem to be too happy about
     padding, being non-interoperable with itself (unless "ikepad" is turned off):

     [fixes our code to work with ikepad=on]

> I'd guess that if xauth attributes need to be padded, they should use
> a different mechanism.

I'll leave that up to you. In a way, I don't care as much anymore about
the IKEv1 code and I urge us to try and minimize changes to it.

Paul


More information about the Swan-dev mailing list